Donny9 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2084821846
##########
include/nuttx/fs/fs.h:
##########
@@ -490,20 +495,20 @@ struct file
* (file descriptor % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK) as column index.
*/
-struct filelist
+struct fdlist
Review Comment:
fdlist->fdtable? like linux ?
##########
fs/inode/fs_files.c:
##########
@@ -554,16 +663,25 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
{
int i = minfd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
int j = minfd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
- FAR struct filelist *list;
+ FAR struct fdlist *list;
FAR struct file *filep;
+ FAR struct fd *fd;
irqstate_t flags;
int ret;
/* Get the file descriptor list. It should not be NULL in this context. */
list = nxsched_get_files_from_tcb(tcb);
- /* Find free file */
+ /* Allocate a new file pointer */
+
+ filep = fs_heap_malloc(sizeof(struct file));
Review Comment:
we should create a pool for alloc file structure, and cache this memory when
release file to void frequency malloc/free.
##########
fs/inode/fs_files.c:
##########
@@ -139,26 +198,26 @@ static int files_extend(FAR struct filelist *list, size_t
row)
return -EMFILE;
}
- files = fs_heap_malloc(sizeof(FAR struct file *) * row);
- DEBUGASSERT(files);
- if (files == NULL)
+ fds = fs_heap_malloc(sizeof(FAR struct fd *) * row);
+ DEBUGASSERT(fds);
+ if (fds == NULL)
{
return -ENFILE;
Review Comment:
ENOMEM
##########
fs/inode/fs_files.c:
##########
@@ -231,8 +289,7 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void
*arg)
return;
}
- filep = files_fget_by_index(&ctcb->group->tg_filelist,
- i, j, NULL);
+ filep = files_fget_by_index(&ctcb->group->tg_filelist, i, j, NULL);
Review Comment:
tg_filelist -> tg_fdlist
##########
include/nuttx/fs/fs.h:
##########
@@ -466,6 +466,15 @@ struct file
off_t f_pos; /* File position */
FAR struct inode *f_inode; /* Driver or file system interface */
FAR void *f_priv; /* Per file driver private data */
+#if CONFIG_FS_BACKTRACE > 0
+ FAR void *f_backtrace[CONFIG_FS_BACKTRACE]; /* Backtrace to while
file opens */
+#endif
+};
+
+struct fd
+{
+ struct file *f_file; /* The file associated with descriptor */
Review Comment:
FAR
##########
fs/inode/fs_files.c:
##########
@@ -554,16 +663,25 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
{
int i = minfd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
int j = minfd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
- FAR struct filelist *list;
+ FAR struct fdlist *list;
FAR struct file *filep;
+ FAR struct fd *fd;
irqstate_t flags;
int ret;
/* Get the file descriptor list. It should not be NULL in this context. */
list = nxsched_get_files_from_tcb(tcb);
- /* Find free file */
+ /* Allocate a new file pointer */
+
+ filep = fs_heap_malloc(sizeof(struct file));
+ if (filep == NULL)
+ {
+ return -ENFILE;
Review Comment:
ENOMEM
##########
include/nuttx/fs/fs.h:
##########
@@ -490,20 +495,20 @@ struct file
* (file descriptor % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK) as column index.
*/
-struct filelist
+struct fdlist
{
- spinlock_t fl_lock; /* Manage access to the file list */
+ spinlock_t fl_lock; /* Manage access to the file descriptor list */
uint8_t fl_rows; /* The number of rows of fl_files array */
Review Comment:
fl_files->fl_fds
##########
fs/inode/fs_files.c:
##########
@@ -66,61 +84,102 @@
* Name: files_fget_by_index
****************************************************************************/
-static FAR struct file *files_fget_by_index(FAR struct filelist *list,
- int l1, int l2, FAR bool *new)
+static FAR struct file *files_fget_by_index(FAR struct fdlist *list,
+ int l1, int l2,
+ FAR struct fd_flags *fdflags)
{
FAR struct file *filep;
+ FAR struct fd *fd;
irqstate_t flags;
flags = spin_lock_irqsave_notrace(&list->fl_lock);
- filep = &list->fl_files[l1][l2];
- spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
-#ifdef CONFIG_FS_REFCOUNT
- if (filep->f_inode != NULL)
- {
- /* When the reference count is zero but the inode has not yet been
- * released, At this point we should return a null pointer
- */
+ /* The callers have ensured that no out of bounds access can happen here */
- int32_t refs = atomic_read(&filep->f_refs);
- do
+ fd = &list->fl_fds[l1][l2];
+
+ /* The file pointer is removed before the file is freed, so there is no
+ * race condition here as long as we take a reference to the file before
+ * releasing file list lock.
+ */
+
+ filep = fd->f_file;
+ if (filep)
+ {
+ fs_reffilep(filep);
+ if (fdflags)
Review Comment:
fdflags != NULL
##########
fs/inode/fs_files.c:
##########
@@ -576,30 +694,39 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
ret = files_extend(list, i + 1);
if (ret < 0)
{
- return ret;
+ goto errout_with_fp;
}
flags = spin_lock_irqsave_notrace(&list->fl_lock);
}
do
{
- filep = &list->fl_files[i][j];
- if (filep->f_inode == NULL)
+ fd = &list->fl_fds[i][j];
+ if (fd->f_file == NULL)
{
- filep->f_oflags = oflags;
+ fd->f_file = filep;
+ filep->f_oflags = oflags & ~O_CLOEXEC;
filep->f_pos = pos;
filep->f_inode = inode;
filep->f_priv = priv;
#ifdef CONFIG_FS_REFCOUNT
atomic_set(&filep->f_refs, 1);
#endif
#ifdef CONFIG_FDSAN
- filep->f_tag_fdsan = 0;
+ fd->f_tag_fdsan = 0;
#endif
#ifdef CONFIG_FDCHECK
- filep->f_tag_fdcheck = 0;
+ fd->f_tag_fdcheck = 0;
#endif
+ if (oflags & O_CLOEXEC)
Review Comment:
fd->f_cloexec = !!(oflags & O_CLOCEXEC);
##########
fs/inode/fs_files.c:
##########
@@ -246,6 +303,85 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void
*arg)
}
}
+/****************************************************************************
+ * Name: fs_closefilep
+ *
+ * Description:
+ * Close an existing file pointer and free the allocated memory.
+ *
+ * Input Parameters:
+ * filep - The file pointer.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; a negated errno value is returned on
+ * any failure.
+ *
+ ****************************************************************************/
+
+int fs_closefilep(FAR struct file *filep)
+{
+ int ret;
+
+ ret = file_close(filep);
+ if (ret >= 0)
+ {
+ fs_heap_free(filep);
+ }
+
+ return ret;
+}
+
+/****************************************************************************
+ * Name: nx_dup2_from_tcb
+ *
+ * Description:
+ * nx_dup2_from_tcb() is similar to the standard 'dup2' interface
+ * except that is not a cancellation point and it does not modify the
+ * errno variable.
+ *
+ * nx_dup2_from_tcb() is an internal NuttX interface and should not be
+ * called from applications.
+ *
+ * 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 nx_dup2_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2)
+{
+ /* If fd1 is a valid file descriptor, and fd1 == fd2, then dup2() does
+ * nothing, and returns fd2.
+ */
+
+ if (fd1 == fd2)
+ {
Review Comment:
don‘t need for line 359-379? because nx_dup3_from_tcb will covert this
branch.
##########
fs/inode/fs_files.c:
##########
@@ -66,61 +84,102 @@
* Name: files_fget_by_index
****************************************************************************/
-static FAR struct file *files_fget_by_index(FAR struct filelist *list,
- int l1, int l2, FAR bool *new)
+static FAR struct file *files_fget_by_index(FAR struct fdlist *list,
+ int l1, int l2,
+ FAR struct fd_flags *fdflags)
{
FAR struct file *filep;
+ FAR struct fd *fd;
irqstate_t flags;
flags = spin_lock_irqsave_notrace(&list->fl_lock);
- filep = &list->fl_files[l1][l2];
- spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
-#ifdef CONFIG_FS_REFCOUNT
- if (filep->f_inode != NULL)
- {
- /* When the reference count is zero but the inode has not yet been
- * released, At this point we should return a null pointer
- */
+ /* The callers have ensured that no out of bounds access can happen here */
- int32_t refs = atomic_read(&filep->f_refs);
- do
+ fd = &list->fl_fds[l1][l2];
+
+ /* The file pointer is removed before the file is freed, so there is no
+ * race condition here as long as we take a reference to the file before
+ * releasing file list lock.
+ */
+
+ filep = fd->f_file;
+ if (filep)
+ {
+ fs_reffilep(filep);
+ if (fdflags)
{
- if (refs == 0)
- {
- filep = NULL;
- break;
- }
+ fdflags->f_cloexec = fd->f_cloexec;
+#ifdef CONFIG_FDSAN
+ fdflags->f_tag_fdsan = fd->f_tag_fdsan;
+#endif
+#ifdef CONFIG_FDCHECK
+ fdflags->f_tag_fdcheck = fd->f_tag_fdcheck;
+#endif
}
- while (!atomic_try_cmpxchg(&filep->f_refs, &refs, refs + 1));
- }
- else if (new == NULL)
- {
- filep = NULL;
- }
- else if (atomic_fetch_add(&filep->f_refs, 1) == 0)
- {
- atomic_fetch_add(&filep->f_refs, 1);
- *new = true;
}
-#else
- if (filep->f_inode == NULL && new == NULL)
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+
+ return filep;
+}
+
+/****************************************************************************
+ * Name: files_fget_by_fd
+ ****************************************************************************/
+
+static FAR struct file *files_fget_by_fd(FAR struct fdlist *list, int fd,
+ FAR struct fd_flags *flags)
+{
+ return files_fget_by_index(list, fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK,
+ fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK, flags);
+}
+
+/****************************************************************************
+ * Name: files_finstall
+ ****************************************************************************/
+
+static FAR struct file *files_finstall(FAR struct fdlist *list, int fd,
+ FAR struct file *filep,
+ FAR struct fd_flags *fdflags)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct file *oldfilep;
+ irqstate_t flags;
+
+ /* Atomically replace the file pointer */
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ oldfilep = list->fl_fds[i][j].f_file;
+ list->fl_fds[i][j].f_file = filep;
+
+ if (flags)
Review Comment:
flags -> fdflags
##########
fs/inode/fs_files.c:
##########
@@ -66,61 +84,102 @@
* Name: files_fget_by_index
****************************************************************************/
-static FAR struct file *files_fget_by_index(FAR struct filelist *list,
- int l1, int l2, FAR bool *new)
+static FAR struct file *files_fget_by_index(FAR struct fdlist *list,
+ int l1, int l2,
+ FAR struct fd_flags *fdflags)
{
FAR struct file *filep;
+ FAR struct fd *fd;
irqstate_t flags;
flags = spin_lock_irqsave_notrace(&list->fl_lock);
- filep = &list->fl_files[l1][l2];
- spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
-#ifdef CONFIG_FS_REFCOUNT
- if (filep->f_inode != NULL)
- {
- /* When the reference count is zero but the inode has not yet been
- * released, At this point we should return a null pointer
- */
+ /* The callers have ensured that no out of bounds access can happen here */
- int32_t refs = atomic_read(&filep->f_refs);
- do
+ fd = &list->fl_fds[l1][l2];
+
+ /* The file pointer is removed before the file is freed, so there is no
+ * race condition here as long as we take a reference to the file before
+ * releasing file list lock.
+ */
+
+ filep = fd->f_file;
+ if (filep)
Review Comment:
filep != NULL, ditto
##########
fs/inode/fs_files.c:
##########
@@ -266,25 +402,18 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void
*arg)
*
****************************************************************************/
-static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2,
- int flags)
+int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2, int flags)
{
- FAR struct filelist *list;
+ FAR struct fdlist *list;
FAR struct file *filep1;
- FAR struct file *filep;
-#ifdef CONFIG_FDCHECK
- uint8_t f_tag_fdcheck;
-#endif
-#ifdef CONFIG_FDSAN
- uint64_t f_tag_fdsan;
-#endif
- bool new = false;
+ FAR struct file *filep2;
+ struct fd_flags fdflags;
int count;
int ret;
if (fd1 == fd2)
{
- return fd1;
+ return -EINVAL;
Review Comment:
keep
##########
fs/inode/fs_files.c:
##########
@@ -371,15 +478,15 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int
fd1, int fd2,
*
****************************************************************************/
-void files_initlist(FAR struct filelist *list)
+void files_initlist(FAR struct fdlist *list)
Review Comment:
files_initlist -> fd_initlist?
--
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.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
