[incubator-nuttx] 01/02: fs: file_dup2 shouldn't hold the file list lock

[ar...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

archer pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git

commit 1e5bfa623aa93b918566e8dc0e2f9c1a1037f45e
Author: Xiang Xiao <xi...@xiaomi.com>
AuthorDate: Sun Jan 3 00:53:46 2021 +0800

    fs: file_dup2 shouldn't hold the file list lock
    
    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
    
    Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
    Change-Id: Ibf02cea9b0b275e7472f9c04fd66b9242285b957
---
 fs/inode/fs_files.c | 93 +++++++++++++++++++++++++++++------------------------
 fs/inode/inode.h    | 14 ++++++++
 fs/vfs/fs_dupfd2.c  | 29 +----------------
 3 files changed, 66 insertions(+), 70 deletions(-)

diff --git a/fs/inode/fs_files.c b/fs/inode/fs_files.c
index 31117cb..5298018 100644
--- a/fs/inode/fs_files.c
+++ b/fs/inode/fs_files.c
@@ -135,7 +135,6 @@ void files_releaselist(FAR struct filelist *list)
 
 int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
 {
-  FAR struct filelist *list;
   FAR struct inode *inode;
   struct file temp;
   int ret;
@@ -150,33 +149,13 @@ int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
       return OK;
     }
 
-  list = nxsched_get_files();
-
-  /* The file list can be NULL under two cases:  (1) One is an obscure
-   * cornercase:  When memory management debug output is enabled.  Then
-   * there may be attempts to write to stdout from malloc before the group
-   * data has been allocated.  The other other is (2) if this is a kernel
-   * thread.  Kernel threads have no allocated file descriptors.
-   */
-
-  if (list != NULL)
-    {
-      ret = _files_semtake(list);
-      if (ret < 0)
-        {
-          /* Probably canceled */
-
-          return ret;
-        }
-    }
-
   /* Increment the reference count on the contained inode */
 
   inode = filep1->f_inode;
   ret   = inode_addref(inode);
   if (ret < 0)
     {
-      goto errout_with_sem;
+      return ret;
     }
 
   /* Then clone the file structure */
@@ -211,7 +190,8 @@ int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
 
       if (ret < 0)
         {
-          goto errout_with_inode;
+          inode_release(inode);
+          return ret;
         }
     }
 
@@ -225,26 +205,7 @@ int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
   /* Return the file structure */
 
   memcpy(filep2, &temp, sizeof(temp));
-
-  if (list != NULL)
-    {
-      _files_semgive(list);
-    }
-
   return OK;
-
-  /* Handle various error conditions */
-
-errout_with_inode:
-  inode_release(inode);
-
-errout_with_sem:
-  if (list != NULL)
-    {
-      _files_semgive(list);
-    }
-
-  return ret;
 }
 
 /****************************************************************************
@@ -294,6 +255,54 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
 }
 
 /****************************************************************************
+ * Name: files_dupfd2
+ *
+ * Description:
+ *   Clone a file descriptor to a specific descriptor number.
+ *
+ * Returned Value:
+ *   fd2 is returned on success; a negated errno value is return on
+ *   any failure.
+ *
+ ****************************************************************************/
+
+int files_dupfd2(int fd1, int fd2)
+{
+  FAR struct filelist *list;
+  int ret;
+
+  if (fd1 < 0 || fd1 >= CONFIG_NFILE_DESCRIPTORS ||
+      fd2 < 0 || fd2 >= CONFIG_NFILE_DESCRIPTORS)
+    {
+      return -EBADF;
+    }
+
+  /* Get the file descriptor list.  It should not be NULL in this context. */
+
+  list = nxsched_get_files();
+  DEBUGASSERT(list != NULL);
+
+  ret = _files_semtake(list);
+  if (ret < 0)
+    {
+      /* Probably canceled */
+
+      return ret;
+    }
+
+  /* Perform the dup2 operation */
+
+  ret = file_dup2(&list->fl_files[fd1], &list->fl_files[fd2]);
+  _files_semgive(list);
+  if (ret < 0)
+    {
+      return ret;
+    }
+
+  return fd2;
+}
+
+/****************************************************************************
  * Name: files_close
  *
  * Description:
diff --git a/fs/inode/inode.h b/fs/inode/inode.h
index dea5753..f2c696e 100644
--- a/fs/inode/inode.h
+++ b/fs/inode/inode.h
@@ -390,6 +390,20 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
                    FAR void *priv, int minfd);
 
 /****************************************************************************
+ * Name: files_dupfd2
+ *
+ * Description:
+ *   Clone a file descriptor to a specific descriptor number.
+ *
+ * Returned Value:
+ *   fd2 is returned on success; a negated errno value is return on
+ *   any failure.
+ *
+ ****************************************************************************/
+
+int files_dupfd2(int fd1, int fd2);
+
+/****************************************************************************
  * Name: files_close
  *
  * Description:
diff --git a/fs/vfs/fs_dupfd2.c b/fs/vfs/fs_dupfd2.c
index d74ccb4..68fcc54 100644
--- a/fs/vfs/fs_dupfd2.c
+++ b/fs/vfs/fs_dupfd2.c
@@ -64,32 +64,5 @@
 
 int fs_dupfd2(int fd1, int fd2)
 {
-  FAR struct file *filep1;
-  FAR struct file *filep2 = NULL;
-  int ret;
-
-  /* Get the file structures corresponding to the file descriptors. */
-
-  ret = fs_getfilep(fd1, &filep1);
-  if (ret >= 0)
-    {
-      ret = fs_getfilep(fd2, &filep2);
-    }
-
-  if (ret < 0)
-    {
-      return ret;
-    }
-
-  DEBUGASSERT(filep1 != NULL && filep2 != NULL);
-
-  /* Perform the dup2 operation */
-
-  ret = file_dup2(filep1, filep2);
-  if (ret < 0)
-    {
-      return ret;
-    }
-
-  return fd2;
+  return files_dupfd2(fd1, fd2);
 }