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/08/17 15:33:48 UTC
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 opened a new pull request #367: nsh: Accept the command line arguments like sh
xiaoxiang781216 opened a new pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367
## Summary
See the dissussion here: https://github.com/apache/incubator-nuttx/pull/1597
## Impact
## 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-apps] davids5 merged pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367
----------------------------------------------------------------
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-apps] v01d commented on a change in pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#discussion_r472279693
##########
File path: nshlib/nsh_timcmds.c
##########
@@ -386,11 +392,11 @@ int cmd_date(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
}
else /* option = '?' */
{
- /* We need to parse to the end anyway so that getopt stays healthy */
+ /* We need parse to the end anyway so that getopt stays healthy */
Review comment:
this comment change does not make sense
----------------------------------------------------------------
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-apps] xiaoxiang781216 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-676570803
But I am different from yours, here is nsh arguments:
and include/nuttx/config.h:
Yes, .config contain the empty string:
But mkconfig will remove the empty string macro, do you have this line:
https://github.com/apache/incubator-nuttx/blob/master/tools/cfgdefine.c#L69
----------------------------------------------------------------
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-apps] v01d commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-675549100
I'm not that familiar with this part of NuttX but I couldn't find anything that caught my attention.
I'm curious, how do you manage nsh to exit? I'm assuming nsh starts some other task via init script, which
you fork, otherwise it this child task would also exit once nsh exits.
In that case, wouldn't be simpler to just add an "exit" at the end? Also, it is typical to end this kind of scripts with an exec()
call so that the calling process (nsh) is replaced by your second task. Not sure if that works in NuttX though.
Anyway, this PR is more general than that, so I agree with the change and aligns with your previous 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
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-675562777
> I'm not that familiar with this part of NuttX but I couldn't find anything that caught my attention.
>
> I'm curious, how do you manage nsh to exit? I'm assuming nsh starts some other task via init script, which
> you fork, otherwise it this child task would also exit once nsh exits.
Yes, we have many services which is started by init.d/rcS run on the background.
> In that case, wouldn't be simpler to just add an "exit" at the end? Also, it is typical to end this kind of scripts with an exec()
> call so that the calling process (nsh) is replaced by your second task. Not sure if that works in NuttX though.
>
Yes, adding 'exit' at the end of init.d/rcS should work, but we want to control the behaviour through defconfig since developers are more familiar to modify defconfig than init.d/rcS. And this also make nsh align with sh's behaviour.
> Anyway, this PR is more general than that, so I agree with the change and aligns with your previous 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
[GitHub] [incubator-nuttx-apps] davids5 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-675599362
@xiaoxiang781216 - I would like to test this and the sister PR. But I need some time. I am fixing other breakage in kinetis at the moment.
----------------------------------------------------------------
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-apps] xiaoxiang781216 commented on a change in pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#discussion_r472284461
##########
File path: nshlib/nsh_timcmds.c
##########
@@ -386,11 +392,11 @@ int cmd_date(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
}
else /* option = '?' */
{
- /* We need to parse to the end anyway so that getopt stays healthy */
+ /* We need parse to the end anyway so that getopt stays healthy */
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-675541590
@patacongo @v01d @davids5 could you review this patch which is paired with https://github.com/apache/incubator-nuttx/pull/1597? so user can let nsh exit after the initialization finish by:
```
CONFIG_INIT_ARGS="\"-c\", \"exit\""
```
----------------------------------------------------------------
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-apps] davids5 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-676587004
@xiaoxiang781216 - yes I have it,
Sorry, the problem was old version of mkconfig binary in my tools dir. I need to fix the out of tree build on our side.
----------------------------------------------------------------
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-apps] xiaoxiang781216 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-676480174
Here is my test(sim:nsh), the behaviour is good:
```
xiaoxiang@DESKTOP-JH2VEP6:~/nuttx/nuttx$ ./nuttx
nsh: mkrd: command not found
login: admin
password:
User Logged-in!
NuttShell (NSH) NuttX-9.1.0
MOTD: username=admin password=Administrator
nsh> poweroff
```
Could you set breakpoint in fopen? so I can see the context when the error happen, or try on sim: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
[GitHub] [incubator-nuttx-apps] davids5 edited a comment on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-676531990
This was a complicated debug, it was the last open on nested scripts.....
The info you asked for is above.
I believe this is wrong https://github.com/apache/incubator-nuttx-apps/pull/367/files#diff-1037d1ad14b4ee447c6ca07d40ccb9f7R131
I have no clue why you do not see it on the sim.
But it may what you are passing "" is not \0
----------------------------------------------------------------
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-apps] xiaoxiang781216 edited a comment on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-675562777
> I'm not that familiar with this part of NuttX but I couldn't find anything that caught my attention.
>
> I'm curious, how do you manage nsh to exit? I'm assuming nsh starts some other task via init script, which
> you fork, otherwise it this child task would also exit once nsh exits.
Yes, we have many services which is started by init.d/rcS run on the background.
> In that case, wouldn't be simpler to just add an "exit" at the end? Also, it is typical to end this kind of scripts with an exec()
> call so that the calling process (nsh) is replaced by your second task. Not sure if that works in NuttX though.
>
Yes, adding 'exit' at the end of init.d/rcS should work too, but we want to control the behaviour through defconfig since developers are more familiar to modify defconfig than init.d/rcS. And this also make nsh align with sh's behaviour.
> Anyway, this PR is more general than that, so I agree with the change and aligns with your previous 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
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-675541590
@patacongo @v01d @davids5 could you review this patch which is paired with https://github.com/apache/incubator-nuttx/pull/1597? so user can let nsh exit to save at least 4KB RAM after the initialization finish by:
```
CONFIG_INIT_ARGS="\"-c\", \"exit\""
```
----------------------------------------------------------------
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-apps] davids5 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-676545121
@xiaoxiang781216 this is the problem the PR apache/incubator-nuttx#1597 did not do what @patacongo asked for with CONFIG_INIT_ARGS
from config.h
`#define CONFIG_INIT_ARGS ""`
----------------------------------------------------------------
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-apps] davids5 commented on pull request #367: nsh: Accept the command line arguments like sh
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #367:
URL: https://github.com/apache/incubator-nuttx-apps/pull/367#issuecomment-676531990
This was a complicated debug, it was the last open on nested scripts.....
The info you asked for is above.
I believe this is wrong https://github.com/apache/incubator-nuttx-apps/pull/367/files#diff-1037d1ad14b4ee447c6ca07d40ccb9f7R131
I have no clue why you do not see it on the sim.
----------------------------------------------------------------
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