* [PATCH v3] conf: Stop parsing options at first non-option argument @ 2024-08-08 4:02 Stefano Brivio 2024-08-08 4:44 ` David Gibson 2024-08-08 9:37 ` Paul Holzinger 0 siblings, 2 replies; 7+ messages in thread From: Stefano Brivio @ 2024-08-08 4:02 UTC (permalink / raw) To: passt-dev; +Cc: Paul Holzinger, David Gibson Given that pasta supports specifying a command to be executed on the command line, even without the usual -- separator as long as there's no ambiguity, we shouldn't eat up options that are not meant for us. Paul reports, for instance, that with: pasta --config-net ip -6 route -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. That's because getopt_long(), by default, shuffles the argument list to shift non-option arguments at the end. Avoid that by adding '+' at the beginning of 'optstring'. Reported-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- v3: Use '+' in optstring and drop first non-option tracking v2: Instead of overriding 'name' in the getopt_long() loop, to force exiting the loop, adjust the exit condition conf.c | 4 ++-- util.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/conf.c b/conf.c index 2f5d649..a79e7a6 100644 --- a/conf.c +++ b/conf.c @@ -1253,9 +1253,9 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) { c->no_dhcp_dns = c->no_dhcp_dns_search = 1; fwd_default = FWD_AUTO; - optstring = "dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; } else { - optstring = "dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; } c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; diff --git a/util.c b/util.c index 7761bd3..0b41404 100644 --- a/util.c +++ b/util.c @@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv) int name, rc; do { - name = getopt_long(argc, argv, ":F", optfd, NULL); + name = getopt_long(argc, argv, "+:F", optfd, NULL); if (name == 'F') { errno = 0; -- @@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv) int name, rc; do { - name = getopt_long(argc, argv, ":F", optfd, NULL); + name = getopt_long(argc, argv, "+:F", optfd, NULL); if (name == 'F') { errno = 0; -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] conf: Stop parsing options at first non-option argument 2024-08-08 4:02 [PATCH v3] conf: Stop parsing options at first non-option argument Stefano Brivio @ 2024-08-08 4:44 ` David Gibson 2024-08-08 9:37 ` Paul Holzinger 1 sibling, 0 replies; 7+ messages in thread From: David Gibson @ 2024-08-08 4:44 UTC (permalink / raw) To: Stefano Brivio; +Cc: passt-dev, Paul Holzinger [-- Attachment #1: Type: text/plain, Size: 993 bytes --] On Thu, Aug 08, 2024 at 06:02:51AM +0200, Stefano Brivio wrote: > Given that pasta supports specifying a command to be executed on the > command line, even without the usual -- separator as long as there's > no ambiguity, we shouldn't eat up options that are not meant for us. > > Paul reports, for instance, that with: > > pasta --config-net ip -6 route > > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. > That's because getopt_long(), by default, shuffles the argument list > to shift non-option arguments at the end. > > Avoid that by adding '+' at the beginning of 'optstring'. > > Reported-by: Paul Holzinger <pholzing@redhat.com> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] conf: Stop parsing options at first non-option argument 2024-08-08 4:02 [PATCH v3] conf: Stop parsing options at first non-option argument Stefano Brivio 2024-08-08 4:44 ` David Gibson @ 2024-08-08 9:37 ` Paul Holzinger 2024-08-08 10:16 ` Stefano Brivio 1 sibling, 1 reply; 7+ messages in thread From: Paul Holzinger @ 2024-08-08 9:37 UTC (permalink / raw) To: Stefano Brivio, passt-dev; +Cc: David Gibson On 08/08/2024 06:02, Stefano Brivio wrote: > Given that pasta supports specifying a command to be executed on the > command line, even without the usual -- separator as long as there's > no ambiguity, we shouldn't eat up options that are not meant for us. > > Paul reports, for instance, that with: > > pasta --config-net ip -6 route > > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. > That's because getopt_long(), by default, shuffles the argument list > to shift non-option arguments at the end. > > Avoid that by adding '+' at the beginning of 'optstring'. > > Reported-by: Paul Holzinger <pholzing@redhat.com> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > --- > v3: Use '+' in optstring and drop first non-option tracking > > v2: Instead of overriding 'name' in the getopt_long() loop, to force > exiting the loop, adjust the exit condition > > conf.c | 4 ++-- > util.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > I like this change but I like to point out that this is a breaking change for any user that sets options after the main argument, i.e. pid. I can tell you that this will not effect podman but I don't know what other users exists out there... I am not sure if it is worth the risk just to improve the UX for the command use case but I guess you already decided it is otherwise you would have not posted this patch. -- Paul Holzinger ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] conf: Stop parsing options at first non-option argument 2024-08-08 9:37 ` Paul Holzinger @ 2024-08-08 10:16 ` Stefano Brivio [not found] ` <50d8ea4a-15ec-48a0-bc31-1e2a5139677b@redhat.com> 2024-08-08 18:33 ` Stefano Brivio 0 siblings, 2 replies; 7+ messages in thread From: Stefano Brivio @ 2024-08-08 10:16 UTC (permalink / raw) To: Paul Holzinger; +Cc: passt-dev, David Gibson On Thu, 8 Aug 2024 11:37:38 +0200 Paul Holzinger <pholzing@redhat.com> wrote: > On 08/08/2024 06:02, Stefano Brivio wrote: > > Given that pasta supports specifying a command to be executed on the > > command line, even without the usual -- separator as long as there's > > no ambiguity, we shouldn't eat up options that are not meant for us. > > > > Paul reports, for instance, that with: > > > > pasta --config-net ip -6 route > > > > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. > > That's because getopt_long(), by default, shuffles the argument list > > to shift non-option arguments at the end. > > > > Avoid that by adding '+' at the beginning of 'optstring'. > > > > Reported-by: Paul Holzinger <pholzing@redhat.com> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > --- > > v3: Use '+' in optstring and drop first non-option tracking > > > > v2: Instead of overriding 'name' in the getopt_long() loop, to force > > exiting the loop, adjust the exit condition > > > > conf.c | 4 ++-- > > util.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > I like this change but I like to point out that this is a breaking > change for any user that sets options after the main argument, i.e. pid. Oh, right, that actually happens to work, even if we never really supported that, the man page has only this form: pasta [OPTION]... PID I could go back to v2, and, on top of that, if we find a single non-option argument that looks like a PID (a number), we would push it to the end of argv and continue parsing. If we find any other number, including one that's after all the other options but before the presumed PID we just pushed, we'll report error. We have anyway the following problem, which we won't make any worse (it can't be done without an actual file with POSIX shell, Bash only): $ 1() { echo a; } $ pasta 1; echo Couldn't open user namespace /proc/1/ns/user: Permission denied $ pasta echo; 1 a > I can tell you that this will not effect podman but I don't know what > other users exists out there... As far as I know, the only other tool using pasta(1) at the moment is rootless-containers (Docker, Usernetes): https://github.com/rootless-containers/rootlesskit/blob/master/pkg/network/pasta/pasta.go which is also fine. Other users are developers and people who try out network topologies and namespaces stuff without root, but I suppose adding the PID at the end is pretty natural anyway. On the other hand, if we can make sure we avoid this kind of breakage at a small cost, why not. I'll try. > I am not sure if it is worth the risk just to improve the UX for the > command use case but I guess you already decided it is otherwise you > would have not posted this patch. No, not really, I wasn't actually aware of the fact that adding the PID before options worked. Thanks for pointing that out. -- Stefano ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <50d8ea4a-15ec-48a0-bc31-1e2a5139677b@redhat.com>]
* Re: [PATCH v3] conf: Stop parsing options at first non-option argument [not found] ` <50d8ea4a-15ec-48a0-bc31-1e2a5139677b@redhat.com> @ 2024-08-08 12:30 ` Stefano Brivio 0 siblings, 0 replies; 7+ messages in thread From: Stefano Brivio @ 2024-08-08 12:30 UTC (permalink / raw) To: Paul Holzinger; +Cc: passt-dev, David Gibson On Thu, 8 Aug 2024 13:29:09 +0200 Paul Holzinger <pholzing@redhat.com> wrote: > On 08/08/2024 12:16, Stefano Brivio wrote: > > On Thu, 8 Aug 2024 11:37:38 +0200 > > Paul Holzinger<pholzing@redhat.com> wrote: > > > >> On 08/08/2024 06:02, Stefano Brivio wrote: > >>> Given that pasta supports specifying a command to be executed on the > >>> command line, even without the usual -- separator as long as there's > >>> no ambiguity, we shouldn't eat up options that are not meant for us. > >>> > >>> Paul reports, for instance, that with: > >>> > >>> pasta --config-net ip -6 route > >>> > >>> -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. > >>> That's because getopt_long(), by default, shuffles the argument list > >>> to shift non-option arguments at the end. > >>> > >>> Avoid that by adding '+' at the beginning of 'optstring'. > >>> > >>> Reported-by: Paul Holzinger<pholzing@redhat.com> > >>> Signed-off-by: Stefano Brivio<sbrivio@redhat.com> > >>> --- > >>> v3: Use '+' in optstring and drop first non-option tracking > >>> > >>> v2: Instead of overriding 'name' in the getopt_long() loop, to force > >>> exiting the loop, adjust the exit condition > >>> > >>> conf.c | 4 ++-- > >>> util.c | 2 +- > >>> 2 files changed, 3 insertions(+), 3 deletions(-) > >>> > >> I like this change but I like to point out that this is a breaking > >> change for any user that sets options after the main argument, i.e. pid. > > Oh, right, that actually happens to work, even if we never really > > supported that, the man page has only this form: > > > > pasta [OPTION]... PID > > > > I could go back to v2, and, on top of that, if we find a single > > non-option argument that looks like a PID (a number), we would push it > > to the end of argv and continue parsing. > > > > If we find any other number, including one that's after all the other > > options but before the presumed PID we just pushed, we'll report error. > > > > We have anyway the following problem, which we won't make any worse (it > > can't be done without an actual file with POSIX shell, Bash only): > > > > $ 1() { echo a; } > > $ pasta 1; echo > > Couldn't open user namespace /proc/1/ns/user: Permission denied > > > > $ pasta echo; 1 > > > > a > > > >> I can tell you that this will not effect podman but I don't know what > >> other users exists out there... > > As far as I know, the only other tool using pasta(1) at the moment is > > rootless-containers (Docker, Usernetes): > > > > https://github.com/rootless-containers/rootlesskit/blob/master/pkg/network/pasta/pasta.go > > > > which is also fine. Other users are developers and people who try out > > network topologies and namespaces stuff without root, but I suppose > > adding the PID at the end is pretty natural anyway. > > > > On the other hand, if we can make sure we avoid this kind of breakage > > at a small cost, why not. I'll try. > > > >> I am not sure if it is worth the risk just to improve the UX for the > >> command use case but I guess you already decided it is otherwise you > >> would have not posted this patch. > > No, not really, I wasn't actually aware of the fact that adding the PID > > before options worked. Thanks for pointing that out. > > Well not just pid, it works the same with a command: > > $ pasta ip addr --config-net > > With this patch this no longer works, as --config-net is now passed to > the ip command. I don't think using it like that makes any sense and it > is super confusing so I like the new way but whoever does use such a > syntax will get broken. Thus it is a trade off to be made. Ah, sure, I see, but that's a completely different level of unreasonableness. I don't think anybody ever dreamt of using it like that. That something I would happily "break". -- Stefano ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] conf: Stop parsing options at first non-option argument 2024-08-08 10:16 ` Stefano Brivio [not found] ` <50d8ea4a-15ec-48a0-bc31-1e2a5139677b@redhat.com> @ 2024-08-08 18:33 ` Stefano Brivio 2024-08-09 0:47 ` David Gibson 1 sibling, 1 reply; 7+ messages in thread From: Stefano Brivio @ 2024-08-08 18:33 UTC (permalink / raw) To: Paul Holzinger; +Cc: passt-dev, David Gibson On Thu, 8 Aug 2024 12:16:58 +0200 Stefano Brivio <sbrivio@redhat.com> wrote: > On Thu, 8 Aug 2024 11:37:38 +0200 > Paul Holzinger <pholzing@redhat.com> wrote: > > > On 08/08/2024 06:02, Stefano Brivio wrote: > > > Given that pasta supports specifying a command to be executed on the > > > command line, even without the usual -- separator as long as there's > > > no ambiguity, we shouldn't eat up options that are not meant for us. > > > > > > Paul reports, for instance, that with: > > > > > > pasta --config-net ip -6 route > > > > > > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. > > > That's because getopt_long(), by default, shuffles the argument list > > > to shift non-option arguments at the end. > > > > > > Avoid that by adding '+' at the beginning of 'optstring'. > > > > > > Reported-by: Paul Holzinger <pholzing@redhat.com> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > > --- > > > v3: Use '+' in optstring and drop first non-option tracking > > > > > > v2: Instead of overriding 'name' in the getopt_long() loop, to force > > > exiting the loop, adjust the exit condition > > > > > > conf.c | 4 ++-- > > > util.c | 2 +- > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > I like this change but I like to point out that this is a breaking > > change for any user that sets options after the main argument, i.e. pid. > > Oh, right, that actually happens to work, even if we never really > supported that, the man page has only this form: > > pasta [OPTION]... PID > > I could go back to v2, and, on top of that, if we find a single > non-option argument that looks like a PID (a number), we would push it > to the end of argv and continue parsing. > > If we find any other number, including one that's after all the other > options but before the presumed PID we just pushed, we'll report error. > > We have anyway the following problem, which we won't make any worse (it > can't be done without an actual file with POSIX shell, Bash only): > > $ 1() { echo a; } > $ pasta 1; echo > Couldn't open user namespace /proc/1/ns/user: Permission denied > > $ pasta echo; 1 > > a > > > I can tell you that this will not effect podman but I don't know what > > other users exists out there... > > As far as I know, the only other tool using pasta(1) at the moment is > rootless-containers (Docker, Usernetes): > > https://github.com/rootless-containers/rootlesskit/blob/master/pkg/network/pasta/pasta.go > > which is also fine. Other users are developers and people who try out > network topologies and namespaces stuff without root, but I suppose > adding the PID at the end is pretty natural anyway. > > On the other hand, if we can make sure we avoid this kind of breakage > at a small cost, why not. I'll try. ...no, not really a small cost. The logic itself is spectacularly complicated, something on the lines of: case 1: if (c->mode != MODE_PASTA) usage(argv[0], stderr, EXIT_FAILURE); /* There can be just one PID in the middle of options */ errno = 0; strtol(argv[optind - 1], NULL, 10); if (errno) { /* Not a PID, stop parsing here */ name = -1; break; } if (!pid_found) { SWAP(argv[optind - 1], argv[argc - 1]); pid_found = true; } if (optind <= argc) optind--; if (optind == argc) name = -1; break; but this would still be acceptable, I guess. However, we have multiple getopt_long() calls, and we would need to either handle the '1' case for each one of them, or modify optstring, or extract, here, some logic from conf_pasta_ns()... which I would really like to avoid. Let me go ahead with v3, which anyway takes care of all the cases that are documented and we actually meant to support. If somebody really needs to insert a PID in the middle of the option list, we'll "fix" this. -- Stefano ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] conf: Stop parsing options at first non-option argument 2024-08-08 18:33 ` Stefano Brivio @ 2024-08-09 0:47 ` David Gibson 0 siblings, 0 replies; 7+ messages in thread From: David Gibson @ 2024-08-09 0:47 UTC (permalink / raw) To: Stefano Brivio; +Cc: Paul Holzinger, passt-dev [-- Attachment #1: Type: text/plain, Size: 4682 bytes --] On Thu, Aug 08, 2024 at 08:33:42PM +0200, Stefano Brivio wrote: > On Thu, 8 Aug 2024 12:16:58 +0200 > Stefano Brivio <sbrivio@redhat.com> wrote: > > > On Thu, 8 Aug 2024 11:37:38 +0200 > > Paul Holzinger <pholzing@redhat.com> wrote: > > > > > On 08/08/2024 06:02, Stefano Brivio wrote: > > > > Given that pasta supports specifying a command to be executed on the > > > > command line, even without the usual -- separator as long as there's > > > > no ambiguity, we shouldn't eat up options that are not meant for us. > > > > > > > > Paul reports, for instance, that with: > > > > > > > > pasta --config-net ip -6 route > > > > > > > > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. > > > > That's because getopt_long(), by default, shuffles the argument list > > > > to shift non-option arguments at the end. > > > > > > > > Avoid that by adding '+' at the beginning of 'optstring'. > > > > > > > > Reported-by: Paul Holzinger <pholzing@redhat.com> > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > > > --- > > > > v3: Use '+' in optstring and drop first non-option tracking > > > > > > > > v2: Instead of overriding 'name' in the getopt_long() loop, to force > > > > exiting the loop, adjust the exit condition > > > > > > > > conf.c | 4 ++-- > > > > util.c | 2 +- > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > I like this change but I like to point out that this is a breaking > > > change for any user that sets options after the main argument, i.e. pid. > > > > Oh, right, that actually happens to work, even if we never really > > supported that, the man page has only this form: > > > > pasta [OPTION]... PID > > > > I could go back to v2, and, on top of that, if we find a single > > non-option argument that looks like a PID (a number), we would push it > > to the end of argv and continue parsing. Please no, this sounds horribly complicated. My experience is that when you try to intuit what the user wants this way, you're more likely to just behave unpredictably in other cases. > > If we find any other number, including one that's after all the other > > options but before the presumed PID we just pushed, we'll report error. > > > > We have anyway the following problem, which we won't make any worse (it > > can't be done without an actual file with POSIX shell, Bash only): > > > > $ 1() { echo a; } > > $ pasta 1; echo > > Couldn't open user namespace /proc/1/ns/user: Permission denied > > > > $ pasta echo; 1 > > > > a > > > > > I can tell you that this will not effect podman but I don't know what > > > other users exists out there... > > > > As far as I know, the only other tool using pasta(1) at the moment is > > rootless-containers (Docker, Usernetes): > > > > https://github.com/rootless-containers/rootlesskit/blob/master/pkg/network/pasta/pasta.go > > > > which is also fine. Other users are developers and people who try out > > network topologies and namespaces stuff without root, but I suppose > > adding the PID at the end is pretty natural anyway. > > > > On the other hand, if we can make sure we avoid this kind of breakage > > at a small cost, why not. I'll try. > > ...no, not really a small cost. The logic itself is spectacularly > complicated, something on the lines of: > > case 1: > if (c->mode != MODE_PASTA) > usage(argv[0], stderr, EXIT_FAILURE); > > /* There can be just one PID in the middle of options */ > errno = 0; > strtol(argv[optind - 1], NULL, 10); > if (errno) { /* Not a PID, stop parsing here */ > name = -1; > break; > } > > if (!pid_found) { > SWAP(argv[optind - 1], argv[argc - 1]); > pid_found = true; > } > > if (optind <= argc) > optind--; > > if (optind == argc) > name = -1; > > break; > > but this would still be acceptable, I guess. However, we have multiple > getopt_long() calls, and we would need to either handle the '1' case > for each one of them, or modify optstring, or extract, here, some logic > from conf_pasta_ns()... which I would really like to avoid. > > Let me go ahead with v3, which anyway takes care of all the cases that > are documented and we actually meant to support. If somebody really > needs to insert a PID in the middle of the option list, we'll "fix" > this. Phew. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-09 3:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-08-08 4:02 [PATCH v3] conf: Stop parsing options at first non-option argument Stefano Brivio 2024-08-08 4:44 ` David Gibson 2024-08-08 9:37 ` Paul Holzinger 2024-08-08 10:16 ` Stefano Brivio [not found] ` <50d8ea4a-15ec-48a0-bc31-1e2a5139677b@redhat.com> 2024-08-08 12:30 ` Stefano Brivio 2024-08-08 18:33 ` Stefano Brivio 2024-08-09 0:47 ` David Gibson
Code repositories for project(s) associated with this public inbox https://passt.top/passt This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for IMAP folder(s).