Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 merged PR #16499: URL: https://github.com/apache/nuttx/pull/16499 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2963235865 > > > please add a commit message explaining the change > > > > > > could you point out which patch need more info? @jerpelea > > [4aaeb80](https://github.com/apache/nuttx/commit/4aaeb809bab26369cb835d65107bdf69152710e1) Done~ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
jerpelea commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-296159 > > please add a commit message explaining the change > > could you point out which patch need more info? @jerpelea https://github.com/apache/nuttx/pull/16499/commits/4aaeb809bab26369cb835d65107bdf69152710e1 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2961589925 > please add a commit message explaining the change could you point out which patch need more info? @jerpelea -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2959482238 > please add a commit message to [0ec77cf](https://github.com/apache/nuttx/commit/0ec77cf1375923da6208322190849597075752ad) and change the title to tools/pynuttx: update fs.py base on new structure fd and file Done~ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2958604587 > > > Fix the race condition issues pointed out by @xiaoxiang781216 and then this is fine to merge by me. I will not review this any longer as this is not my code anymore. > > > > > > The new patch still follow your code base, but detail is refine since FS is a very important subsystem and this patch series change the life cycle management, so it's always good to have more expert to review and refine the patch. > > Yes, thanks for spending a substantial amount of time doing the review. I think this is ready to merge now. There is a practical reason why I cannot spend any more time with NuttX as I will be moving on to a different line of work very soon. This means this shall be my last contribution (of any kind) to this project. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2958601775 @jlaitine The changes made this time are indeed quite significant, involving modifications to the core relationship between file descriptors and file structures. We've made over 140 adjustments internally, and including testing, it took us a week to complete.  Our original intention was always to improve the patch as much as possible. The new PR has been revised following @pussuw the previous approach, and also maintained the original contributor as the owner of the patch. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on code in PR #16499:
URL: https://github.com/apache/nuttx/pull/16499#discussion_r2137501582
##
fs/inode/fs_files.c:
##
@@ -587,37 +621,138 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
#endif
}
+/
+ * Name: fdlist_allocate
+ *
+ * Description:
+ * Allocate a struct fd instance and associate it with an empty file
+ * instance. The difference between this function and
+ * file_allocate_from_inode is that this function is only used for
+ * placeholder purposes. Later, the caller will initialize the file entity
+ * through the returned filep.
+ *
+ * The fd allocated by this function can be released using fdlist_close.
+ *
+ * After the function call is completed, it will hold a reference count
+ * for the filep. Therefore, when the filep is no longer in use, it is
+ * necessary to call file_put to release the reference count, in order
+ * to avoid a race condition where the file might be closed during
+ * this process.
+ *
+ * Returned Value:
+ * Returns the file descriptor == index into the files array on success;
+ * a negated errno value is returned on any failure.
+ *
+ /
+
+int fdlist_allocate(FAR struct fdlist *list, int oflags,
+int minfd, FAR struct file **filep)
+{
+ int fd;
+
+ *filep = fs_heap_zalloc(sizeof(struct file));
+ if (*filep == NULL)
+{
+ return -ENOMEM;
+}
+
+ fd = fdlist_dupfile(list, oflags, minfd, *filep);
+ if (fd < 0)
+{
+ fs_heap_free(*filep);
Review Comment:
done~
##
fs/inode/fs_files.c:
##
@@ -587,37 +621,138 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
#endif
}
+/
+ * Name: fdlist_allocate
+ *
+ * Description:
+ * Allocate a struct fd instance and associate it with an empty file
+ * instance. The difference between this function and
+ * file_allocate_from_inode is that this function is only used for
+ * placeholder purposes. Later, the caller will initialize the file entity
+ * through the returned filep.
+ *
+ * The fd allocated by this function can be released using fdlist_close.
+ *
+ * After the function call is completed, it will hold a reference count
+ * for the filep. Therefore, when the filep is no longer in use, it is
+ * necessary to call file_put to release the reference count, in order
+ * to avoid a race condition where the file might be closed during
+ * this process.
+ *
+ * Returned Value:
+ * Returns the file descriptor == index into the files array on success;
+ * a negated errno value is returned on any failure.
+ *
+ /
+
+int fdlist_allocate(FAR struct fdlist *list, int oflags,
+int minfd, FAR struct file **filep)
+{
+ int fd;
+
+ *filep = fs_heap_zalloc(sizeof(struct file));
+ if (*filep == NULL)
+{
+ return -ENOMEM;
+}
+
+ fd = fdlist_dupfile(list, oflags, minfd, *filep);
+ if (fd < 0)
+{
+ fs_heap_free(*filep);
+}
+ else
+{
+ file_ref(*filep);
Review Comment:
Done~
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2957958025 > > Fix the race condition issues pointed out by @xiaoxiang781216 and then this is fine to merge by me. I will not review this any longer as this is not my code anymore. > > The new patch still follow your code base, but detail is refine since FS is a very important subsystem and this patch series change the life cycle management, so it's always good to have more expert to review and refine the patch. Yes, thanks for spending a substantial amount of time doing the review. I think this is ready to merge now. There is a practical reason why I cannot spend any more time with NuttX as I will be moving on to a different line of work very soon. This means this shall be my last contribution (of any kind) to this project. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2957935536 > Fix the race condition issues pointed out by @xiaoxiang781216 and then this is fine to merge by me. I will not review this any longer as this is not my code anymore. The new patch still follow your code base, but detail is refine since FS is a very important subsystem, so it's always good to have more expert to review and refine the patch. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
jlaitine commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2957885073 Why are the original patches, *which we tested to be working correctly*, partially re-written in this PR, instead of pointing out if there were errors in those in the closed PR? I completely disagree with this approach. Sorry to intervene, but *YOU CAN'T DO IT LIKE THIS*. Please: 1) Please re-open the previous PR 2) If there are *bugs* or *architectural issues* with that one, please point them out 3) Integrate that PR, it already went through a lengthy review, and we invested a lot of time in testing and reviewing that one in our organization(s) 4) If any bugs are found later * add new patches to fix them * This current process makes no sense to me. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2957928622 > Why are the original patches, _which we tested to be working correctly_, partially re-written in this PR, instead of pointing out if there were errors in those in the closed PR? > > I completely disagree with this approach. Sorry to intervene, but _YOU CAN'T DO IT LIKE THIS_. > > Please: > > 1. Please re-open the previous PR > it's closed by @pussuw , we are asking @pussuw giving @Donny9 to update that patch before creating a new one, bu @pussuw suggest us to create a new one. > 2. If there are _bugs_ or _architectural issues_ with that one, please point them out > Most comments are fixed in this patch, if you really want I can repeat all comments in that pr again. > 3. Integrate that PR, it already went through a lengthy review, and we invested a lot of time in testing and reviewing that one in our organization(s) > We also take huge time to review, refine and test @pussuw 's work, actually several bugs, uncompleted implementation and better approach update in this pr. > 4. If any bugs are found later * add new patches to fix them * > But why not fix all found issues before merging it? > > This current process makes no sense to me. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16499:
URL: https://github.com/apache/nuttx/pull/16499#discussion_r2136966092
##
fs/inode/fs_files.c:
##
@@ -587,37 +621,138 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
#endif
}
+/
+ * Name: fdlist_allocate
+ *
+ * Description:
+ * Allocate a struct fd instance and associate it with an empty file
+ * instance. The difference between this function and
+ * file_allocate_from_inode is that this function is only used for
+ * placeholder purposes. Later, the caller will initialize the file entity
+ * through the returned filep.
+ *
+ * The fd allocated by this function can be released using fdlist_close.
+ *
+ * After the function call is completed, it will hold a reference count
+ * for the filep. Therefore, when the filep is no longer in use, it is
+ * necessary to call file_put to release the reference count, in order
+ * to avoid a race condition where the file might be closed during
+ * this process.
+ *
+ * Returned Value:
+ * Returns the file descriptor == index into the files array on success;
+ * a negated errno value is returned on any failure.
+ *
+ /
+
+int fdlist_allocate(FAR struct fdlist *list, int oflags,
+int minfd, FAR struct file **filep)
+{
+ int fd;
+
+ *filep = fs_heap_zalloc(sizeof(struct file));
+ if (*filep == NULL)
+{
+ return -ENOMEM;
+}
+
+ fd = fdlist_dupfile(list, oflags, minfd, *filep);
+ if (fd < 0)
+{
+ fs_heap_free(*filep);
Review Comment:
file_put
##
fs/inode/fs_files.c:
##
@@ -587,37 +621,138 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
#endif
}
+/
+ * Name: fdlist_allocate
+ *
+ * Description:
+ * Allocate a struct fd instance and associate it with an empty file
+ * instance. The difference between this function and
+ * file_allocate_from_inode is that this function is only used for
+ * placeholder purposes. Later, the caller will initialize the file entity
+ * through the returned filep.
+ *
+ * The fd allocated by this function can be released using fdlist_close.
+ *
+ * After the function call is completed, it will hold a reference count
+ * for the filep. Therefore, when the filep is no longer in use, it is
+ * necessary to call file_put to release the reference count, in order
+ * to avoid a race condition where the file might be closed during
+ * this process.
+ *
+ * Returned Value:
+ * Returns the file descriptor == index into the files array on success;
+ * a negated errno value is returned on any failure.
+ *
+ /
+
+int fdlist_allocate(FAR struct fdlist *list, int oflags,
+int minfd, FAR struct file **filep)
+{
+ int fd;
+
+ *filep = fs_heap_zalloc(sizeof(struct file));
+ if (*filep == NULL)
+{
+ return -ENOMEM;
+}
+
+ fd = fdlist_dupfile(list, oflags, minfd, *filep);
+ if (fd < 0)
+{
+ fs_heap_free(*filep);
+}
+ else
+{
+ file_ref(*filep);
Review Comment:
need add ref before dup to avoid the race condition
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2957568329 > need update app side too: > > ``` > kernel/fs/cases/fs_getfilep_test.c: In function 'test_nuttx_fs_getfilep01': > Error: kernel/fs/cases/fs_getfilep_test.c:75:9: error: implicit declaration of function 'fs_getfilep' [-Werror=implicit-function-declaration] >75 | ret = fs_getfilep(fileno(fp), &filep); > | ^~~ > Error: kernel/fs/cases/fs_getfilep_test.c:103:3: error: implicit declaration of function 'fs_putfilep' [-Werror=implicit-function-declaration] > 103 | fs_putfilep(filep); > | ^~~ > ``` https://github.com/apache/nuttx-apps/pull/3091 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on code in PR #16499:
URL: https://github.com/apache/nuttx/pull/16499#discussion_r2136838430
##
fs/inode/fs_files.c:
##
@@ -258,70 +292,43 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void
*arg)
*
/
-int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2, int flags)
+int fdlist_dup3(FAR struct fdlist *list, int fd1, int fd2, int flags)
{
- FAR struct filelist *list;
FAR struct file *filep1;
- FAR struct file *filep;
- bool new = false;
- int count;
int ret;
if (fd1 == fd2)
{
- return fd1;
+ return -EINVAL;
}
#ifdef CONFIG_FDCHECK
- fd1 = fdcheck_restore(fd1);
fd2 = fdcheck_restore(fd2);
#endif
/* Get the file descriptor list. It should not be NULL in this context. */
- list = nxsched_get_files_from_tcb(tcb);
- count = files_countlist(list);
-
- if (fd1 < 0 || fd1 >= count || fd2 < 0)
+ if (fd2 < 0)
{
return -EBADF;
}
- if (fd2 >= count)
-{
- ret = files_extend(list, fd2 / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + 1);
- if (ret < 0)
-{
- return ret;
-}
-}
-
- filep1 = files_fget(list, fd1);
- if (filep1 == NULL)
+ ret = fdlist_extend(list,
Review Comment:
Done ~
##
fs/inode/fs_files.c:
##
@@ -587,37 +618,120 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
#endif
}
+/
+ * Name: fdlist_allocate
+ *
+ * Description:
+ * Allocate a struct fd instance and associate it with an empty file
+ * instance. The difference between this function and
+ * file_allocate_from_inode is that this function is only used for
+ * placeholder purposes. Later, the caller will initialize the file entity
+ * through the returned filep.
+ *
+ * The fd allocated by this function can be released using fdlist_close.
+ *
+ * Returned Value:
+ * Returns the file descriptor == index into the files array on success;
+ * a negated errno value is returned on any failure.
+ *
+ /
+
+int fdlist_allocate(FAR struct fdlist *list, int oflags,
+int minfd, FAR struct file **filep)
+{
+ int fd;
+
+ *filep = fs_heap_zalloc(sizeof(struct file));
+ if (*filep == NULL)
+{
+ return -ENOMEM;
+}
+
+ fd = fdlist_dupfile(list, oflags, minfd, *filep);
+ if (fd < 0)
Review Comment:
Done ~
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16499: URL: https://github.com/apache/nuttx/pull/16499#issuecomment-2956563766 need update app side too: ``` kernel/fs/cases/fs_getfilep_test.c: In function 'test_nuttx_fs_getfilep01': Error: kernel/fs/cases/fs_getfilep_test.c:75:9: error: implicit declaration of function 'fs_getfilep' [-Werror=implicit-function-declaration] 75 | ret = fs_getfilep(fileno(fp), &filep); | ^~~ Error: kernel/fs/cases/fs_getfilep_test.c:103:3: error: implicit declaration of function 'fs_putfilep' [-Werror=implicit-function-declaration] 103 | fs_putfilep(filep); | ^~~ ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16499:
URL: https://github.com/apache/nuttx/pull/16499#discussion_r2136222979
##
fs/inode/fs_files.c:
##
@@ -587,37 +618,120 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR
struct inode *inode,
#endif
}
+/
+ * Name: fdlist_allocate
+ *
+ * Description:
+ * Allocate a struct fd instance and associate it with an empty file
+ * instance. The difference between this function and
+ * file_allocate_from_inode is that this function is only used for
+ * placeholder purposes. Later, the caller will initialize the file entity
+ * through the returned filep.
+ *
+ * The fd allocated by this function can be released using fdlist_close.
+ *
+ * Returned Value:
+ * Returns the file descriptor == index into the files array on success;
+ * a negated errno value is returned on any failure.
+ *
+ /
+
+int fdlist_allocate(FAR struct fdlist *list, int oflags,
+int minfd, FAR struct file **filep)
+{
+ int fd;
+
+ *filep = fs_heap_zalloc(sizeof(struct file));
+ if (*filep == NULL)
+{
+ return -ENOMEM;
+}
+
+ fd = fdlist_dupfile(list, oflags, minfd, *filep);
+ if (fd < 0)
Review Comment:
need hold additional reference after succeed and call file_put at the caller
to avoid the race condition
##
fs/inode/fs_files.c:
##
@@ -258,70 +292,43 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void
*arg)
*
/
-int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2, int flags)
+int fdlist_dup3(FAR struct fdlist *list, int fd1, int fd2, int flags)
{
- FAR struct filelist *list;
FAR struct file *filep1;
- FAR struct file *filep;
- bool new = false;
- int count;
int ret;
if (fd1 == fd2)
{
- return fd1;
+ return -EINVAL;
}
#ifdef CONFIG_FDCHECK
- fd1 = fdcheck_restore(fd1);
fd2 = fdcheck_restore(fd2);
#endif
/* Get the file descriptor list. It should not be NULL in this context. */
- list = nxsched_get_files_from_tcb(tcb);
- count = files_countlist(list);
-
- if (fd1 < 0 || fd1 >= count || fd2 < 0)
+ if (fd2 < 0)
{
return -EBADF;
}
- if (fd2 >= count)
-{
- ret = files_extend(list, fd2 / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + 1);
- if (ret < 0)
-{
- return ret;
-}
-}
-
- filep1 = files_fget(list, fd1);
- if (filep1 == NULL)
+ ret = fdlist_extend(list,
Review Comment:
is line 102 right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2956208399 > > Hi @Donny9 it is not possible to add permissions to tiiuae/nuttx, you can push the patches to another branch and make a new PR. > > you don't give the full write permission of your nuttx git to @Donny9, but the branch which already upstream to nuttx like this:  anyway, let's continue review on [the new ](https://github.com/apache/nuttx /pull/16499) and close this pr. Not sure I have rights to do even that. Let's just move to the new PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw closed pull request #16361: fs/vfs: Separate file descriptors from file descriptions URL: https://github.com/apache/nuttx/pull/16361 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2956158379 > Hi @Donny9 it is not possible to add permissions to tiiuae/nuttx, you can push the patches to another branch and make a new PR. you don't give the full write permission of your nuttx git to @Donny9, but the branch which already upstream to nuttx like this:  -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2955578742 @pussuw Hello, could you please grant me write permissions to your GitHub repository? I've made updates to the code locally and need to sync them up, but I'm currently encountering permission issues.   If it's not convenient to grant the permissions, I can submit a new Pull Request (PR) for review instead. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2955595803 Hi @Donny9 it is not possible to add permissions to tiiuae/nuttx, you can push the patches to another branch and make a new PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
jlaitine commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2948240713 @xiaoxiang781216 , Could you elaborate a bit why can't this be integrated and further optimizations / cleanups done on top of this? Does this patchset break something? For me it is fixing a serious issue, so I'd rather see this going in first, and other changes later. Why are we waiting for some cleanup pathces before integrating a PR which fixes a kernel crash? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2945176351 > > I am experiencing kernel crashes related to sockets, sendmsg->psock_sendmsg . > > This PR solves the issue; is this now good to be merged? > > @Donny9 , @xiaoxiang781216 ? > > please wait a moment, because we found that mountpt_operations::dup doesn't need in the new approach and dup function need more change. @Donny9 is refining the patchset and do the internal testing. Does this have something to do with file_dup and file_dup2 (or file pointer duplication in general)? They both seem to fit very poorly into the new approach but did not know whether they can be removed or not. I also suspected that the ops::dup() is redundant but did not know for sure if some kernel component depends on it. Good that you are making a cleanup round for them, but I'd like to remind that even in its current state these patches should not break existing functionality and they fix a massive kernel side crash (for us at least). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2945015695 > I am experiencing kernel crashes related to sockets, sendmsg->psock_sendmsg . > > This PR solves the issue; is this now good to be merged? > > @Donny9 , @xiaoxiang781216 ? please wait a moment, because we found that mountpt_operations::dup doesn't need in the new approach and dup function need more change. @Donny9 is refining the patchset and do the internal 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
jlaitine commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2939352629 I am experiencing kernel crashes related to sockets, sendmsg->psock_sendmsg . This PR solves the issue; is this now good to be merged? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2931222990 > Is this ready for merge? I have seen crashes probably caused by file descriptor race conditions. @Donny9 is testing and refining the patch, the verified patchset will be sent out tomorrow. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
jpaali commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2929009120 Is this ready for merge? I have seen crashes probably caused by file descriptor race conditions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2904247947 > @pussuw please rebase this PR, i will try them on my local enviroment.  The current branch should be conflict free, I don't get conflicts locally nor does gh detect any -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
Donny9 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2904196749 @pussuw please rebase this PR, i will try them on my local enviroment.  -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2101941154
##
fs/inode/fs_files.c:
##
@@ -858,6 +1026,342 @@ int fs_putfilep(FAR struct file *filep)
}
#endif
+/
+ * Name: fs_setcloseonexec
+ *
+ * Description:
+ * Given a file descriptor, set the close on exec flag.
+ *
+ * Input Parameters:
+ * fd - The file descriptor
+ * flag - The close on exec flag
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; a negated errno value is returned on
+ * any failure.
+ *
+ /
+
+int fs_setcloseonexec(int fd, int flag)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct fdlist *list;
+ irqstate_t flags;
+ int ret = OK;
+
+ list = nxsched_get_fdlist();
+
+ /* Perform some sanity checks */
+
+ if (fd < 0 || fd >= fdlist_count(list))
+{
+ return -EBADF;
+}
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ if (list->fl_fds[i][j].f_file == NULL)
+{
+ ret = -EBADF;
+}
+ else if (flag == FD_CLOEXEC)
+{
+ list->fl_fds[i][j].f_cloexec = true;
+}
+ else
+{
+ list->fl_fds[i][j].f_cloexec = false;
+}
+
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+ return ret;
+}
+
+/
+ * Name: fs_getcloseonexec
+ *
+ * Description:
+ * Given a file descriptor, get the close on exec flag.
+ *
+ * Input Parameters:
+ * fd - The file descriptor
+ *
+ * Returned Value:
+ * A non-negative value on success; A negated errno value is returned on
+ * any failure to indicate the nature of the failure.
+ *
+ /
+
+int fs_getcloseonexec(int fd)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct fdlist *list;
+ irqstate_t flags;
+ int ret;
+
+ list = nxsched_get_fdlist();
+
+ /* Perform some sanity checks */
+
+ if (fd < 0 || fd >= fdlist_count(list))
+{
+ return -EBADF;
+}
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ if (list->fl_fds[i][j].f_file == NULL)
+{
+ ret = -EBADF;
+}
+ else
+{
+ ret = list->fl_fds[i][j].f_cloexec ? FD_CLOEXEC : 0;
+}
+
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+ return ret;
+}
+
+/
+ * Name: nx_ioctl
+ *
+ * Description:
+ * nx_ioctl() will perform the operation specified by 'cmd' on an open
+ * file.
+ *
+ * Input Parameters:
+ * fd - File descriptor of the open file
+ * cmd - Identifies the operation to be performed. Command specific
+ * arguments may follow.
+ * arg - The argument.
+ *
+ * Returned Value:
+ * A non-negative value is returned on success; a negated errno value is
+ * returned on any failure.
+ *
+ /
+
+int nx_ioctl(int fd, int cmd, unsigned long arg)
Review Comment:
I moved it to fs_ioctl
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2100055033
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
The flag handling for fdsan / fdcheck now works as before. Thanks @hujun260
for the help.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2099903455
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
fdcheck seems to work much like fdsan, it encodes some information to the
user fd and decodes the actual fd from the user fd when the kernel needs it.
There seem to be many hidden paths that don't do fdcheck_restore which is
causing crashes for me.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2099685831
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
Ok, it seems I have misunderstood how the fdsan tags are inherited (I
thought they were inherited always, but they are not!).
I'll try to summarize how I think fdsan should behave:
- open() -> set fdsan_tag to 0 for the new fd
- dup() -> set fdsan_tag to 0 for the new dup'd fd
- dup2() / dup3() -> set fdsan_tag to 0, if the file was not open, but
preserve fdsan_tag if it was
- fdsan_tag is NEVER inherited from another descriptor, i.e.
- When fork()'ing, the child does NOT inherit fdsan_tag from parent ?
- When dup2()/dup3()'ing newfd does NOT inherit fdsan_tag from oldfd ?
Does this sound correct ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2099685831
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
Ok, it seems I have misunderstood how the fdsan tags are inherited. I'll try
to summarize how I think fdsan should behave:
- open() -> set fdsan_tag to 0 for the new fd
- dup() -> set fdsan_tag to 0 for the new dup'd fd
- dup2() / dup3() -> set fdsan_tag to 0, if the file was not open, but
preserve fdsan_tag if it was
- fdsan_tag is NEVER inherited from another descriptor, i.e.
- When fork()'ing, the child does NOT inherit fdsan_tag from parent ?
- When dup2()/dup3()'ing newfd does NOT inherit fdsan_tag from oldfd ?
Does this sound correct ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2099203038
##
fs/inode/fs_files.c:
##
@@ -858,6 +1026,342 @@ int fs_putfilep(FAR struct file *filep)
}
#endif
+/
+ * Name: fs_setcloseonexec
+ *
+ * Description:
+ * Given a file descriptor, set the close on exec flag.
+ *
+ * Input Parameters:
+ * fd - The file descriptor
+ * flag - The close on exec flag
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; a negated errno value is returned on
+ * any failure.
+ *
+ /
+
+int fs_setcloseonexec(int fd, int flag)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct fdlist *list;
+ irqstate_t flags;
+ int ret = OK;
+
+ list = nxsched_get_fdlist();
+
+ /* Perform some sanity checks */
+
+ if (fd < 0 || fd >= fdlist_count(list))
+{
+ return -EBADF;
+}
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ if (list->fl_fds[i][j].f_file == NULL)
+{
+ ret = -EBADF;
+}
+ else if (flag == FD_CLOEXEC)
+{
+ list->fl_fds[i][j].f_cloexec = true;
+}
+ else
+{
+ list->fl_fds[i][j].f_cloexec = false;
+}
+
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+ return ret;
+}
+
+/
+ * Name: fs_getcloseonexec
+ *
+ * Description:
+ * Given a file descriptor, get the close on exec flag.
+ *
+ * Input Parameters:
+ * fd - The file descriptor
+ *
+ * Returned Value:
+ * A non-negative value on success; A negated errno value is returned on
+ * any failure to indicate the nature of the failure.
+ *
+ /
+
+int fs_getcloseonexec(int fd)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct fdlist *list;
+ irqstate_t flags;
+ int ret;
+
+ list = nxsched_get_fdlist();
+
+ /* Perform some sanity checks */
+
+ if (fd < 0 || fd >= fdlist_count(list))
+{
+ return -EBADF;
+}
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ if (list->fl_fds[i][j].f_file == NULL)
+{
+ ret = -EBADF;
+}
+ else
+{
+ ret = list->fl_fds[i][j].f_cloexec ? FD_CLOEXEC : 0;
+}
+
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+ return ret;
+}
+
+/
+ * Name: nx_ioctl
+ *
+ * Description:
+ * nx_ioctl() will perform the operation specified by 'cmd' on an open
+ * file.
+ *
+ * Input Parameters:
+ * fd - File descriptor of the open file
+ * cmd - Identifies the operation to be performed. Command specific
+ * arguments may follow.
+ * arg - The argument.
+ *
+ * Returned Value:
+ * A non-negative value is returned on success; a negated errno value is
+ * returned on any failure.
+ *
+ /
+
+int nx_ioctl(int fd, int cmd, unsigned long arg)
Review Comment:
I wanted to keep the fl_lock semantics hidden in 1 file (fs_files). I can
move it to fs_ioctl too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2099170414
##
fs/inode/fs_files.c:
##
@@ -858,6 +1026,342 @@ int fs_putfilep(FAR struct file *filep)
}
#endif
+/
+ * Name: fs_setcloseonexec
+ *
+ * Description:
+ * Given a file descriptor, set the close on exec flag.
+ *
+ * Input Parameters:
+ * fd - The file descriptor
+ * flag - The close on exec flag
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; a negated errno value is returned on
+ * any failure.
+ *
+ /
+
+int fs_setcloseonexec(int fd, int flag)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct fdlist *list;
+ irqstate_t flags;
+ int ret = OK;
+
+ list = nxsched_get_fdlist();
+
+ /* Perform some sanity checks */
+
+ if (fd < 0 || fd >= fdlist_count(list))
+{
+ return -EBADF;
+}
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ if (list->fl_fds[i][j].f_file == NULL)
+{
+ ret = -EBADF;
+}
+ else if (flag == FD_CLOEXEC)
+{
+ list->fl_fds[i][j].f_cloexec = true;
+}
+ else
+{
+ list->fl_fds[i][j].f_cloexec = false;
+}
+
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+ return ret;
+}
+
+/
+ * Name: fs_getcloseonexec
+ *
+ * Description:
+ * Given a file descriptor, get the close on exec flag.
+ *
+ * Input Parameters:
+ * fd - The file descriptor
+ *
+ * Returned Value:
+ * A non-negative value on success; A negated errno value is returned on
+ * any failure to indicate the nature of the failure.
+ *
+ /
+
+int fs_getcloseonexec(int fd)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct fdlist *list;
+ irqstate_t flags;
+ int ret;
+
+ list = nxsched_get_fdlist();
+
+ /* Perform some sanity checks */
+
+ if (fd < 0 || fd >= fdlist_count(list))
+{
+ return -EBADF;
+}
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ if (list->fl_fds[i][j].f_file == NULL)
+{
+ ret = -EBADF;
+}
+ else
+{
+ ret = list->fl_fds[i][j].f_cloexec ? FD_CLOEXEC : 0;
+}
+
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+ return ret;
+}
+
+/
+ * Name: nx_ioctl
+ *
+ * Description:
+ * nx_ioctl() will perform the operation specified by 'cmd' on an open
+ * file.
+ *
+ * Input Parameters:
+ * fd - File descriptor of the open file
+ * cmd - Identifies the operation to be performed. Command specific
+ * arguments may follow.
+ * arg - The argument.
+ *
+ * Returned Value:
+ * A non-negative value is returned on success; a negated errno value is
+ * returned on any failure.
+ *
+ /
+
+int nx_ioctl(int fd, int cmd, unsigned long arg)
Review Comment:
why move nx_ioctl here, not fs_ioctl.c? it's better to expose one api which
convert `fd` to `struct fd`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097578555 ## fs/vfs/fs_ioctl.c: ## @@ -39,6 +40,15 @@ * Private Functions / +/ + * Name: fd_vioctl + / + +static int fd_vioctl(int fd, int req, va_list ap) Review Comment: The problem with sim is that it inlines this static function which messes up the variadic argument list; instead of making a copy of ap for this function call, this function consumes the first argument... -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097578555 ## fs/vfs/fs_ioctl.c: ## @@ -39,6 +40,15 @@ * Private Functions / +/ + * Name: fd_vioctl + / + +static int fd_vioctl(int fd, int req, va_list ap) Review Comment: The problem with sim is that it inlines this static function which messes up the variadic argument list; instead of making a copy of ap for this function call, this function consumes the first argument... One way to fix this would be to make nx_vioctl, is that acceptable ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
hujun260 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097993216
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
@pussuw
We designed it by imitating the principle of Google's fdsan.
https://android.googlesource.com/platform/bionic/+/master/docs/fdsan.md
There are some fdsan test cases under this path.
apps/testing/fs/fdsantest
fdcheck, similar to fdsan, is also a tool used to detect whether the
ownership of file descriptors (FDs) is legal. fdcheck can automatically detect
whether the ownership of FDs is legal between processes and within a process.
This helps avoid incorrect FD passing that may lead to misuse, such as using an
FD after it has been freed.
You can also refer to the following test cases.
[fdchecktest.zip](https://github.com/user-attachments/files/20348585/fdchecktest.zip)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097578555 ## fs/vfs/fs_ioctl.c: ## @@ -39,6 +40,15 @@ * Private Functions / +/ + * Name: fd_vioctl + / + +static int fd_vioctl(int fd, int req, va_list ap) Review Comment: ~~The problem with sim is that it inlines this static function which messes up the variadic argument list; instead of making a copy of ap for this function call, this function consumes the first argument... One way to fix this would be to change nx_ioctl to nx_vioctl, is that acceptable ?~~ What I said is garbage. No matter what I do the variadic list is modified by the function (even though a copy is passed). A va_end/va_start is needed. You learn something new everyday.. ## fs/vfs/fs_ioctl.c: ## @@ -39,6 +40,15 @@ * Private Functions / +/ + * Name: fd_vioctl + / + +static int fd_vioctl(int fd, int req, va_list ap) Review Comment: ~~The problem with sim is that it inlines this static function which messes up the variadic argument list; instead of making a copy of ap for this function call, this function consumes the first argument...~~ ~~One way to fix this would be to change nx_ioctl to nx_vioctl, is that acceptable ?~~ What I said is garbage. No matter what I do the variadic list is modified by the function (even though a copy is passed). A va_end/va_start is needed. You learn something new everyday.. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097578555 ## fs/vfs/fs_ioctl.c: ## @@ -39,6 +40,15 @@ * Private Functions / +/ + * Name: fd_vioctl + / + +static int fd_vioctl(int fd, int req, va_list ap) Review Comment: The problem with sim is that it inlines this static function which messes up the variadic argument list; instead of making a copy of ap for this function call, this function consumes the first argument... One way to fix this would be to change nx_ioctl to nx_vioctl, is that acceptable ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097230197
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
@hujun260 could you provide a intro doc?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2097200315
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
I ended up removing the code, because I think it's just dead code.
I'm not sure if fdsan/fdcheck work as before with this new file descriptor
system, because I don't know what they're supposed to do and there is no
documentation or git / commit log history that would tell me. Can someone
explain shortly what they are ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095136393
##
fs/vfs/fs_open.c:
##
@@ -372,7 +372,7 @@ int file_open(FAR struct file *filep, FAR const char *path,
int oflags, ...)
if (ret >= OK)
{
- FS_ADD_BACKTRACE(filep);
+ FS_ADD_BACKTRACE(fdlist_fdget(nxsched_get_fdlist(), ret));
Review Comment:
file_allocate_from_tcb adds the backtrace in that branch.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2891403355 > > So, I can't understand why need move fdsan/fdcheck to fcntl > > No need to move, the funtion nx_fcntl is just named very badly. But I'm finally starting to undestand what you want me to do, you want me to implement the fd flag handling in nx_ioctl ? Is it OK if I move nx_ioctl into fs_files.c ? I'd still like to add the locking and hide the locks in there. Yes, this approach could reduce the change and keep the compatibility. All code which operate on the field in `struct fd` should be moved from file_xxx to nx_xxx. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095922616 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN Review Comment: ~~Ok so let's move nx_fcntl here and re-name it nx_ioctl ?~~ Forget about this comment. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2891372442 > So, I can't understand why need move fdsan/fdcheck to fcntl No need to move, the funtion nx_fcntl is just named very badly. But I'm finally starting to undestand what you want me to do, you want me to implement the fd flag handling in nx_ioctl ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095922616 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN Review Comment: Ok so let's move nx_fcntl here and re-name it nx_ioctl ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2891308759 > > > I tried to modify the fcntl/ioctl calls to separate fd and file requests. I don't think it is a good idea to modify/remove FIOC numbers. > > > > > > if so, why not keep implement in nx_ioctl and call nx_ioctl not nx_fcntl in the related places? > > I created nx_fcntl into fs_files for two reasons: > > * Atomically set / get file descriptor flags, like close on exec, fdsan / fdcheck > > * Keep the fl_lock semantics hidden there > > > We could re-name nx_fcntl to nx_modfdflags or something similar ? Would that be acceptable. I think nx_fcntl is now a very confusing name. `fcntl` could not only modify flags in `struct fd`, but also in `struct file`, so ioctl should do both too. So, I can't understand why need move fdsan/fdcheck to `fcntl`, but it's fine if you want to move the implement to fcntl, but please update FIOC_xxx to FL_xxx and stop forwarding them between `ioctl` and `fcntl` for compatibility, since they are introduced to support fdsan/fdcheck which is an internal implementation detail, not expect caller from userspace. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2891276973 > > I tried to modify the fcntl/ioctl calls to separate fd and file requests. I don't think it is a good idea to modify/remove FIOC numbers. > > if so, why not keep implement in nx_ioctl and call nx_ioctl not nx_fcntl in the related places? I created nx_fcntl into fs_files for two reasons: - Atomically set / get file descriptor flags, like close on exec, fdsan / fdcheck - Keep the fl_lock semantics hidden there We could re-name nx_fcntl to nx_modfdflags or something similar ? Would that be acceptable. I think nx_fcntl is now a very confusing name. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2891209631 > I tried to modify the fcntl/ioctl calls to separate fd and file requests. I don't think it is a good idea to modify/remove FIOC numbers. if so, why not keep implement in nx_ioctl and call nx_ioctl not nx_fcntl in the related places? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095821342
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
let's recheck the code
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095207463 ## fs/vfs/fs_ioctl.c: ## @@ -43,7 +43,7 @@ * Name: file_vioctl / -static int file_vioctl(FAR struct file *filep, int req, va_list ap) +static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd) Review Comment: ~~ I'll just move them all to fcntl ? Whichever way you prefer. ~~ I think this is a bad idea. ## fs/vfs/fs_ioctl.c: ## @@ -43,7 +43,7 @@ * Name: file_vioctl / -static int file_vioctl(FAR struct file *filep, int req, va_list ap) +static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd) Review Comment: ~~~ I'll just move them all to fcntl ? Whichever way you prefer. ~~~ I think this is a bad idea. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095591335 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN case FIOC_SETTAG_FDSAN: -filep->f_tag_fdsan = *(FAR uint64_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDSAN: -*(FAR uint64_t *)arg = filep->f_tag_fdsan; -ret = OK; -break; -#endif - -#ifdef CONFIG_FDCHECK case FIOC_SETTAG_FDCHECK: -filep->f_tag_fdcheck = *(FAR uint8_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDCHECK: -*(FAR uint8_t *)arg = filep->f_tag_fdcheck; -ret = OK; +ret = nx_fcntl(fd, req, arg); Review Comment: it's new ioctl and used by libc wrapper, so it's fine to rename it to macro aligned with fcntl -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095624051 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN Review Comment: I would like to concentrate handling file descriptor flags into 1 function, the current name is nx_fcntl but I think we should re-name it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095624051 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN Review Comment: I would like to concentrate handling file descriptor flags into 1 function, the current name for this is nx_fcntl but I think we should re-name it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095617002 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN Review Comment: but why not still implement in nx_ioctl and call nx_ioctl directly from the place which is required. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095597205 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN case FIOC_SETTAG_FDSAN: -filep->f_tag_fdsan = *(FAR uint64_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDSAN: -*(FAR uint64_t *)arg = filep->f_tag_fdsan; -ret = OK; -break; -#endif - -#ifdef CONFIG_FDCHECK case FIOC_SETTAG_FDCHECK: -filep->f_tag_fdcheck = *(FAR uint8_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDCHECK: -*(FAR uint8_t *)arg = filep->f_tag_fdcheck; -ret = OK; +ret = nx_fcntl(fd, req, arg); Review Comment: if so, why not call nx_ioctl, but nx_fcntl here? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095608918 ## include/nuttx/fs/ioctl.h: ## @@ -197,8 +197,6 @@ * configuration * OUT: None */ - -#ifdef CONFIG_FDSAN Review Comment: yes, either we implement in nx_ioctl or nx_fcntl, but don't need forward from nx_fcntl to nx_ioctl, or nx_ioctl to nx_fcntl. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095605256 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN case FIOC_SETTAG_FDSAN: -filep->f_tag_fdsan = *(FAR uint64_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDSAN: -*(FAR uint64_t *)arg = filep->f_tag_fdsan; -ret = OK; -break; -#endif - -#ifdef CONFIG_FDCHECK case FIOC_SETTAG_FDCHECK: -filep->f_tag_fdcheck = *(FAR uint8_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDCHECK: -*(FAR uint8_t *)arg = filep->f_tag_fdcheck; -ret = OK; +ret = nx_fcntl(fd, req, arg); Review Comment: Can you suggest a better name for nx_fcntl ? What that function is trying to do is "modify or read file descriptor flags atomically". The original name I thought of was fs_fdcntl. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095602534 ## fs/inode/fs_files.c: ## @@ -858,6 +1027,263 @@ int fs_putfilep(FAR struct file *filep) } #endif +/ + * Name: nx_fcntl + * + * Description: + * nx_fcntl() will perform the operation specified by 'cmd' on an open + * file. + * + * Input Parameters: + * fd - File descriptor of the open file + * cmd - Identifies the operation to be performed. Command specific + * arguments may follow. + * arg - The argument. + * + * Returned Value: + * A non-negative value is returned on success; a negated errno value is + * returned on any failure. + * + / + +int nx_fcntl(int fd, int cmd, unsigned long arg) Review Comment: I'm trying to hide fl_lock semantics inside fs_files.c. We could make getters/setters for all of the file descriptor flags (closeonexec, fdsan, fdcheck) but this would generate a lot of code. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095592349 ## fs/inode/fs_files.c: ## @@ -858,6 +1027,263 @@ int fs_putfilep(FAR struct file *filep) } #endif +/ + * Name: nx_fcntl + * + * Description: + * nx_fcntl() will perform the operation specified by 'cmd' on an open + * file. + * + * Input Parameters: + * fd - File descriptor of the open file + * cmd - Identifies the operation to be performed. Command specific + * arguments may follow. + * arg - The argument. + * + * Returned Value: + * A non-negative value is returned on success; a negated errno value is + * returned on any failure. + * + / + +int nx_fcntl(int fd, int cmd, unsigned long arg) Review Comment: should we keep it in fs_fcntl.c -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095383461 ## include/nuttx/fs/ioctl.h: ## @@ -197,8 +197,6 @@ * configuration * OUT: None */ - -#ifdef CONFIG_FDSAN Review Comment: I just realized there is a problem with forwarding FIOC_ requests to nx_fcntl; there can be collisions. I need to define new NX_FCNTL request numbers for safety. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095515021
##
fs/vfs/fs_dup.c:
##
@@ -65,29 +65,32 @@ int file_dup(FAR struct file *filep, int minfd, int flags)
uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */
#endif
- fd2 = file_allocate(g_root_inode, 0, 0, NULL, minfd, true);
+ /* Pass the close on exec flag to file_allocate */
+
+ fd2 = file_allocate(g_root_inode, flags, 0, NULL, minfd, true);
if (fd2 < 0)
{
return fd2;
}
ret = fs_getfilep(fd2, &filep2);
#ifdef CONFIG_FDSAN
- f_tag_fdsan = filep2->f_tag_fdsan;
+ f_tag_fdsan = nx_fcntl(fd2, NX_GETTAG_FDSAN, 0);
Review Comment:
@xiaoxiang781216 should I remove these calls for fdsan/fdcheck. I think they
are redundant.
- file_allocate initializes fdsan & fdcheck to 0 for fd2
- reading the value here for fd2 will return 0 every time
- writing back 0 after file_dup2 is redundant as file_dup2 does not modify
fdsan/fdcheck values
I left the old implementation as is, even though I think it could be
removed. What's your opinion ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095207463 ## fs/vfs/fs_ioctl.c: ## @@ -43,7 +43,7 @@ * Name: file_vioctl / -static int file_vioctl(FAR struct file *filep, int req, va_list ap) +static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd) Review Comment: ~~I'll just move them all to fcntl ? Whichever way you prefer.~~ Nevermind, that's not a good idea. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095207463 ## fs/vfs/fs_ioctl.c: ## @@ -43,7 +43,7 @@ * Name: file_vioctl / -static int file_vioctl(FAR struct file *filep, int req, va_list ap) +static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd) Review Comment: ~~I'll just move them all to fcntl ? Whichever way you prefer.~~ I think this is a bad idea. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2890406654 I tried to modify the fcntl/ioctl calls to separate fd and file requests. I don't think it is a good idea to modify/remove FIOC numbers. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095120372 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN case FIOC_SETTAG_FDSAN: -filep->f_tag_fdsan = *(FAR uint64_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDSAN: -*(FAR uint64_t *)arg = filep->f_tag_fdsan; -ret = OK; -break; -#endif - -#ifdef CONFIG_FDCHECK case FIOC_SETTAG_FDCHECK: -filep->f_tag_fdcheck = *(FAR uint8_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDCHECK: -*(FAR uint8_t *)arg = filep->f_tag_fdcheck; -ret = OK; +ret = nx_fcntl(fd, req, arg); Review Comment: We can do it, I did not want to move FIOC_XXX because it would become a regression for users of this ioctl. If it is fine to create this regression, I'll do the change ? Btw, are we allowed to create our own proprietary fcntl request numbers ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095207463 ## fs/vfs/fs_ioctl.c: ## @@ -43,7 +43,7 @@ * Name: file_vioctl / -static int file_vioctl(FAR struct file *filep, int req, va_list ap) +static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd) Review Comment: I'll just move them all to fcntl ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095207463 ## fs/vfs/fs_ioctl.c: ## @@ -43,7 +43,7 @@ * Name: file_vioctl / -static int file_vioctl(FAR struct file *filep, int req, va_list ap) +static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd) Review Comment: I'll just move them all to fcntl ? Whichever way you prefer. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095136393
##
fs/vfs/fs_open.c:
##
@@ -372,7 +372,7 @@ int file_open(FAR struct file *filep, FAR const char *path,
int oflags, ...)
if (ret >= OK)
{
- FS_ADD_BACKTRACE(filep);
+ FS_ADD_BACKTRACE(fdlist_fdget(nxsched_get_fdlist(), ret));
Review Comment:
file_allocate_from_tcb adds the backtrace in that branch.
I think this needs to be removed. I assumed ret == fd but it's just 0 (OK)
so this backtrace doesn't work.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095124199 ## include/nuttx/fs/ioctl.h: ## @@ -197,8 +197,6 @@ * configuration * OUT: None */ - -#ifdef CONFIG_FDSAN Review Comment: Answered above, in order to forward these to nx_fcntl they need to be defined always. nx_fcntl checks if the feature is enabled or not. I also think masking these is redundant. I can restore the ifdef if you think it's better. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095120372 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN case FIOC_SETTAG_FDSAN: -filep->f_tag_fdsan = *(FAR uint64_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDSAN: -*(FAR uint64_t *)arg = filep->f_tag_fdsan; -ret = OK; -break; -#endif - -#ifdef CONFIG_FDCHECK case FIOC_SETTAG_FDCHECK: -filep->f_tag_fdcheck = *(FAR uint8_t *)arg; -ret = OK; -break; - case FIOC_GETTAG_FDCHECK: -*(FAR uint8_t *)arg = filep->f_tag_fdcheck; -ret = OK; +ret = nx_fcntl(fd, req, arg); Review Comment: We can do it, I did not want to move FIOC_XXX because it would become a regression for users of this ioctl. If it is fine to create this regression, I'll do the change ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095118825 ## fs/vfs/fs_ioctl.c: ## @@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req, va_list ap) } break; -#ifdef CONFIG_FDSAN Review Comment: It makes it possible to forward all to nx_fcntl and let it check if fdsan/fdcheck are enabled or not. Makes it cleaner IMO. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2095112790 ## fs/inode/fs_files.c: ## @@ -858,6 +1027,263 @@ int fs_putfilep(FAR struct file *filep) } #endif +/ + * Name: nx_fcntl + * + * Description: + * nx_fcntl() will perform the operation specified by 'cmd' on an open + * file. + * + * Input Parameters: + * fd - File descriptor of the open file + * cmd - Identifies the operation to be performed. Command specific + * arguments may follow. + * arg - The argument. + * + * Returned Value: + * A non-negative value is returned on success; a negated errno value is + * returned on any failure. + * + / + +int nx_fcntl(int fd, int cmd, unsigned long arg) Review Comment: > where do we implement nx_fcntl @xiaoxiang781216 it's here. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2094179941
##
fs/vfs/fs_ioctl.c:
##
@@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req,
va_list ap)
}
break;
-#ifdef CONFIG_FDSAN
case FIOC_SETTAG_FDSAN:
-filep->f_tag_fdsan = *(FAR uint64_t *)arg;
-ret = OK;
-break;
-
case FIOC_GETTAG_FDSAN:
-*(FAR uint64_t *)arg = filep->f_tag_fdsan;
-ret = OK;
-break;
-#endif
-
-#ifdef CONFIG_FDCHECK
case FIOC_SETTAG_FDCHECK:
-filep->f_tag_fdcheck = *(FAR uint8_t *)arg;
-ret = OK;
-break;
-
case FIOC_GETTAG_FDCHECK:
-*(FAR uint8_t *)arg = filep->f_tag_fdcheck;
-ret = OK;
+ret = nx_fcntl(fd, req, arg);
Review Comment:
if all check want to move fcntl, we should move FIOC_x_xxCHECK to
fcntl.h and rename to F__xxCHECK, and don't need forward the call from
ioctl to fcntl.
##
fs/vfs/fs_ioctl.c:
##
@@ -135,29 +135,12 @@ static int file_vioctl(FAR struct file *filep, int req,
va_list ap)
}
break;
-#ifdef CONFIG_FDSAN
Review Comment:
why remove the check
##
include/nuttx/fs/ioctl.h:
##
@@ -197,8 +197,6 @@
* configuration
* OUT: None
*/
-
-#ifdef CONFIG_FDSAN
Review Comment:
why remove the check
##
fs/procfs/fs_procfsproc.c:
##
@@ -1325,9 +1325,10 @@ static ssize_t proc_groupfd(FAR struct proc_file_s
*procfile,
if (linesize < STATUS_LINELEN)
{
#if CONFIG_FS_BACKTRACE > 0
+ struct fd *fd = fdlist_fdget(&group->tg_fdlist, i);
Review Comment:
add FAR
##
include/nuttx/fs/ioctl.h:
##
@@ -208,9 +206,6 @@
* Pointer to file tag
* OUT: None
*/
-#endif
-
-#ifdef CONFIG_FDCHECK
Review Comment:
ditto
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2094174857 ## fs/vfs/fs_fcntl.c: ## @@ -46,7 +46,7 @@ * Name: file_vfcntl / -static int file_vfcntl(FAR struct file *filep, int cmd, va_list ap) +static int file_vfcntl(FAR struct file *filep, int cmd, va_list ap, int fd) Review Comment: remove fd, we should move the handle of F_DUPFD/F_DUPFD_CLOEXEC/F_GETFD/F_SETFD to fcntl/nx_fcntl and call dup/nx_dup instead -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2094177845 ## include/nuttx/fs/fs.h: ## @@ -466,6 +466,15 @@ struct file off_t f_pos; /* File position */ Review Comment: The memory leak will always happen without refcount. so, it isn't optional with the new implementation -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2094175112
##
fs/vfs/fs_fcntl.c:
##
@@ -91,7 +91,7 @@ static int file_vfcntl(FAR struct file *filep, int cmd,
va_list ap)
*/
{
- ret = filep->f_oflags & O_CLOEXEC ? FD_CLOEXEC : 0;
+ ret = nx_fcntl(fd, F_GETFD, 0);
Review Comment:
where do we implement nx_fcntl
##
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_LOCK_BUCKET_SIZE > 0
+ bool locked; /* Filelock state: false - unlocked, true -
locked */
+#endif
+};
+
+struct fd
+{
+ FAR struct file *f_file; /* The file associated with descriptor */
+ bool f_cloexec; /* Close on exec */
#ifdef CONFIG_FDSAN
Review Comment:
move after line 484 to reduce the whole between fields
##
fs/vfs/fs_fcntl.c:
##
@@ -46,7 +46,7 @@
* Name: file_vfcntl
/
-static int file_vfcntl(FAR struct file *filep, int cmd, va_list ap)
+static int file_vfcntl(FAR struct file *filep, int cmd, va_list ap, int fd)
Review Comment:
remove fd, we should move the handle of
F_DUPFD/F_DUPFD_CLOEXEC/F_GETFD/F_SETFD to fcntl and call dup instead
##
fs/vfs/fs_ioctl.c:
##
@@ -43,7 +43,7 @@
* Name: file_vioctl
/
-static int file_vioctl(FAR struct file *filep, int req, va_list ap)
+static int file_vioctl(FAR struct file *filep, int req, va_list ap, int fd)
Review Comment:
remove fd, we should move
FIOCLEX/FIONCLEX/FIOC_SETTAG_FDSAN/FIOC_GETTAG_FDSAN/FIOC_SETTAG_FDCHECK/FIOC_GETTAG_FDCHECK
to ioctl
##
fs/vfs/fs_open.c:
##
@@ -372,7 +372,7 @@ int file_open(FAR struct file *filep, FAR const char *path,
int oflags, ...)
if (ret >= OK)
{
- FS_ADD_BACKTRACE(filep);
+ FS_ADD_BACKTRACE(fdlist_fdget(nxsched_get_fdlist(), ret));
Review Comment:
need move to nx_vopen
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086976819 ## fs/vfs/fs_dup2.c: ## @@ -82,29 +81,15 @@ int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags) inode = filep1->f_inode; inode_addref(inode); - /* If there is already an inode contained in the new file structure, Review Comment: Can you please explain the use case how / where inode is realloced ? Is this solution "free without clear" still needed ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086970591 ## net/local/local_sendmsg.c: ## @@ -125,7 +125,7 @@ static int local_sendctl(FAR struct local_conn_s *conn, goto fail; } - ret = file_dup2(filep, filep2); + ret = file_dup2(filep, filep2); /* TODO: What about this ? */ Review Comment: That's the question I have, does file_dup have any meaning inside the kernel ? Why do kernel drivers care about file descriptors, they are userspace stuff anyhow. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
GUIDINGLI commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086926058 ## fs/vfs/fs_dup2.c: ## @@ -82,29 +81,15 @@ int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags) inode = filep1->f_inode; inode_addref(inode); - /* If there is already an inode contained in the new file structure, Review Comment: how to handle this situation, realloced by others ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086894004 ## fs/vfs/fs_close.c: ## @@ -134,46 +143,84 @@ int file_close_without_clear(FAR struct file *filep) inode_release(inode); Review Comment: @Donny9 could you look at this 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086872746 ## include/nuttx/fs/fs.h: ## @@ -466,6 +466,15 @@ struct file off_t f_pos; /* File position */ Review Comment: refcount is required to implement the POSIX semantic. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086870916 ## include/nuttx/fs/fs.h: ## @@ -959,7 +964,7 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist, * / -FAR struct file *files_fget(FAR struct filelist *list, int fd); +FAR struct file *files_fget(FAR struct fdlist *list, int fd); Review Comment: ok -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086869529
##
net/local/local_recvmsg.c:
##
@@ -175,7 +175,7 @@ static void local_recvctl(FAR struct local_conn_s *conn,
count = peer->lc_cfpcount;
for (i = 0; i < count; i++)
{
- fds[i] = file_dup(peer->lc_cfps[i], 0,
+ fds[i] = file_dup(peer->lc_cfps[i], 0, /* TODO: WHat about this ? */
Review Comment:
maybe, we should remove file_dupx
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
xiaoxiang781216 commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086867781 ## net/local/local_sendmsg.c: ## @@ -125,7 +125,7 @@ static int local_sendctl(FAR struct local_conn_s *conn, goto fail; } - ret = file_dup2(filep, filep2); + ret = file_dup2(filep, filep2); /* TODO: What about this ? */ Review Comment: with the new implementation, I guess that we should remove file_dupx, do you have any real caller inside kernel space? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086328589 ## include/nuttx/fs/fs.h: ## @@ -466,6 +466,15 @@ struct file off_t f_pos; /* File position */ Review Comment: @anchao what is your opinion on this ? Should I leave the option and prevent file closing ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086067813
##
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:
No, the old implementation was simply wrong.
The dup3() function shall be equivalent to the dup2() function, except that
it shall be an error if fildes is equal to fildes2. You need to return -EINVAL
if fd1 == fd2. This is fixed here.
##
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:
No, the old implementation was simply wrong.
`
The dup3() function shall be equivalent to the dup2() function, except that
it shall be an error if fildes is equal to fildes2. You need to return -EINVAL
if fd1 == fd2. This is fixed here.`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on PR #16361: URL: https://github.com/apache/nuttx/pull/16361#issuecomment-2875412623 ``` nsh> fdinfo pid:0 FD OFLAGS TYPE POS PATH BACKTRACE 0 3 10 /dev/console 0x40053878 0x400356e4 0x40038072 0x40038390 0x400053a2 0x40002ce6 0x40002613 0x78a2faa2a1c 1 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053ba 0x40002ce6 0x4000261 2 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053c9 0x40002ce6 0x4000261 pid:1 FD OFLAGS TYPE POS PATH BACKTRACE 0 3 10 /dev/console 0x40053878 0x400356e4 0x40038072 0x40038390 0x400053a2 0x40002ce6 0x40002613 0x78a2faa2a1c 1 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053ba 0x40002ce6 0x4000261 2 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053c9 0x40002ce6 0x4000261 pid:2 FD OFLAGS TYPE POS PATH BACKTRACE 0 3 10 /dev/console 0x40053878 0x400356e4 0x40038072 0x40038390 0x400053a2 0x40002ce6 0x40002613 0x78a2faa2a1c 1 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053ba 0x40002ce6 0x4000261 2 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053c9 0x40002ce6 0x4000261 pid:3 FD OFLAGS TYPE POS PATH BACKTRACE 0 3 10 /dev/console 0x40053878 0x400356e4 0x40038072 0x40038390 0x400053a2 0x40002ce6 0x40002613 0x78a2faa2a1c 1 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053ba 0x40002ce6 0x4000261 2 3 10 /dev/console 0x40053878 0x40036b20 0x40035240 0x40035abf 0x40035aea 0x400053c9 0x40002ce6 0x4000261 pid:4 FD OFLAGS TYPE POS PATH BACKTRACE 0 3 10 /dev/console 0x40053878 0x40036b20 0x40036b76 0x400358e7 0x400054d2 0x40006dc8 0x40003633 0x4000378 1 3 10 /dev/console 0x40053878 0x40036b20 0x40036b76 0x400358e7 0x400054d2 0x40006dc8 0x40003633 0x4000378 2 3 10 /dev/console 0x40053878 0x40036b20 0x40036b76 0x400358e7 0x400054d2 0x40006dc8 0x40003633 0x4000378 3 307305 /proc//0x40053878 0x400356e4 0x40038072 0x40038478 0x4005ad3d 0x400291aa 0x40028b11 0x40022ec 4 1025300x40053878 0x400356e4 0x40038072 0x40038478 0x40028d61 0x40026394 0x4002925e 0x40028b1 ``` Backtrace should now be working. At least it's doing something. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086063889
##
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:
Ditto
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086069196
##
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:
Oops, nice catch !
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086067813
##
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:
No, the old implementation was simply wrong.
`
The dup3() function shall be equivalent to the dup2() function, except that
it shall be an error if fildes is equal to fildes2. You need to return -EINVAL
if fd1 == fd2. `
This is fixed here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086066759
##
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:
No, this is correct, we must handle fd1==fd2 differently for dup2 and dup3
dup2(): `unless fildes is equal to fildes2 in which case dup2() shall return
fildes2 without closing it. `
dup3(): `The dup3() function shall be equivalent to the dup2() function,
except that it shall be an error if fildes is equal to fildes2,`
https://pubs.opengroup.org/onlinepubs/9799919799/functions/dup.html
##
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:
No, this is correct, we must handle fd1==fd2 differently for dup2 and dup3
dup2(): `unless fildes is equal to fildes2 in which case dup2() shall return
fildes2 without closing it. `
dup3(): `The dup3() function shall be equivalent to the dup2() function,
except that it shall be an error if fildes is equal to fildes2,`
https://pubs.opengroup.org/onlinepubs/9799919799/functions/dup.html
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086063197
##
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 is not a valid error number from open/dup/dup2/dup3
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086061232 ## 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 is fine, and I don't want to copy Linux code. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086061787
##
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:
Sure, but not in the scope of this patch. It is already big enough.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
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_tfl_lock;/* Manage access to the file list */
+ spinlock_tfl_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
Re: [PR] fs/vfs: Separate file descriptors from file descriptions [nuttx]
pussuw commented on code in PR #16361: URL: https://github.com/apache/nuttx/pull/16361#discussion_r2085174457 ## include/nuttx/fs/fs.h: ## @@ -466,6 +466,15 @@ struct file off_t f_pos; /* File position */ Review Comment: Yes I forgot to remove the option, I'll remove it tomorrow. One option of course if we want to keep CONFIG_FS_REFCOUNT we must prevent file_close altogether, because it will never be safe to call file_close. The simplest case is stdout/stderr/stdin, now the file is shared by ALL children of init, and no duplicate file structs are created. So when a process exits, it is NOT SAFE to free stdout/stderr/stdin for that process if there is no reference counter protecting them. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
