Re: [PATCH v3] conf: Stop parsing options at first non-option argument

[Stefano Brivio <sbrivio@redhat.com>]

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