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/03/21 04:23:29 UTC
[GitHub] [incubator-nuttx] yamt opened a new pull request #599: sim: Add a
CLI option to override vpnkit socket
yamt opened a new pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395994709
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
> @xiaoxiang781216
> how would it skip an unknown option, say, -g?
> note that -g might or might not take an argument.
Yes, getopt can't handle the option with argument. But I still think the code struct is better even we have to parse the argument directly, do you think so?
----------------------------------------------------------------
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] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r396185811
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
i agree environment variables don't have modularity issues. i feel it's a bit backdoor-ish though.
----------------------------------------------------------------
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] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395991229
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
@xiaoxiang781216
how would it skip an unknown option, say, -g?
note that -g might or might not take an argument.
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395964124
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
How about we just save argc and argv in some global variable? Then each component could invoke getopt to parse the argument who is interesting. The advantage is that:
1.Don't need expose the component internal detail to the world
2.Don't need modify the common code to add the new option
----------------------------------------------------------------
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] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395986831
##########
File path: boards/sim/sim/sim/NETWORK-VPNKIT.txt
##########
@@ -34,6 +35,3 @@ You can use it as the following:
% vpnkit --ethernet /tmp/vpnkit-nuttx &
% ./nuttx
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r396175198
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
maybe. a command line option is more convenient for me though.
may i move the command line option part to a separate PR? i haven't expected it was this controversial.
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395989688
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
> getopt doesn't work that way. or are you suggesting to stop using getopt?
Why we can't use getopt like this:
```
while ((option = getopt(argc, argv, "f")) != ERROR)
{
if (option == 'f')
{
...
}
}
```
----------------------------------------------------------------
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] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395998371
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
it depends.
if we invent a "generic" framework, how many components would use it?
if it's just a few or several, i guess #ifdef is good enough.
----------------------------------------------------------------
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] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395967568
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
@xiaoxiang781216 with your suggestion, how can component-A skip options for component-B?
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r396001633
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
BTW, is it enough that we just provide Kconfig like tap bridge?
----------------------------------------------------------------
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] yamt commented on issue #599: sim: Add a CLI
option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on issue #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#issuecomment-602328693
i'll create separate PRs.
----------------------------------------------------------------
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] yamt closed pull request #599: sim: Add a CLI
option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt closed pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599
----------------------------------------------------------------
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] yamt commented on a change in pull request #599:
sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395986287
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
getopt doesn't work that way. or are you suggesting to stop using getopt?
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395976582
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
Just ignore the option he doesn't understand.
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r395976693
##########
File path: boards/sim/sim/sim/NETWORK-VPNKIT.txt
##########
@@ -34,6 +35,3 @@ You can use it as the following:
% vpnkit --ethernet /tmp/vpnkit-nuttx &
% ./nuttx
Review comment:
How about add --vpnkit-sock description 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r396183049
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
> maybe. a command line option is more convenient for me though.
>
> may i move the command line option part to a separate PR? i haven't expected it was this controversial.
Sure, thanks for your support.
BTW, how about we use environment variable which is more easier than command line for extension.
----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull
request #599: sim: Add a CLI option to override vpnkit socket
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #599: sim: Add a CLI option to override vpnkit socket
URL: https://github.com/apache/incubator-nuttx/pull/599#discussion_r396183049
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -77,6 +78,8 @@ static char g_logbuffer[4096];
int main(int argc, char **argv, char **envp)
{
+ process_cli_options(argc, argv);
Review comment:
> maybe. a command line option is more convenient for me though.
>
> may i move the command line option part to a separate PR? i haven't expected it was this controversial.
Sure, thanks for your support.
BTW, how about we use environment variable which is more easier than command line for extension.
1.Command line is normally use for the frequent used option
2.Environment variable is normallhy used for the seldom changed option
And unix domain socket path is the second case, right?
----------------------------------------------------------------
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