You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "michallenc (via GitHub)" <gi...@apache.org> on 2024/09/23 20:08:03 UTC
[PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
michallenc opened a new pull request, #13584:
URL: https://github.com/apache/nuttx/pull/13584
## Summary
The `FIOC_FILEPATH` ioctl call is required if smartfs is to be used together with inotify monitoring system. This partially implements the call support to smartfs file system.
The call currently does not work correctly if subdirectories are in the file's path. For example `/dir/file` will be returned correctly, but `/dir/subdir/file` will exclude the `subdir` part and return only `/dir/file`. The full support is complicated due to the SmartFS design.
The correct procedure would be to take the inode's path obtained by inode_getpath and let SmartFS recursively get the full path from the file. However, SmartFS entries do not keep the information about their parents and it is not possible to obtain the full path by going backwards. The file system currently takes the advantage of knowing the full path during the node creation/remove/rename/etc and does the forward search with node by node string comparison.
This possible solution would require the introduction of parent field in the entry structure and going backwards to obtain the full path. This would mean the modification of SmartFS structures including the ones stored in the NOR flash, most likely causing the incompatibility with previous versions.
## Impact
SmartFS now at least partially work with inotify for calls that do not have direct knowledge of the full file path.
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "michallenc (via GitHub)" <gi...@apache.org>.
michallenc commented on code in PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#discussion_r1776752966
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -217,6 +219,21 @@ static int smartfs_open(FAR struct file *filep, FAR const char *relpath,
goto errout_with_lock;
}
+ /* Save the full path to smartfs_ofile_s stuctrue. This is needed
+ * to FIOC_FILEPATH ioctlc all support.
+ */
+
+ pathlen = strlen(relpath) + 1;
+ sf->path = kmm_malloc(pathlen);
Review Comment:
Both 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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#discussion_r1777270663
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -210,13 +212,20 @@ static int smartfs_open(FAR struct file *filep, FAR const char *relpath,
/* Locate the directory entry for this path */
- sf = kmm_malloc(sizeof *sf);
+ pathlen = strlen(relpath) + 1;
+ sf = kmm_malloc(sizeof(*sf) + pathlen);
Review Comment:
```suggestion
sf = kmm_malloc(sizeof(*sf) + pathlen - 1);
```
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -210,13 +212,20 @@ static int smartfs_open(FAR struct file *filep, FAR const char *relpath,
/* Locate the directory entry for this path */
- sf = kmm_malloc(sizeof *sf);
+ pathlen = strlen(relpath) + 1;
+ sf = kmm_malloc(sizeof(*sf) + pathlen);
if (sf == NULL)
{
ret = -ENOMEM;
goto errout_with_lock;
}
+ /* Save the full path to smartfs_ofile_s stuctrue. This is needed
+ * to FIOC_FILEPATH ioctlc all support.
+ */
+
+ strlcpy(sf->path, relpath, pathlen);
Review Comment:
memcpy
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -997,9 +1006,45 @@ static off_t smartfs_seek(FAR struct file *filep, off_t offset, int whence)
static int smartfs_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
{
- /* We don't use any ioctls */
+ FAR struct smartfs_ofile_s *priv;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Recover our private data from the struct file instance */
+
+ priv = filep->f_priv;
+ inode = filep->f_inode;
+ ret = nxmutex_lock(&g_lock);
Review Comment:
don't need lock
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "michallenc (via GitHub)" <gi...@apache.org>.
michallenc commented on PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#issuecomment-2369283341
I am not 100 % forcing this to be merged. It provides SmartFS with least partial support for inotify but does not handle correctly subdirectories and there is no way how to detect it at runtime and return error if subdirectory is present. On the other hand, there are both compile time and runtime warning describing this issue, so the user gets the memo.
I would generally prefer to merge this, but I understand if it is rejected. I currently don't have time to dive into SmartFS properly and rewrite it to provide the full support, but maybe it will inspire someone to finish 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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#issuecomment-2369899875
one simple method is that smartfs_open cache the path into smartfs_ofile_s.
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#issuecomment-2372862381
> > one simple method is that smartfs_open cache the path into smartfs_ofile_s.
>
> Yes, but can we afford it for smaller microcontrollers with less memory?
But if smartfs can't restore the full path from the leaf node like littlefs, the full path has to be record in open callback.
> The maximum path length is 4 KBs if I remember correctly, although yes, the entire length won't be used in 99 % cases. But I suppose that's why most of the file systems do not store the entire path and iterate backwards to get it.
But in most case, the path is less than 128bytes.
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "michallenc (via GitHub)" <gi...@apache.org>.
michallenc commented on code in PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#discussion_r1777348693
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -210,13 +212,20 @@ static int smartfs_open(FAR struct file *filep, FAR const char *relpath,
/* Locate the directory entry for this path */
- sf = kmm_malloc(sizeof *sf);
+ pathlen = strlen(relpath) + 1;
+ sf = kmm_malloc(sizeof(*sf) + pathlen);
Review Comment:
Substracted
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "michallenc (via GitHub)" <gi...@apache.org>.
michallenc commented on PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#issuecomment-2373289704
Ok, then we can simplify this quite a lot.
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "michallenc (via GitHub)" <gi...@apache.org>.
michallenc commented on code in PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#discussion_r1777348111
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -210,13 +212,20 @@ static int smartfs_open(FAR struct file *filep, FAR const char *relpath,
/* Locate the directory entry for this path */
- sf = kmm_malloc(sizeof *sf);
+ pathlen = strlen(relpath) + 1;
+ sf = kmm_malloc(sizeof(*sf) + pathlen);
if (sf == NULL)
{
ret = -ENOMEM;
goto errout_with_lock;
}
+ /* Save the full path to smartfs_ofile_s stuctrue. This is needed
+ * to FIOC_FILEPATH ioctlc all support.
+ */
+
+ strlcpy(sf->path, relpath, pathlen);
Review Comment:
Done
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -997,9 +1006,45 @@ static off_t smartfs_seek(FAR struct file *filep, off_t offset, int whence)
static int smartfs_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
{
- /* We don't use any ioctls */
+ FAR struct smartfs_ofile_s *priv;
+ FAR struct inode *inode;
+ int ret;
+
+ /* Recover our private data from the struct file instance */
+
+ priv = filep->f_priv;
+ inode = filep->f_inode;
+ ret = nxmutex_lock(&g_lock);
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "michallenc (via GitHub)" <gi...@apache.org>.
michallenc commented on PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#issuecomment-2370303032
> one simple method is that smartfs_open cache the path into smartfs_ofile_s.
Yes, but can we afford it for smaller microcontrollers with less memory? The maximum path length is 4 KBs if I remember correctly, although yes, the entire length won't be used in 99 % cases. But I suppose that's why most of the file systems do not store the entire path and iterate backwards to get 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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #13584:
URL: https://github.com/apache/nuttx/pull/13584
--
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
Re: [PR] smartfs: add partial support for FIOC_FILEPATH ioctl [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #13584:
URL: https://github.com/apache/nuttx/pull/13584#discussion_r1776161043
##########
fs/smartfs/smartfs.h:
##########
@@ -294,6 +294,7 @@ struct smartfs_ofile_s
FAR uint8_t *buffer; /* Sector buffer to reduce writes */
uint8_t bflags; /* Buffer flags */
#endif
+ FAR char *path; /* The full path to the file */
Review Comment:
```suggestion
char path[1]; /* The full path to the file */
```
and move to the end of struct
##########
fs/smartfs/smartfs_smart.c:
##########
@@ -217,6 +219,21 @@ static int smartfs_open(FAR struct file *filep, FAR const char *relpath,
goto errout_with_lock;
}
+ /* Save the full path to smartfs_ofile_s stuctrue. This is needed
+ * to FIOC_FILEPATH ioctlc all support.
+ */
+
+ pathlen = strlen(relpath) + 1;
+ sf->path = kmm_malloc(pathlen);
Review Comment:
let's change line 215 to:
```
sf = kmm_malloc(sizeof(*sf) + len);
```
and remove the second allocation here to simply the error handling
--
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