Option parsing: Allow the same option multiple times

Option parsing: Allow the same option multiple times

[Paul Holzinger]

Hi all,

following up on a quick discussion with Stefano on IRC.

passt/pasta currently rejects most (not all) options when specified 
multiple times, i.e. pasta -I eth0 -I eth1 ... fails. I think it makes 
more sense to just use the last one instead.

My use case: In podman I added a new containers.conf option[1] which 
allows users to set default pasta cli options. However users can also 
add options on the podman cli with podman run --net=pasta:... For me it 
would make the most sense to just append those to the config options and 
then let pasta deal with it. This allows some form of overwrite 
mechanism, i.e. by default I may have "-I" , "eth0" in containers.conf 
but for one specific container I want to use a different interface name 
and set --net=pasta:-I,eth1 on the cli. Then podman should just hand "-I 
eth0 -I eth1" to pasta and then pasta picks the last one.

If we keep the current behavior it means I am forced to parse the 
options in podman and dedup them which is hard to maintain as podman 
would need to keep up with pasta upstream.

I am willing to send a patch to change this so please let me know if 
anyone would object to that.


[1] 
https://github.com/containers/common/commit/da3ac5d6d025f2933e655a0541eddbcf26070583

Thanks, Paul

Re: Option parsing: Allow the same option multiple times

[Stefano Brivio]

On Thu, 15 Jun 2023 14:57:37 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> Hi all,
> 
> following up on a quick discussion with Stefano on IRC.
> 
> passt/pasta currently rejects most (not all) options when specified 
> multiple times, i.e. pasta -I eth0 -I eth1 ... fails. I think it makes 
> more sense to just use the last one instead.
> 
> My use case: In podman I added a new containers.conf option[1] which 
> allows users to set default pasta cli options. However users can also 
> add options on the podman cli with podman run --net=pasta:... For me it 
> would make the most sense to just append those to the config options and 
> then let pasta deal with it. This allows some form of overwrite 
> mechanism, i.e. by default I may have "-I" , "eth0" in containers.conf 
> but for one specific container I want to use a different interface name 
> and set --net=pasta:-I,eth1 on the cli. Then podman should just hand "-I 
> eth0 -I eth1" to pasta and then pasta picks the last one.
> 
> If we keep the current behavior it means I am forced to parse the 
> options in podman and dedup them which is hard to maintain as podman 
> would need to keep up with pasta upstream.

I had to do something similar in libpod/networking_pasta_linux.go,
which, if you change this, could also be simplified a bit.

> I am willing to send a patch to change this so please let me know if 
> anyone would object to that.

For the record, as I mentioned on IRC, I think it makes sense.

Maybe it's less "correct" as a behaviour, and it would make it a bit
harder for users to spot (unlikely) mistakes on the command line, but
making integrations simpler probably outweighs this.

I don't have a good idea for sentences like "This option can be
specified zero (for defaults) to two times (once for IPv4, once for IPv6)."
that are currently in the man page... maybe we could switch from
"This option can be specified" to "This option specifies one to two..."
and similar.

-- 
Stefano

Re: Option parsing: Allow the same option multiple times

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]

On Fri, Jun 16, 2023 at 11:04:00PM +0200, Stefano Brivio wrote:
> On Thu, 15 Jun 2023 14:57:37 +0200
> Paul Holzinger <pholzing@redhat.com> wrote:
> 
> > Hi all,
> > 
> > following up on a quick discussion with Stefano on IRC.
> > 
> > passt/pasta currently rejects most (not all) options when specified 
> > multiple times, i.e. pasta -I eth0 -I eth1 ... fails. I think it makes 
> > more sense to just use the last one instead.
> > 
> > My use case: In podman I added a new containers.conf option[1] which 
> > allows users to set default pasta cli options. However users can also 
> > add options on the podman cli with podman run --net=pasta:... For me it 
> > would make the most sense to just append those to the config options and 
> > then let pasta deal with it. This allows some form of overwrite 
> > mechanism, i.e. by default I may have "-I" , "eth0" in containers.conf 
> > but for one specific container I want to use a different interface name 
> > and set --net=pasta:-I,eth1 on the cli. Then podman should just hand "-I 
> > eth0 -I eth1" to pasta and then pasta picks the last one.
> > 
> > If we keep the current behavior it means I am forced to parse the 
> > options in podman and dedup them which is hard to maintain as podman 
> > would need to keep up with pasta upstream.
> 
> I had to do something similar in libpod/networking_pasta_linux.go,
> which, if you change this, could also be simplified a bit.
> 
> > I am willing to send a patch to change this so please let me know if 
> > anyone would object to that.
> 
> For the record, as I mentioned on IRC, I think it makes sense.
> 
> Maybe it's less "correct" as a behaviour, and it would make it a bit
> harder for users to spot (unlikely) mistakes on the command line, but
> making integrations simpler probably outweighs this.
> 
> I don't have a good idea for sentences like "This option can be
> specified zero (for defaults) to two times (once for IPv4, once for IPv6)."
> that are currently in the man page... maybe we could switch from
> "This option can be specified" to "This option specifies one to two..."
> and similar.

