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 2020/09/22 08:43:25 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1864: fs: Add the related path support
xiaoxiang781216 opened a new pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864
## Summary
all functions which accept the path argument should support the related path:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
## Impact
All function accepted path argument will support the related path now.
## Testing
LTP
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493622678
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -430,6 +430,29 @@ static int _inode_search(FAR struct inode_search_s *desc)
return ret;
}
+/****************************************************************************
+ * Name: _inode_getcwd
+ *
+ * Description:
+ * Return the current working directory
+ *
+ ****************************************************************************/
+
+static FAR const char *_inode_getcwd(void)
+{
+ FAR const char *pwd = "";
+
+#ifndef CONFIG_DISABLE_ENVIRON
+ pwd = getenv("PWD");
+ if (pwd == NULL)
+ {
+ pwd = CONFIG_LIB_HOMEDIR;
+ }
+#endif
+
+ return pwd;
+}
+
Review comment:
If both PWD and CONFIG_LIB_HOMEDIR doesn't exist or are empty, the follow code:
```
asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
```
will add the / to the relative path which mean the root is the current directory in this case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493599486
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -467,7 +490,20 @@ int inode_search(FAR struct inode_search_s *desc)
* node if node is a symbolic link.
*/
- DEBUGASSERT(desc != NULL);
+ DEBUGASSERT(desc != NULL && desc->path != NULL);
+
+ /* Convert the relative path to the absolute path */
+
+ if (*desc->path != '/')
+ {
+ asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
+ if (desc->buffer == NULL)
Review comment:
Is there a memory leak here? Where is desc->buffer freed?
Since it is not a new field, it is probably freed somewhere. Can you verify that it is freed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 merged pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 merged pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-698247491
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493651890
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -430,6 +430,29 @@ static int _inode_search(FAR struct inode_search_s *desc)
return ret;
}
+/****************************************************************************
+ * Name: _inode_getcwd
+ *
+ * Description:
+ * Return the current working directory
+ *
+ ****************************************************************************/
+
+static FAR const char *_inode_getcwd(void)
+{
+ FAR const char *pwd = "";
+
+#ifndef CONFIG_DISABLE_ENVIRON
+ pwd = getenv("PWD");
+ if (pwd == NULL)
+ {
+ pwd = CONFIG_LIB_HOMEDIR;
+ }
+#endif
+
+ return pwd;
+}
+
Review comment:
> getenv() is prototyped in stdlib.h which is not included by this file. It may be being included through some sneak patch, but it is safer if stdlib.h were included explicitly.
Include stdlib.h now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1864: fs: Add the related path support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-696597933
> @xiaoxiang781216 related ->relative ?
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-697250471
Greg would you please have a look at 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493598244
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -430,6 +430,29 @@ static int _inode_search(FAR struct inode_search_s *desc)
return ret;
}
+/****************************************************************************
+ * Name: _inode_getcwd
+ *
+ * Description:
+ * Return the current working directory
+ *
+ ****************************************************************************/
+
+static FAR const char *_inode_getcwd(void)
+{
+ FAR const char *pwd = "";
+
+#ifndef CONFIG_DISABLE_ENVIRON
+ pwd = getenv("PWD");
+ if (pwd == NULL)
+ {
+ pwd = CONFIG_LIB_HOMEDIR;
+ }
+#endif
+
+ return pwd;
+}
+
Review comment:
This logic will not work if PWD is not defined on the thread. But I can find only one case where this will be an issue:
fs/mount/fs_automount.c: ret = inode_search(&desc);
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493590922
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -430,6 +430,29 @@ static int _inode_search(FAR struct inode_search_s *desc)
return ret;
}
+/****************************************************************************
+ * Name: _inode_getcwd
+ *
+ * Description:
+ * Return the current working directory
+ *
+ ****************************************************************************/
+
+static FAR const char *_inode_getcwd(void)
+{
+ FAR const char *pwd = "";
+
+#ifndef CONFIG_DISABLE_ENVIRON
+ pwd = getenv("PWD");
+ if (pwd == NULL)
+ {
+ pwd = CONFIG_LIB_HOMEDIR;
+ }
+#endif
+
+ return pwd;
+}
+
Review comment:
If feels awkward to use CONFIG_LIB_HOMEDIR since that is a setting for the C library. I am not recommending any change, but it does not feel right 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-698247491
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-697250471
Greg would you pleas have a look at 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493599486
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -467,7 +490,20 @@ int inode_search(FAR struct inode_search_s *desc)
* node if node is a symbolic link.
*/
- DEBUGASSERT(desc != NULL);
+ DEBUGASSERT(desc != NULL && desc->path != NULL);
+
+ /* Convert the relative path to the absolute path */
+
+ if (*desc->path != '/')
+ {
+ asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
+ if (desc->buffer == NULL)
Review comment:
Is there a memory leak here? Where is desc->buffer freed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-697553380
@davids5 I have completed the review that you requested. Please merge if you are satisfied.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1864: fs: Add the related path support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-696590634
@xiaoxiang781216 related ->relative ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-698074478
@davids5 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1864: fs: Add the related path support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-696590634
@xiaoxiang781216 related ->relative ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493589778
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -430,6 +430,29 @@ static int _inode_search(FAR struct inode_search_s *desc)
return ret;
}
+/****************************************************************************
+ * Name: _inode_getcwd
+ *
+ * Description:
+ * Return the current working directory
+ *
+ ****************************************************************************/
+
+static FAR const char *_inode_getcwd(void)
+{
+ FAR const char *pwd = "";
+
+#ifndef CONFIG_DISABLE_ENVIRON
+ pwd = getenv("PWD");
+ if (pwd == NULL)
+ {
+ pwd = CONFIG_LIB_HOMEDIR;
+ }
+#endif
+
+ return pwd;
+}
+
Review comment:
getenv() is prototyped in stdlib.h which is not included by this file. It may be being included through some sneak patch, but it is safer if stdlib.h were included explicitly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#issuecomment-697937577
@patacongo - Thank you!
@xiaoxiang781216 Are you done? Use the
button when you have all the review item 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1864: fs: Add the relative path support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1864:
URL: https://github.com/apache/incubator-nuttx/pull/1864#discussion_r493618966
##########
File path: fs/inode/fs_inodesearch.c
##########
@@ -467,7 +490,20 @@ int inode_search(FAR struct inode_search_s *desc)
* node if node is a symbolic link.
*/
- DEBUGASSERT(desc != NULL);
+ DEBUGASSERT(desc != NULL && desc->path != NULL);
+
+ /* Convert the relative path to the absolute path */
+
+ if (*desc->path != '/')
+ {
+ asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
+ if (desc->buffer == NULL)
Review comment:
Yes, it's release in RELEASE_SEARCH. I reuse the same code which release the soft link buffer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org