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/12/14 06:09:38 UTC
[GitHub] [incubator-nuttx-apps] masayuki2009 opened a new pull request #521: nshlib: Fix nsh_usbconsole.c
masayuki2009 opened a new pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521
## Summary
- stdin/stdout/stderr are now preallocated in libc and fs_fd
in file_struct are also initialized to -1
- So we need to call fdopen() for stdin again as we did before.
- Also, cn_outstream and cn_errstream are not needed to be set.
- See apps/nshlib/nsh_console.h as well
## Impact
- nsh_usbconsole.c only
## Testing
- Tested with stm32f4discovery:usbnsh
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744320682
@michallenc could you try this patch with your new config(https://github.com/apache/incubator-nuttx/pull/2526)?
----------------------------------------------------------------
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542395630
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
> But, I am curious that line 87-89 already duplicate fd to 0-2, why we still need call fdopen at line 98-100 again?
The file descriptors are duplicated, but the the streams are still not opened due to the failure of fs_fdopen() when the IDLE thread was created. Duplicating the file descriptor does not create the stream.
This should not be done in the the OS (not the application) OR it could be done in _all_ applications and the non-standard requirement should be well documented. The latter is basically the way things worked before your change that broke 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-apps] masayuki2009 commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744197633
See https://github.com/apache/incubator-nuttx/pull/2526 as well
----------------------------------------------------------------
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] patacongo edited a comment on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744429948
What about Telnet. If networking were enabled, would Telnet not also fail in exactly the same way?
This solution cannot be implemented in a single application like NSH. It _must_ be implemented as a system-wide solution that fixes all possible applications that can be started with no fixed console device.
EVERY application that redirects stdin, stdout, and stderr can fail this way. This cannot be an application side solution. Especially becuase it is non-standard and undocumented.
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744215357
Final patch to fix: https://github.com/apache/incubator-nuttx/issues/2203
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542427013
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
> > But, I am curious that line 87-89 already duplicate fd to 0-2, why we still need call fdopen at line 98-100 again?
>
> The file descriptors are duplicated, but the the streams are still not opened due to the failure of fs_fdopen() when the IDLE thread was created. Duplicating the file descriptor does not create the stream.
>
Ok, I understand the situation now.
> This should not be done in the the OS (not the application) OR it could be done in _all_ applications and the non-standard requirement should be well documented.
Maybe the right thing we should do:
1. Always enable DEV_ZERO or DEV_NULL
2. Or skip check 0-2 valid in fs_fdopen
> The latter is basically the way things worked before your change that broke this.
But with the patch: https://github.com/apache/incubator-nuttx/pull/2263
I think the new behaviour is same as before now. If CONFIG_DEV_CONSOLE isn't enabled, fdopen is always required regardless whether my patch apply.
----------------------------------------------------------------
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] patacongo commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744429948
What about Telnet. If networking were enabled, would Telnet not also fail in exactly the same way?
This solution cannot be implemented in a single application like NSH. It _must_ be implemented as a system-wide solution that fixes all possible applications that can be started with no fixed console device.
----------------------------------------------------------------
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] btashton commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744220660
I'm confused because this seems to back out the change that came in from @masayuki2009 comment here https://github.com/apache/incubator-nuttx-apps/pull/425#issuecomment-718398878
I just want to make sure we are not missing something.
----------------------------------------------------------------
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542348706
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
This is generally not needed. The fact that fdopen() corrects the problem is an artifact of the fact that CONFIG_DEV_CONSOLE is not set in the configuration.
See sched/group/goupr_setupidlefiles.c:
98 #ifdef CONFIG_DEV_CONSOLE
99 fd = nx_open("/dev/console", O_RDWR);
100 if (fd == 0)
101 {
102 /* Successfully opened /dev/console as stdin (fd == 0) */
103
104 fs_dupfd2(0, 1);
105 fs_dupfd2(0, 2);
106 }
125 #endif
Subsequently in fdopen() calls in group_setupstreams() will fail in fs/vfs/fd_open() because the file descriptor check will fail, fs_chedfd().
I believe that it is the logic in sched/group/goupr_setupidlefiles.c. Since stdin, stdout, and stderr must always be available, it is wrong to permit CONFIG_DEV_CONSOLE to not be set.
A better solution would be to use /dev/null for tstdin, stdout, and stderr if CONFIG_DEV_CONSOLE is not set. Otherwise, EVERY application that works with no initial, default console must use this same non-standard, non-documented trick.
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542368040
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
but dev/null or dev/zero may disable too.
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542427013
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
> > But, I am curious that line 87-89 already duplicate fd to 0-2, why we still need call fdopen at line 98-100 again?
>
> The file descriptors are duplicated, but the the streams are still not opened due to the failure of fs_fdopen() when the IDLE thread was created. Duplicating the file descriptor does not create the stream.
>
Ok, I understand the situation now.
> This should not be done in the the OS (not the application) OR it could be done in _all_ applications and the non-standard requirement should be well documented.
Maybe the right thing we should do:
1. Always enable DEV_ZERO or DEV_NULL
2. Or skip check 0-2 valid in fs_fdopen
The second approach is reasonable since VFS will always ensure 0-2 is valid when the usespace issue the file system operation. And then we can remove fs_fdopen too.
> The latter is basically the way things worked before your change that broke this.
But with the patch: https://github.com/apache/incubator-nuttx/pull/2263
I think the new behaviour is same as before now. If CONFIG_DEV_CONSOLE isn't enabled, fdopen is always required regardless whether my patch apply.
----------------------------------------------------------------
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] btashton commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744205956
@masayuki2009 This lines up with the change I was making, but I am unclear now why we needed to add `FAR struct console_stdio_s *pstate` to the function definition in the first place.
----------------------------------------------------------------
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542348706
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
This is generally not needed. The fact that fdopen() corrects the problem is an artifact of the fact that CONFIG_DEV_CONSOLE is not set in the configuration.
See sched/group/goupr_setupidlefiles.c:
98 #ifdef CONFIG_DEV_CONSOLE
99 fd = nx_open("/dev/console", O_RDWR);
100 if (fd == 0)
101 {
102 /* Successfully opened /dev/console as stdin (fd == 0) */
103
104 fs_dupfd2(0, 1);
105 fs_dupfd2(0, 2);
106 }
125 #endif
I believe that it is the logic in sched/group/goupr_setupidlefiles.c. Since stdin, stdout, and stderr must always be available, it is wrong to permit CONFIG_DEV_CONSOLE to not be set.
A better solution would be to use /dev/null for tstdin, stdout, and stderr if CONFIG_DEV_CONSOLE is not set. Otherwise, EVERY application that works with no initial, default console must use this same non-standard, non-documented trick.
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542380844
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
But, I am curious that line 87-89 already duplicate fd to 0-2, why we still need call fdopen at line 98-100 again?
----------------------------------------------------------------
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 merged pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521
----------------------------------------------------------------
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 #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744340606
Thanks @michallenc I will merge both patch 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-apps] masayuki2009 commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-824443431
FYI.
https://github.com/apache/incubator-nuttx-apps/pull/523
--
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542372434
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
> but dev/null or dev/zero may disable too.
Yes, it would have to e changed so that it cannot be disabled. Not a big deal.
----------------------------------------------------------------
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] masayuki2009 commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744210401
> @masayuki2009 This lines up with the change I was making, but I am unclear now why we needed to add `FAR struct console_stdio_s *pstate` to the function definition in the first place.
OK, I will remove it later.
----------------------------------------------------------------
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542395630
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
> But, I am curious that line 87-89 already duplicate fd to 0-2, why we still need call fdopen at line 98-100 again?
The file descriptors are duplicated, but the the streams are still not opened due to the failure of fs_fdopen() when the IDLE thread was created. Duplicating the file descriptor does not create the stream.
----------------------------------------------------------------
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542395630
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
> But, I am curious that line 87-89 already duplicate fd to 0-2, why we still need call fdopen at line 98-100 again?
The file descriptors are duplicated, but the the streams are still not opened due to the failure of fs_fdopen() when the IDLE thread was created. Duplicating the file descriptor does not create the stream.
This should not be done in the application OR it should be done in all applications and the non-standard requirement should be well documented.
----------------------------------------------------------------
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] michallenc commented on pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
michallenc commented on pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#issuecomment-744331133
> @michallenc could you try this patch with your new config([apache/incubator-nuttx#2526](https://github.com/apache/incubator-nuttx/pull/2526))?
@xiaoxiang781216 Sure. This seems to be working fine.
```
NuttShell (NSH) NuttX-9.1.1
nsh>
nsh>
nsh>
nsh> ls dev
/dev:
console
null
ttyACM0
ttyS0
nsh> ?
help usage: help [-v] [<cmd>]
. cd echo hexdump mh rm time xd
[ cp exec kill mount rmdir true
? cmp exit ls mv set uname
basename dirname false mb mw sleep umount
break dd free mkdir ps source unset
cat df help mkrd pwd test usleep
Builtin Apps:
sh nsh
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] patacongo commented on a change in pull request #521: nshlib: Fix nsh_usbconsole.c
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #521:
URL: https://github.com/apache/incubator-nuttx-apps/pull/521#discussion_r542348706
##########
File path: nshlib/nsh_usbconsole.c
##########
@@ -88,15 +88,16 @@ static void nsh_configstdio(int fd, FAR struct console_stdio_s *pstate)
dup2(fd, 1);
dup2(fd, 2);
- /* Setup the stdout */
-
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(1, "a");
-
- /* Setup the stderr */
+ /* fdopen to get the stdin, stdout and stderr streams.
+ *
+ * fd = 0 is stdin (read-only)
+ * fd = 1 is stdout (write-only, append)
+ * fd = 2 is stderr (write-only, append)
+ */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(2, "a");
+ fdopen(0, "r");
+ fdopen(1, "a");
+ fdopen(2, "a");
Review comment:
This is generally not needed. The fact that fdopen() corrects the problem is an artifact of the fact that CONFIG_DEV_CONSOLE is not set in the configuration.
See sched/group/goupr_setupidlefiles.c:
98 #ifdef CONFIG_DEV_CONSOLE
99 fd = nx_open("/dev/console", O_RDWR);
100 if (fd == 0)
101 {
102 /* Successfully opened /dev/console as stdin (fd == 0) */
103
104 fs_dupfd2(0, 1);
105 fs_dupfd2(0, 2);
106 }
125 #endif
I believe that it is the logic in sched/group/goupr_setupidlefiles.c. Since stdin, stdout, and stderr must always be available, it is wrong to permit CONFIG_DEV_CONSOLE to not be set.
A better solution would be to use /dev/null for tstdin, stdout, and stderr if CONFIG_DEV_CONSOLE is not set.
----------------------------------------------------------------
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