So, I'm actually a bit hesitant about applying this treatment (allow
multiple, last one wins) to -a specifically.  The reason being that we
have draft plans to allow multiple addresses within the guest/ns.
That might logically lead to allowing arbitrary numbers of -a options
in future, where *all* the addresses apply.  If we'd previously
allowed multiple options, but only the last one applies, that might be
a breaking semantic change.

-- 
David Gibson			| 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 --]

Re: Option parsing: Allow the same option multiple times

[Stefano Brivio]

On Mon, 19 Jun 2023 14:49:42 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 16, 2023 at 11:04:00PM +0200, Stefano Brivio wrote:
> > On Thu, 15 Jun 2023 14:57:37 +0200
> > Paul Holzinger <pholzing@redhat.com> wrote:
> > 
> > > Hi all,
> > > 
> > > following up on a quick discussion with Stefano on IRC.
> > > 
> > > passt/pasta currently rejects most (not all) options when specified 
> > > multiple times, i.e. pasta -I eth0 -I eth1 ... fails. I think it makes 
> > > more sense to just use the last one instead.
> > > 
> > > My use case: In podman I added a new containers.conf option[1] which 
> > > allows users to set default pasta cli options. However users can also 
> > > add options on the podman cli with podman run --net=pasta:... For me it 
> > > would make the most sense to just append those to the config options and 
> > > then let pasta deal with it. This allows some form of overwrite 
> > > mechanism, i.e. by default I may have "-I" , "eth0" in containers.conf 
> > > but for one specific container I want to use a different interface name 
> > > and set --net=pasta:-I,eth1 on the cli. Then podman should just hand "-I 
> > > eth0 -I eth1" to pasta and then pasta picks the last one.
> > > 
> > > If we keep the current behavior it means I am forced to parse the 
> > > options in podman and dedup them which is hard to maintain as podman 
> > > would need to keep up with pasta upstream.
> > 
> > I had to do something similar in libpod/networking_pasta_linux.go,
> > which, if you change this, could also be simplified a bit.
> > 
> > > I am willing to send a patch to change this so please let me know if 
> > > anyone would object to that.
> > 
> > For the record, as I mentioned on IRC, I think it makes sense.
> > 
> > Maybe it's less "correct" as a behaviour, and it would make it a bit
> > harder for users to spot (unlikely) mistakes on the command line, but
> > making integrations simpler probably outweighs this.
> > 
> > I don't have a good idea for sentences like "This option can be
> > specified zero (for defaults) to two times (once for IPv4, once for IPv6)."
> > that are currently in the man page... maybe we could switch from
> > "This option can be specified" to "This option specifies one to two..."
> > and similar.
> 
> So, I'm actually a bit hesitant about applying this treatment (allow
> multiple, last one wins) to -a specifically.  The reason being that we
> have draft plans to allow multiple addresses within the guest/ns.
> That might logically lead to allowing arbitrary numbers of -a options
> in future, where *all* the addresses apply.  If we'd previously
> allowed multiple options, but only the last one applies, that might be
> a breaking semantic change.

Uh oh, right... and it might also apply to -g / --gateway eventually.

On the other hand: are two semantic changes (both of which not breaking
any reasonable or expected previous usage) in a relatively short period
of time so much worse than one (we plan to change -a handling anyway)?

