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/04/17 10:55:44 UTC
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 opened a new pull request
#185: nsh improvement
xiaoxiang781216 opened a new pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185
The key fix is: sh command shouldn't change the current shell environment variables
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615500390
I think this change is a good idea, but the implementation is insufficient. It should not be merged in its current form. A proper solution will require a more substantial effort.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615922164
> > Ok, I will rename sh to source and provide a new sh implemenation like nsh.
>
> The alias '.' would be nice to. Like the current test has '[' as its alias.
>
Yes, I will add '.' alias.
> I am not sure what you mean by "a new sh implemenation like nsh."
Add a new program named sh call nsh_system.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in
pull request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410689040
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
Yes, actually the old 'sh' command should name to 'source'.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410503327
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
The OpenGroup.org requirement is not so clear: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html but I think we all know how it is supposed to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410499975
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
The child task will inherit the address environment of the parent. But any changes made to the environment by the child will not nore change the parent's environment.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on issue #185:
nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-616141810
The change is more complex than I expected and appears to include additional, unexplained changes. For example, why is boardctl(BOARDIOC_FINALINIT) removed?
Nevermind... I see that it was moved. I will merge when after I review a little more.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615844029
> There is a logic flaw in hsow system() is called. The system() call will block until the cmd is executed. Any & or redirection on the command will not be handled. Too much breakage! That is not acceptable and must be fixed.
>
But the old implementation call nsh_script directly, which also block until the script finish execution. If the old behaviour support & or redirection, then the new should support too, because the only difference is that the new one has a longer call chain:
new: cmd_sh->system->posix_task->nsh_system->nsh_parse->nsh_source->nsh_script
old: cmd_sh->nsh_script
The real action happen inside nsh_script, so the key is that nsh_script need to enhance to support & and >.
> Hmmm... Or does the system command run on a pthread via cmd_sh(). In that case, then the & background option would work (although it ties up both a pthread and a task).
>
Should we put the logic into nsh_script? since we have support:
sh /path/to/script &
source /path/to/script &
> But the redirection as it is implemented for cmd_sh() will not work for system(cmd). Within NSH, redirection is handled via nsh_output(), stdin/stdout/stderr are not changed.
>
Yes, if the redirection just change the internal parser state not stdin/stdtout/stderr, the new nsh task can't inherit the redirection setting.
> So even though you redirect the output at cmd_sh, will not be redirected by system(). This is not good. This is a good idea but not ready to come into the master.
How about we modify stdin/stdout/stderr directly?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo merged pull request #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615875439
> @patacongo - Do you see any harm in renaming sh to `source` and `.`
No, that seems appropriate to me. In that case, if 'sh' were implemented as a builtin task, it would just execute the nsh command 'source' or '.' in the context of the spawned task. Then it becomes sh since it has no effect on the parent task.
>
> I would also add the concern of stack nesting and fragmentation to the list.
>
> Please contrast the existing implementation to the proposed one in terms of stack nesting and fragmentation when the /etc/rind.d/rcS has
>
> `sh ./l1` and l1 has `sh .l2`
>
> From a small platform with the goal of having a clean set of scripts, that really just needs `sh` to include the as if it were one file.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410501604
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
You would also have to have SYSTEM command enabled.
NOTE: This effects only the SH command. The start-up script goes through a different path: nsh_main->nsh_consolemain->nsh_initscript->nsh_script
So the the init script always executes on the thread of execution of nsh_main().
I believe that this change is consistent with the POSIX requirement and it must be included.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615523020
After thinking more, I think that make 'sh' a built-in function might solve all of the issues that I raised. The & backgrounding would working correctly without tying up a pthread; and re-redirection of I/O would work too.
This is really quite a simple and usable alternative design. Please consider 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-616141810
The change is more complex than I expected and appears to include additional, unexplained changes. For example, why is boardctl(BOARDIOC_FINALINIT) removed?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410510054
##########
File path: nshlib/nsh_fscmds.c
##########
@@ -1675,8 +1687,26 @@ int cmd_rmdir(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
****************************************************************************/
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-#ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
int cmd_sh(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
+{
+ char cmd[CONFIG_NSH_LINELEN];
+
+ strncpy(cmd, "source ", sizeof(cmd) - 1);
+ strncat(cmd, argv[1], sizeof(cmd) - 1);
+
+ return system(cmd);
Review comment:
Or does the system command run on a pthread. In that case, then the & background option would work (although it ties up both a pthread and a task).
But the redirection as it is implemented for cmd_sh() will not work for system(cmd). Within NSH, redirection is handled via nsh_output(), stdin/stdout/stderr are not changed.
So even though you redirect the output at cmd_sh, will not be redirected by system(). This is not good. This is a good idea but not ready to come into the master.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-616153375
Oh, I forget this case. I will revert this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410911857
##########
File path: nshlib/nsh_altconsole.c
##########
@@ -292,12 +294,6 @@ int nsh_consolemain(int argc, char *argv[])
netinit_bringup();
#endif
-#if defined(CONFIG_NSH_ARCHINIT) && defined(CONFIG_BOARDCTL_FINALINIT)
- /* Perform architecture-specific final-initialization (if configured) */
-
- boardctl(BOARDIOC_FINALINIT, 0);
-#endif
-
Review comment:
I don't understand why this is removed. It seems unrelated to the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on issue #185:
nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615875439
> @patacongo - Do you see any harm in renaming sh to `source` and `.`
No, that seems appropriate to me. In that case, if 'sh' were implemented as a builtin task, it would just execute the nsh command 'source' or '.' in the context of the spawned task. Then it becomes sh since it has no effect on the parent task.
This would require update to documentation, however. These two would be affected: apps/nshlib/README.txt and nuttx/Documents/NuttShell.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in
pull request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410913625
##########
File path: nshlib/nsh_altconsole.c
##########
@@ -292,12 +294,6 @@ int nsh_consolemain(int argc, char *argv[])
netinit_bringup();
#endif
-#if defined(CONFIG_NSH_ARCHINIT) && defined(CONFIG_BOARDCTL_FINALINIT)
- /* Perform architecture-specific final-initialization (if configured) */
-
- boardctl(BOARDIOC_FINALINIT, 0);
-#endif
-
Review comment:
BOARDIOC_FINALINIT spread in many place, it's better to put into one place: nsh_initscript.
Since this PR contain all issues I found in reviewing nsh iniitialization process, it's better to review each patch one by one at here:
https://github.com/apache/incubator-nuttx-apps/pull/185/commits
Each commit message describe why I make this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on issue
#185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615844029
> There is a logic flaw in hsow system() is called. The system() call will block until the cmd is executed. Any & or redirection on the command will not be handled. Too much breakage! That is not acceptable and must be fixed.
>
But the old implementation call nsh_script directly, which also block until the script finish execution. If the old behaviour support &, then the new should support too, because the only difference is that the new one has a longer call chain:
new: cmd_sh->system->posix_task->nsh_system->nsh_parse->nsh_source->nsh_script
old: cmd_sh->nsh_script
The real action happen inside nsh_script, so the key is that nsh_script need to enhance to support &.
> Hmmm... Or does the system command run on a pthread via cmd_sh(). In that case, then the & background option would work (although it ties up both a pthread and a task).
>
Should we put the logic into nsh_script? since we have support:
sh /path/to/script &
source /path/to/script &
Both will call nsh_script finally.
> But the redirection as it is implemented for cmd_sh() will not work for system(cmd). Within NSH, redirection is handled via nsh_output(), stdin/stdout/stderr are not changed.
>
Yes, if the redirection just change the internal parser state not stdin/stdtout/stderr, the new nsh task can't inherit the redirection setting.
> So even though you redirect the output at cmd_sh, will not be redirected by system(). This is not good. This is a good idea but not ready to come into the master.
How about we modify stdin/stdout/stderr directly?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] davids5 commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410178966
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
@xiaoxiang781216 - Will these changes cause existing scripts to behave differently? I.E inherited vars, and flags?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] davids5 commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410429330
##########
File path: nshlib/nsh_command.c
##########
@@ -481,6 +481,10 @@ static const struct cmdmap_s g_cmdmap[] =
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
{ "sh", cmd_sh, 2, 2, "<script-path>" },
+# ifdef CONFIG_SYSTEM_NSH_PROGNAME
+ { CONFIG_SYSTEM_NSH_PROGNAME,
+ cmd_sh, 1, 2, "<script-path>" },
Review comment:
Will this break existing scripts? If so Please add a porting guide to this PR and note ALL the breaking changes that this PR is introducing.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615393620
@btashton this PR always fail to pull docker image, I add your patch and see what happen.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] davids5 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615870966
@patacongo - Do you see any harm in renaming sh to `source` and `.`
I would also add the concern of stack nesting and fragmentation to the list.
Please contrast the existing implementation to the proposed one in terms of stack nesting and fragmentation when the /etc/rind.d/rcS has
`sh ./l1` and l1 has `sh .l2`
From a small platform with the goal of having a clean set of scripts, that really just needs `sh` to include the as if it were one file.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on issue #185:
nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615867957
An additional not about CONFIG_BUILD_KERNEL mode. In this mode, both nsh and sh would be programs in a file system. 'sh' would not be a builtin task.
Also, create_task(), task_spawn() and any other API that references physical address cannot be used in kernel mode.
apps/system/system already has supports from running nsh from a file system, but requires the setting CONFIG_SYSTEM_SYSTEM_SHPATH.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] davids5 commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410687476
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
@xiaoxiang781216 - Thank you! are you implementing "." for source? The would help, on constrained platforms.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in
pull request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410685725
##########
File path: nshlib/nsh_command.c
##########
@@ -481,6 +481,10 @@ static const struct cmdmap_s g_cmdmap[] =
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
{ "sh", cmd_sh, 2, 2, "<script-path>" },
+# ifdef CONFIG_SYSTEM_NSH_PROGNAME
+ { CONFIG_SYSTEM_NSH_PROGNAME,
+ cmd_sh, 1, 2, "<script-path>" },
Review comment:
Yes, this change will forbid run nsh without argument. It's very dangerous because nsh:main will do some one time initialization by checking whether the argument is empty.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615879465
> Ok, I will rename sh to source and provide a new sh implemenation like nsh.
The alias '.' would be nice to. Like the current test has '[' as its alias.
I am not sure what you mean by "a new sh implemenation like nsh."
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410504235
##########
File path: nshlib/nsh_fscmds.c
##########
@@ -1675,8 +1687,26 @@ int cmd_rmdir(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
****************************************************************************/
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-#ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
int cmd_sh(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
+{
+ char cmd[CONFIG_NSH_LINELEN];
+
+ strncpy(cmd, "source ", sizeof(cmd) - 1);
+ strncat(cmd, argv[1], sizeof(cmd) - 1);
+
+ return system(cmd);
Review comment:
There is a logic flaw here. The system() call will block until the cmd is executed. Any & or redirection on the command will not be handled. That is incorrect and must be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in
pull request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410680271
##########
File path: system/system/system.c
##########
@@ -148,7 +148,7 @@ int system(FAR const char *cmd)
errcode = posix_spawn(&pid, CONFIG_SYSTEM_SYSTEM_SHPATH, NULL, &attr,
argv, (FAR char * const *)NULL);
#else
- errcode = task_spawn(&pid, "popen", nsh_system, NULL, &attr,
+ errcode = task_spawn(&pid, "system", nsh_system, NULL, &attr,
Review comment:
This argument give the new task a name:
1.system function should get the name "system"
2.popen function should get the name "popen"
I think this is a typo error and then make a 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615867367
> > After thinking more, I think that make 'sh' a built-in function might solve all of the issues that I raised. The & backgrounding would working correctly without tying up a pthread; and re-redirection of I/O would work too.
> > This is really quite a simple and usable alternative design. Please consider it.
>
> You mean run the script inside a new pthread? but the script still can change the task environment.
No, "built-in" tasks as managed by app/builtin are not pthreads. They are normal tasks.
If you create a apps/system/sh builtin task, you an execute it from the the nsh command line just as with and of the nsh internal commands. It will run in a separate task (not a pthread). Any changes made in the environment variables will be local to that built in task, and will not change the environment in the parent task.
In addition, thesed built-in tasks will support running in foreground or background and will support re-direction of I/O. They will work exactly as we need sh to work in all ways. The proposed solutions solves one problem but breaks many critical features. Creating apps/system/sh will solve all provides with no loss in features.
I highy recommend that you consider using a built in task at apps/system/sh and not break the good things in the current implementation of sh.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615877850
Ok, I will rename sh to source and provide a new sh implemenation like nsh.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in
pull request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410680852
##########
File path: nshlib/nsh.h
##########
@@ -1008,10 +1008,15 @@ int cmd_irqinfo(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
int cmd_readlink(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv);
#endif
#if CONFIG_NFILE_STREAMS > 0 && !defined(CONFIG_NSH_DISABLESCRIPT)
-# ifndef CONFIG_NSH_DISABLE_SH
+#if !defined(CONFIG_NSH_DISABLE_SH) && defined(CONFIG_SYSTEM_SYSTEM)
Review comment:
Yes, the old sh modify the parent environment directly which don't complain with all other POSIX variant implementation. That is why I made this change.
@davids5 the old sh behaviour can achieve with the new command "source" I add in the same patch.
1.The new 'source' command like the old 'sh' execute the script in the current nsh task
2.The new 'sh' implementation execute the script in the new task like other POSIX OS.
3./etc/init.d/rcS and /etc/.nshrc still run inside the current nsh task like before.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615469892
The PR check failure appears to be a real error:
find: /Users/runner/runners/2.169.0/work/incubator-nuttx-apps/incubator-nuttx-apps/apps/bin: No such file or directory
make[1]: *** No rule to make target `/Users/runner/runners/2.169.0/work/incubator-nuttx-apps/incubator-nuttx-apps/nuttx/inclu', needed by `bin/lib_puts.o'. Stop.
At least the path "/Users/runner/runners/2.169.0/work/incubator-nuttx-apps/incubator-nuttx-apps/nuttx/inclu" appears to be incomplete.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in
pull request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410913625
##########
File path: nshlib/nsh_altconsole.c
##########
@@ -292,12 +294,6 @@ int nsh_consolemain(int argc, char *argv[])
netinit_bringup();
#endif
-#if defined(CONFIG_NSH_ARCHINIT) && defined(CONFIG_BOARDCTL_FINALINIT)
- /* Perform architecture-specific final-initialization (if configured) */
-
- boardctl(BOARDIOC_FINALINIT, 0);
-#endif
-
Review comment:
BOARDIOC_FINALINIT spread in many place, it's better to put into one place: nsh_initscript.
Since this PR contain all issues I found in reviewing nsh iniitialization process, it's better to review each patch one by one at here:
https://github.com/apache/incubator-nuttx-apps/pull/185/commits
Each commit message describe why I make 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-616148130
> I found that we forget to call boardctl(BOARDIOC_FINALINIT) when CONFIG_NSH_TELNET equals y and CONFIG_NSH_CONSOLE equals n. Calling boardctl(BOARDIOC_FINALINIT) inside nsh_initscript can fix this problem and avoid the code duplication.
But isn't nsh_initscript only called if there is an initialization script? Does that mean that the function calls moved tin nsh_initscript() will not be executed if there is no initialization script (the default 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615846221
> The PR check failure appears to be a real error:
>
> ```
> find: /Users/runner/runners/2.169.0/work/incubator-nuttx-apps/incubator-nuttx-apps/apps/bin: No such file or directory
> make[1]: *** No rule to make target `/Users/runner/runners/2.169.0/work/incubator-nuttx-apps/incubator-nuttx-apps/nuttx/inclu', needed by `bin/lib_puts.o'. Stop.
> ```
>
> At least the path "/Users/runner/runners/2.169.0/work/incubator-nuttx-apps/incubator-nuttx-apps/nuttx/inclu" appears to be incomplete.
This may due to the parallel build issue we haven't fixed yet.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] patacongo commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615867957
An additional not about CONFIG_BUILD_KERNEL mode. In this mode, both nsh and sh would be programs in a file system. Also, create_task(), task_spawn() and any other API that references physical address cannot be used.
apps/system/system already has supports from running nsh from a file system, but requires the setting CONFIG_SYSTEM_SYSTEM_SHPATH.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-616144601
> The change is more complex than I expected and appears to include additional, unexplained changes. For example, why is boardctl(BOARDIOC_FINALINIT) removed?
I found that we forget to call boardctl(BOARDIOC_FINALINIT) when CONFIG_NSH_TELNET equals y and CONFIG_NSH_CONSOLE equals n. Calling boardctl(BOARDIOC_FINALINIT) inside nsh_initscript can fix this problem and avoid the code duplication.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on issue #185: nsh
improvement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#issuecomment-615844602
> After thinking more, I think that make 'sh' a built-in function might solve all of the issues that I raised. The & backgrounding would working correctly without tying up a pthread; and re-redirection of I/O would work too.
>
> This is really quite a simple and usable alternative design. Please consider it.
You mean run the script inside a new pthread? but the script still can change the task environment.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx-apps] davids5 commented on a change in pull
request #185: nsh improvement
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #185: nsh improvement
URL: https://github.com/apache/incubator-nuttx-apps/pull/185#discussion_r410177410
##########
File path: system/system/system.c
##########
@@ -148,7 +148,7 @@ int system(FAR const char *cmd)
errcode = posix_spawn(&pid, CONFIG_SYSTEM_SYSTEM_SHPATH, NULL, &attr,
argv, (FAR char * const *)NULL);
#else
- errcode = task_spawn(&pid, "popen", nsh_system, NULL, &attr,
+ errcode = task_spawn(&pid, "system", nsh_system, NULL, &attr,
Review comment:
@xiaoxiang781216 What is the reason/motivation for this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services