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 2021/07/01 14:12:58 UTC
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 opened a new pull request #791: Revert "Follow up task_spawn change from kernel side"
xiaoxiang781216 opened a new pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791
## Summary
since the prototype of task_spawn restore to the origin
This reverts commit 58293abb8e896bb04f3a76bf8b48206debe68f26.
Depends on https://github.com/apache/incubator-nuttx/pull/4027
## Impact
Avoid API compatibility issue
## 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] davids5 commented on pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#issuecomment-872287470
@xiaoxiang781216 I understand that @slorquet took issue with this change. It was done in master not the last release correct? Even if it were in the last release. It will show up in the build. I do not think is should be reverted it just needs to be documented as a breaking (build) change.
How can we improve if we keep it all the same?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] slorquet edited a comment on pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
slorquet edited a comment on pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#issuecomment-872334098
@davids5 yep no issue with this technical change, it saves a bit of stack
posix_spawn has the *pid parameter, but the function here is task_spawn, which is not in posix (seems so)
Having task_spawn returns the PID and have one less parameter is at least consistent with VXWorks: http://beru.univ-brest.fr/~singhoff/ENS/UE_temps_reel/TP-VXWORKS/CORRECTION/Man-VxWorks-1.pdf
yes it's in master, so there are no release notes yet. This might just need to be a reminder to document this in the next release notes :)
Moreover, you have no ability to test this, because all is going to be well if the apps master matches the nuttx master
The issue happens only with "old" apps trying to use a new nuttx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#discussion_r662714651
##########
File path: system/popen/popen.c
##########
@@ -253,12 +253,8 @@ FILE *popen(FAR const char *command, FAR const char *mode)
errcode = posix_spawn(&container->shell, argv[0], &file_actions,
&attr, argv, (FAR char * const *)NULL);
#else
- container->shell = task_spawn("popen", nsh_system, &file_actions,
- &attr, argv + 1, (FAR char * const *)NULL);
- if (container->shell < 0)
- {
- errcode = -container->shell;
- }
+ errcode = task_spawn(&container->shell, "popen", nsh_system, &file_actions,
+ &attr, argv, (FAR char * const *)NULL);
Review comment:
Please see #763 and apache/incubator-nuttx#3916.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 edited a comment on pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#issuecomment-872289412
> @xiaoxiang781216 I understand that @slorquet took issue with this change. It was done in master not the last release correct? Even if it were in the last release. It will show up in the build. I do not think is should be reverted it just needs to be documented as a breaking (build) change.
>
> How can we improve if we keep it all the same?
task_spawn restore the old prototype and behaviour, nxtask_spawn has the new prototype and behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] hartmannathan commented on a change in pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#discussion_r662393172
##########
File path: system/popen/popen.c
##########
@@ -253,12 +253,8 @@ FILE *popen(FAR const char *command, FAR const char *mode)
errcode = posix_spawn(&container->shell, argv[0], &file_actions,
&attr, argv, (FAR char * const *)NULL);
#else
- container->shell = task_spawn("popen", nsh_system, &file_actions,
- &attr, argv + 1, (FAR char * const *)NULL);
- if (container->shell < 0)
- {
- errcode = -container->shell;
- }
+ errcode = task_spawn(&container->shell, "popen", nsh_system, &file_actions,
+ &attr, argv, (FAR char * const *)NULL);
Review comment:
I don't understand why `argv + 1` is becoming `argv`. Not saying it's wrong, just mentioning it to make sure it isn't by mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#issuecomment-872289412
> @xiaoxiang781216 I understand that @slorquet took issue with this change. It was done in master not the last release correct? Even if it were in the last release. It will show up in the build. I do not think is should be reverted it just needs to be documented as a breaking (build) change.
>
> How can we improve if we keep it all the same?
task_spawn has the old prototype and behaviour, nxtask_spawn has the new prototype and behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 closed pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] slorquet edited a comment on pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
slorquet edited a comment on pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#issuecomment-872334098
@davids5 yep no issue with this technical change, it saves a bit of stack
posix_spawn has the *pid parameter, but the function here is task_spawn, which is not in posix (seems so)
Having task_spawn returns the PID and have one less parameter is at least consistent with VXWorks: http://beru.univ-brest.fr/~singhoff/ENS/UE_temps_reel/TP-VXWORKS/CORRECTION/Man-VxWorks-1.pdf
yes it's in master, so there are no release notes yet. This might just need to be a reminder to document this in the next release notes :)
Moreover, you have no ability to test this, because all is going to be well if the apps master matches the nuttx master
The issue happens only with "old" apps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] hartmannathan commented on a change in pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#discussion_r662393302
##########
File path: system/system/system.c
##########
@@ -137,12 +137,8 @@ int system(FAR const char *cmd)
errcode = posix_spawn(&pid, argv[0], NULL, &attr,
argv, (FAR char * const *)NULL);
#else
- pid = task_spawn("system", nsh_system, NULL, &attr,
- argv + 1, (FAR char * const *)NULL);
- if (pid < 0)
- {
- errcode = -pid;
- }
+ errcode = task_spawn(&pid, "system", nsh_system, NULL, &attr,
+ argv, (FAR char * const *)NULL);
Review comment:
Same comment here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] slorquet commented on pull request #791: Revert "Follow up task_spawn change from kernel side"
Posted by GitBox <gi...@apache.org>.
slorquet commented on pull request #791:
URL: https://github.com/apache/incubator-nuttx-apps/pull/791#issuecomment-872334098
@davids5 yep no issue with this technical change, it saves a bit of stack and is more posix compliant
posix_spawn has the *pid parameter, but the function here is task_spawn, which is not in posix (seems so)
Having task_spawn returns the PID and have one less parameter is at least consistent with VXWorks: http://beru.univ-brest.fr/~singhoff/ENS/UE_temps_reel/TP-VXWORKS/CORRECTION/Man-VxWorks-1.pdf
yes it's in master, so there are no release notes yet. This might just need to be a reminder to document this in the next release notes :)
Moreover, you have no ability to test this, because all is going to be well if the apps master matches the nuttx master
The issue happens only with "old" apps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org