You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/10/26 10:20:22 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #7434: Refine the implementation of files_allocate
xiaoxiang781216 opened a new pull request, #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434
## Summary
- fs/vfs: Rename files_allocate to file_allocate
- fs/vfs: Move file_allocate from fs/inode/inode.h to include/nuttx/fs/fs.h
- fs/vfs: Let caller control whether add the reference count of inode in file_allocate
## Impact
The caller of files_allocate
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1006140831
##########
include/nuttx/fs/fs.h:
##########
@@ -775,6 +775,22 @@ void files_releaselist(FAR struct filelist *list);
int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist);
+/****************************************************************************
+ * Name: file_allocate
+ *
+ * Description:
+ * Allocate a struct files instance and associate it with an inode
+ * instance. Returns the file descriptor == index into the files array.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; a negated errno value is returned on
+ * any failure.
Review Comment:
```suggestion
* Description:
* Allocate a struct files instance and associate it with an inode
* instance.
*
* Returned Value:
* Returns the file descriptor == index into the files array on success;
* a negated errno value is returned on any failure.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1007366350
##########
fs/inode/fs_files.c:
##########
@@ -225,17 +235,24 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
/* The space of file array isn't enough, allocate a new filechunk */
ret = files_extend(list, i + 1);
- if (ret >= 0)
+ if (ret < 0)
Review Comment:
Why not keep `if (ret >= 0)` and add
```
if (addref)
{
inode_addref(inode);
}
```
inside?
I do not mind with current code, but we can save one `nxmutex_unlock(&list->fl_lock);` call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1007557617
##########
fs/inode/fs_files.c:
##########
@@ -225,17 +235,24 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
/* The space of file array isn't enough, allocate a new filechunk */
ret = files_extend(list, i + 1);
- if (ret >= 0)
+ if (ret < 0)
Review Comment:
I want to move inode_addref out of lock to improve the concurrency a little bit:)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1005720841
##########
include/nuttx/fs/fs.h:
##########
@@ -775,6 +775,18 @@ void files_releaselist(FAR struct filelist *list);
int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist);
+/****************************************************************************
+ * Name: file_allocate
+ *
+ * Description:
+ * Allocate a struct files instance and associate it with an inode
+ * instance. Returns the file descriptor == index into the files array.
+ *
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1006164525
##########
include/nuttx/fs/fs.h:
##########
@@ -775,6 +775,22 @@ void files_releaselist(FAR struct filelist *list);
int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist);
+/****************************************************************************
+ * Name: file_allocate
+ *
+ * Description:
+ * Allocate a struct files instance and associate it with an inode
+ * instance. Returns the file descriptor == index into the files array.
+ *
+ * Returned Value:
+ * Zero (OK) is returned on success; a negated errno value is returned on
+ * any failure.
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1005513657
##########
include/nuttx/fs/fs.h:
##########
@@ -775,6 +775,18 @@ void files_releaselist(FAR struct filelist *list);
int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist);
+/****************************************************************************
+ * Name: file_allocate
+ *
+ * Description:
+ * Allocate a struct files instance and associate it with an inode
+ * instance. Returns the file descriptor == index into the files array.
+ *
Review Comment:
Please add `Returned Value:` section
##########
fs/inode/fs_files.c:
##########
@@ -225,17 +231,23 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
/* The space of file array isn't enough, allocate a new filechunk */
ret = files_extend(list, i + 1);
- if (ret >= 0)
+ nxmutex_unlock(&list->fl_lock);
+ if (ret < 0)
{
- list->fl_files[i][0].f_oflags = oflags;
- list->fl_files[i][0].f_pos = pos;
- list->fl_files[i][0].f_inode = inode;
- list->fl_files[i][0].f_priv = priv;
- ret = i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ return ret;
}
- nxmutex_unlock(&list->fl_lock);
- return ret;
+ list->fl_files[i][0].f_oflags = oflags;
+ list->fl_files[i][0].f_pos = pos;
+ list->fl_files[i][0].f_inode = inode;
+ list->fl_files[i][0].f_priv = priv;
Review Comment:
shouldn't list access be protected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7434: Refine the implementation of files_allocate
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7434:
URL: https://github.com/apache/incubator-nuttx/pull/7434#discussion_r1005715751
##########
fs/inode/fs_files.c:
##########
@@ -225,17 +231,23 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
/* The space of file array isn't enough, allocate a new filechunk */
ret = files_extend(list, i + 1);
- if (ret >= 0)
+ nxmutex_unlock(&list->fl_lock);
+ if (ret < 0)
{
- list->fl_files[i][0].f_oflags = oflags;
- list->fl_files[i][0].f_pos = pos;
- list->fl_files[i][0].f_inode = inode;
- list->fl_files[i][0].f_priv = priv;
- ret = i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ return ret;
}
- nxmutex_unlock(&list->fl_lock);
- return ret;
+ list->fl_files[i][0].f_oflags = oflags;
+ list->fl_files[i][0].f_pos = pos;
+ list->fl_files[i][0].f_inode = inode;
+ list->fl_files[i][0].f_priv = priv;
Review Comment:
My fault, fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org