You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/01/08 03:33:52 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #2661: fs: file_dup2 shouldn't hold the file list lock
xiaoxiang781216 opened a new pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661
## Summary
the argument passed to file_dup2 doesn't always come from task file list so it doesn't make sense to hold the file list lock and then it is better to do the protection in the new function files_dupfd2.
And remove fs_dupfd and fs_dupfd2 internal functions, let's call either nx_dup/nx_dup2 or file_dup/file_dup2 instead just like other fs api: xxx->nx_xxx->file_xxx
## Impact
Improve the implentation detail, shouldn't change the behaviour.
## Testing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao merged pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
anchao merged pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661#discussion_r554425556
##########
File path: fs/vfs/fs_dup.c
##########
@@ -50,6 +50,47 @@
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: file_dup
+ *
+ * Description:
+ * Equivalent to the standard dup() function except that it
+ * accepts a struct file instance instead of a file descriptor.
+ *
+ * Returned Value:
+ * The new file descriptor is returned on success; a negated errno value
+ * is returned on any failure.
+ *
+ ****************************************************************************/
+
+int file_dup(FAR struct file *filep, int minfd)
+{
+ struct file filep2;
+ int fd2;
+ int ret;
+
+ /* Let file_dup2() do the real work */
+
+ memset(&filep2, 0, sizeof(filep2));
+ ret = file_dup2(filep, &filep2);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Then allocate a new file descriptor for the inode */
+
+ fd2 = files_allocate(filep2.f_inode, filep2.f_oflags,
+ filep2.f_pos, filep2.f_priv, minfd);
+ if (fd2 < 0)
+ {
+ file_close(&filep2);
+ return fd2;
Review comment:
It should be fine to return -EINTR not -EBADF/-EMFILE since it report the real problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661#discussion_r554423440
##########
File path: fs/vfs/fs_dup.c
##########
@@ -50,6 +50,47 @@
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: file_dup
+ *
+ * Description:
+ * Equivalent to the standard dup() function except that it
+ * accepts a struct file instance instead of a file descriptor.
+ *
+ * Returned Value:
+ * The new file descriptor is returned on success; a negated errno value
+ * is returned on any failure.
+ *
+ ****************************************************************************/
+
+int file_dup(FAR struct file *filep, int minfd)
+{
+ struct file filep2;
+ int fd2;
+ int ret;
+
+ /* Let file_dup2() do the real work */
+
+ memset(&filep2, 0, sizeof(filep2));
+ ret = file_dup2(filep, &filep2);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Then allocate a new file descriptor for the inode */
+
+ fd2 = files_allocate(filep2.f_inode, filep2.f_oflags,
+ filep2.f_pos, filep2.f_priv, minfd);
+ if (fd2 < 0)
+ {
+ file_close(&filep2);
+ return fd2;
Review comment:
Should we be concerned if f`iles_allocate` fails at `_files_semtake` and returns other values than `EBADF` and `EMFILE`?
##########
File path: fs/vfs/fs_dup.c
##########
@@ -50,6 +50,47 @@
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: file_dup
+ *
+ * Description:
+ * Equivalent to the standard dup() function except that it
+ * accepts a struct file instance instead of a file descriptor.
+ *
+ * Returned Value:
+ * The new file descriptor is returned on success; a negated errno value
+ * is returned on any failure.
+ *
+ ****************************************************************************/
+
+int file_dup(FAR struct file *filep, int minfd)
+{
+ struct file filep2;
+ int fd2;
+ int ret;
+
+ /* Let file_dup2() do the real work */
+
+ memset(&filep2, 0, sizeof(filep2));
+ ret = file_dup2(filep, &filep2);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Then allocate a new file descriptor for the inode */
+
+ fd2 = files_allocate(filep2.f_inode, filep2.f_oflags,
+ filep2.f_pos, filep2.f_priv, minfd);
+ if (fd2 < 0)
+ {
+ file_close(&filep2);
+ return fd2;
Review comment:
Should we be concerned if `files_allocate` fails at `_files_semtake` and returns other values than `EBADF` and `EMFILE`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661#issuecomment-759295866
LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661#discussion_r554423440
##########
File path: fs/vfs/fs_dup.c
##########
@@ -50,6 +50,47 @@
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: file_dup
+ *
+ * Description:
+ * Equivalent to the standard dup() function except that it
+ * accepts a struct file instance instead of a file descriptor.
+ *
+ * Returned Value:
+ * The new file descriptor is returned on success; a negated errno value
+ * is returned on any failure.
+ *
+ ****************************************************************************/
+
+int file_dup(FAR struct file *filep, int minfd)
+{
+ struct file filep2;
+ int fd2;
+ int ret;
+
+ /* Let file_dup2() do the real work */
+
+ memset(&filep2, 0, sizeof(filep2));
+ ret = file_dup2(filep, &filep2);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Then allocate a new file descriptor for the inode */
+
+ fd2 = files_allocate(filep2.f_inode, filep2.f_oflags,
+ filep2.f_pos, filep2.f_priv, minfd);
+ if (fd2 < 0)
+ {
+ file_close(&filep2);
+ return fd2;
Review comment:
Should we be concerned if `files_allocate` fails at `_files_semtake` and returns other values than `EBADF` and `EMFILE`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661#discussion_r554423440
##########
File path: fs/vfs/fs_dup.c
##########
@@ -50,6 +50,47 @@
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: file_dup
+ *
+ * Description:
+ * Equivalent to the standard dup() function except that it
+ * accepts a struct file instance instead of a file descriptor.
+ *
+ * Returned Value:
+ * The new file descriptor is returned on success; a negated errno value
+ * is returned on any failure.
+ *
+ ****************************************************************************/
+
+int file_dup(FAR struct file *filep, int minfd)
+{
+ struct file filep2;
+ int fd2;
+ int ret;
+
+ /* Let file_dup2() do the real work */
+
+ memset(&filep2, 0, sizeof(filep2));
+ ret = file_dup2(filep, &filep2);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Then allocate a new file descriptor for the inode */
+
+ fd2 = files_allocate(filep2.f_inode, filep2.f_oflags,
+ filep2.f_pos, filep2.f_priv, minfd);
+ if (fd2 < 0)
+ {
+ file_close(&filep2);
+ return fd2;
Review comment:
Should we be concerned if f`iles_allocate` fails at `_files_semtake` and returns other values than `EBADF` and `EMFILE`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2661: fs: file_dup2 shouldn't hold the file list lock
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2661:
URL: https://github.com/apache/incubator-nuttx/pull/2661#discussion_r554425556
##########
File path: fs/vfs/fs_dup.c
##########
@@ -50,6 +50,47 @@
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: file_dup
+ *
+ * Description:
+ * Equivalent to the standard dup() function except that it
+ * accepts a struct file instance instead of a file descriptor.
+ *
+ * Returned Value:
+ * The new file descriptor is returned on success; a negated errno value
+ * is returned on any failure.
+ *
+ ****************************************************************************/
+
+int file_dup(FAR struct file *filep, int minfd)
+{
+ struct file filep2;
+ int fd2;
+ int ret;
+
+ /* Let file_dup2() do the real work */
+
+ memset(&filep2, 0, sizeof(filep2));
+ ret = file_dup2(filep, &filep2);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Then allocate a new file descriptor for the inode */
+
+ fd2 = files_allocate(filep2.f_inode, filep2.f_oflags,
+ filep2.f_pos, filep2.f_priv, minfd);
+ if (fd2 < 0)
+ {
+ file_close(&filep2);
+ return fd2;
Review comment:
It should be fine to return -EINTR not -EBADF/-EMFILE since it report the real problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org