I guess handling -a differently in Podman's options would still be
additional effort for Paul. In my opinion, as long as we keep man pages
updated, we should still prioritise making our lives (and the code!)
easier.

-- 
Stefano

Re: Option parsing: Allow the same option multiple times

[Paul Holzinger]

On 19/06/2023 08:18, Stefano Brivio wrote:
> On Mon, 19 Jun 2023 14:49:42 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
>> On Fri, Jun 16, 2023 at 11:04:00PM +0200, Stefano Brivio wrote:
>>> On Thu, 15 Jun 2023 14:57:37 +0200
>>> Paul Holzinger <pholzing@redhat.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> following up on a quick discussion with Stefano on IRC.
>>>>
>>>> passt/pasta currently rejects most (not all) options when specified
>>>> multiple times, i.e. pasta -I eth0 -I eth1 ... fails. I think it makes
>>>> more sense to just use the last one instead.
>>>>
>>>> My use case: In podman I added a new containers.conf option[1] which
>>>> allows users to set default pasta cli options. However users can also
>>>> add options on the podman cli with podman run --net=pasta:... For me it
>>>> would make the most sense to just append those to the config options and
>>>> then let pasta deal with it. This allows some form of overwrite
>>>> mechanism, i.e. by default I may have "-I" , "eth0" in containers.conf
>>>> but for one specific container I want to use a different interface name
>>>> and set --net=pasta:-I,eth1 on the cli. Then podman should just hand "-I
>>>> eth0 -I eth1" to pasta and then pasta picks the last one.
>>>>
>>>> If we keep the current behavior it means I am forced to parse the
>>>> options in podman and dedup them which is hard to maintain as podman
>>>> would need to keep up with pasta upstream.
>>> I had to do something similar in libpod/networking_pasta_linux.go,
>>> which, if you change this, could also be simplified a bit.
>>>
>>>> I am willing to send a patch to change this so please let me know if
>>>> anyone would object to that.
>>> For the record, as I mentioned on IRC, I think it makes sense.
>>>
>>> Maybe it's less "correct" as a behaviour, and it would make it a bit
>>> harder for users to spot (unlikely) mistakes on the command line, but
>>> making integrations simpler probably outweighs this.
>>>
>>> I don't have a good idea for sentences like "This option can be
>>> specified zero (for defaults) to two times (once for IPv4, once for IPv6)."
>>> that are currently in the man page... maybe we could switch from
>>> "This option can be specified" to "This option specifies one to two..."
>>> and similar.
>> So, I'm actually a bit hesitant about applying this treatment (allow
>> multiple, last one wins) to -a specifically.  The reason being that we
>> have draft plans to allow multiple addresses within the guest/ns.
>> That might logically lead to allowing arbitrary numbers of -a options
>> in future, where *all* the addresses apply.  If we'd previously
>> allowed multiple options, but only the last one applies, that might be
>> a breaking semantic change.
> Uh oh, right... and it might also apply to -g / --gateway eventually.
>
> On the other hand: are two semantic changes (both of which not breaking
> any reasonable or expected previous usage) in a relatively short period
> of time so much worse than one (we plan to change -a handling anyway)?
>
> I guess handling -a differently in Podman's options would still be
> additional effort for Paul. In my opinion, as long as we keep man pages
> updated, we should still prioritise making our lives (and the code!)
> easier.
>
If you plan to make changes like that I agree that supporting it might 
result in possible breaking changes for some users.

However I like to point out that the current behavior is inconsistent 
and only works for some options which is the reason I thought this would 
work in the first place, namely it works fine with `--ns-mac-addr` right 
now.


Another option for Podman would be to discard the options from the conf 
file when cli options are set so it is either or. Easy to do but I think 
it is not so great for users as this means a user would need to type all 
options again if they just want to change a single one.

Re: Option parsing: Allow the same option multiple times

[Stefano Brivio]

