public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: Paul Holzinger <pholzing@redhat.com>
Cc: passt-dev@passt.top, David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v3] conf: Stop parsing options at first non-option argument
Date: Thu, 8 Aug 2024 12:16:58 +0200	[thread overview]
Message-ID: <20240808121658.6574ef4c@elisabeth> (raw)
In-Reply-To: <f27604a3-9244-4adb-bc69-ae057c1b3eea@redhat.com>

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


  reply	other threads:[~2024-08-08 10:17 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
     [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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240808121658.6574ef4c@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    --cc=pholzing@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).