On Tue, 20 Jun 2023 16:50:49 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> On 19/06/2023 08:18, Stefano Brivio wrote:
> > On Mon, 19 Jun 2023 14:49:42 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >  
> >> On Fri, Jun 16, 2023 at 11:04:00PM +0200, Stefano Brivio wrote:  
> >>> On Thu, 15 Jun 2023 14:57:37 +0200
> >>> Paul Holzinger <pholzing@redhat.com> wrote:
> >>>  
> >>>> Hi all,
> >>>>
> >>>> following up on a quick discussion with Stefano on IRC.
> >>>>
> >>>> passt/pasta currently rejects most (not all) options when specified
> >>>> multiple times, i.e. pasta -I eth0 -I eth1 ... fails. I think it makes
> >>>> more sense to just use the last one instead.
> >>>>
> >>>> My use case: In podman I added a new containers.conf option[1] which
> >>>> allows users to set default pasta cli options. However users can also
> >>>> add options on the podman cli with podman run --net=pasta:... For me it
> >>>> would make the most sense to just append those to the config options and
> >>>> then let pasta deal with it. This allows some form of overwrite
> >>>> mechanism, i.e. by default I may have "-I" , "eth0" in containers.conf
> >>>> but for one specific container I want to use a different interface name
> >>>> and set --net=pasta:-I,eth1 on the cli. Then podman should just hand "-I
> >>>> eth0 -I eth1" to pasta and then pasta picks the last one.
> >>>>
> >>>> If we keep the current behavior it means I am forced to parse the
> >>>> options in podman and dedup them which is hard to maintain as podman
> >>>> would need to keep up with pasta upstream.  
> >>> I had to do something similar in libpod/networking_pasta_linux.go,
> >>> which, if you change this, could also be simplified a bit.
> >>>  
> >>>> I am willing to send a patch to change this so please let me know if
> >>>> anyone would object to that.  
> >>> For the record, as I mentioned on IRC, I think it makes sense.
> >>>
> >>> Maybe it's less "correct" as a behaviour, and it would make it a bit
> >>> harder for users to spot (unlikely) mistakes on the command line, but
> >>> making integrations simpler probably outweighs this.
> >>>
> >>> I don't have a good idea for sentences like "This option can be
> >>> specified zero (for defaults) to two times (once for IPv4, once for IPv6)."
> >>> that are currently in the man page... maybe we could switch from
> >>> "This option can be specified" to "This option specifies one to two..."
> >>> and similar.  
> >> So, I'm actually a bit hesitant about applying this treatment (allow
> >> multiple, last one wins) to -a specifically.  The reason being that we
> >> have draft plans to allow multiple addresses within the guest/ns.
> >> That might logically lead to allowing arbitrary numbers of -a options
> >> in future, where *all* the addresses apply.  If we'd previously
> >> allowed multiple options, but only the last one applies, that might be
> >> a breaking semantic change.  
> > Uh oh, right... and it might also apply to -g / --gateway eventually.
> >
> > On the other hand: are two semantic changes (both of which not breaking
> > any reasonable or expected previous usage) in a relatively short period
> > of time so much worse than one (we plan to change -a handling anyway)?
> >
> > I guess handling -a differently in Podman's options would still be
> > additional effort for Paul. In my opinion, as long as we keep man pages
> > updated, we should still prioritise making our lives (and the code!)
> > easier.
>
> If you plan to make changes like that I agree that supporting it might 
> result in possible breaking changes for some users.

Yes, but for how long...? I think we can really prefer a simple
solution (as long as man pages are updated) and accept that we'll
(slightly) change the behaviour twice.

It's not even the expected usage, it's about going through:

1. reporting errors with multiple options (logically invalid)

2. not reporting errors with multiple options (logically invalid, some
   useless)

3. not reporting errors with multiple options (logically valid, all
   useful)

if somebody is in case 2., their configuration (that we'll be "fixing"
all of a sudden) is kind of problematic anyway.

> However I like to point out that the current behavior is inconsistent 
> and only works for some options which is the reason I thought this would 
> work in the first place, namely it works fine with `--ns-mac-addr` right 
> now.

Right, also that.

> Another option for Podman would be to discard the options from the conf 
> file when cli options are set so it is either or. Easy to do but I think 
> it is not so great for users as this means a user would need to type all 
> options again if they just want to change a single one.

To me that looks much more confusing than any slight "breakage" we
might have as a result.

Nah, really, let's not overcomplicate this. I think the biggest problem
for users at the moment is that we all struggle to find the time to
implement "nice" things they would be eager to use.

I'd suggest to just go ahead with what you were proposing at the
beginning. I also talked with David on the weekly call yesterday and
he's fine with this.

-- 
Stefano