Re: [PATCH] conf: Accept duplicate and conflicting options, the last one wins

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 19 Jun 2024 11:50:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jun 18, 2024 at 06:17:23PM +0200, Stefano Brivio wrote:
> > In multiple occasions, especially when passt(1) and pasta(1) are used
> > in integrations such as the one with Podman, the ability to override
> > earlier options on the command line with later one would have been
> > convenient.
> > 
> > Recently, to debug a number of issues happening with Podman, I would
> > have liked to ask users to share a debug log by passing --debug as
> > additional option, but pasta refuses --quiet (always passed by Podman)
> > and --debug at the same time.
> > 
> > Finally, somebody took care of this on Podman side at:
> >   https://github.com/containers/common/pull/2052
> > 
> > but still, we'll probably have similar cases, or older versions of
> > Podman around, etc.
> > 
> > I think there's some value in telling users about duplicated or
> > conflicting options, because that might reveal issues in integrations
> > or accidental misconfigurations, but by now I'm fairly convinced that
> > the downsides outweigh this.  
> 
> I tend to agree.
> 
> > Drop checks about duplicate options and mutually exclusive ones. In
> > some cases, we need to also undo a couple of initialisations caused
> > by earlier options, but this looks like a simplification, overall.
> > 
> > Suggested-by: Paul Holzinger <pholzing@redhat.com>
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> [snip]
> > +		case 'd':
> >  			c->debug = 1;
> > +			c->quiet = 0;
> >  			break;
> >  		case 'e':
> > -			if (logfile)
> > -				die("Can't log to both file and stderr");
> > -
> > -			if (c->force_stderr)
> > -				die("Multiple --stderr options given");
> > -
> >  			c->force_stderr = 1;
> > +			logfile = NULL;
> >  			break;  
> 
> I would suggest leaving this one as is for now.  I think the least
> surprising behaviour for -e and -l together would be to log to both
> stderr and the file.  They're not inherently contradictory, it's just
> because of an implementation limitation.

It's not really because of something missing in the implementation,
it's that way because the KubeVirt integration needed to ensure that,
with a log file, we won't print anything to standard error (which was
otherwise giving them issues).

> Theferore, in this case, I
> think it's best to give an error rather than behaving in a surprising
> way.  Later on we can implement being able to log to both at once.

But sure, one might have the reasonable expectation (even though that
would conflict with the description from the man page) that -e and -l
would log to file and stderr at the same time, so I'll drop this part,
and describe this behaviour as an exception in the man page.

> >  		case 'l':
> > -			if (c->force_stderr)
> > -				die("Can't log to both stderr and file");
> > -
> > -			if (logfile)
> > -				die("Multiple --log-file options given");
> > -
> >  			logfile = optarg;
> > +			c->force_stderr = 0;  
> 
> Same here, of course.
> 
> 
> >  			break;
> >  		case 'q':
> > -			if (c->quiet)
> > -				die("Multiple --quiet options given");
> > -
> > -			if (c->debug)
> > -				die("Either --debug or --quiet");
> > -
> >  			c->quiet = 1;
> > +			c->debug = c->trace = 0;
> >  			break;
> >  		case 'f':
> > -			if (c->foreground)
> > -				die("Multiple --foreground options given");
> > -
> >  			c->foreground = 1;
> >  			break;
> >  		case 's':
> > -			if (*c->sock_path)
> > -				die("Multiple --socket options given");
> > -
> >  			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
> >  				       optarg);
> >  			if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
> >  				die("Invalid socket path: %s", optarg);
> >  
> > +			c->fd_tap = -1;
> >  			break;
> >  		case 'F':
> > -			if (c->fd_tap >= 0)
> > -				die("Multiple --fd options given");
> > -
> >  			errno = 0;
> >  			c->fd_tap = strtol(optarg, NULL, 0);  
> 
> I'm a little bit dubious about this one too.  In terms of pure
> semantics, then yes, overriding makes sense.  But -F specifically
> requires the caller to have set up fds in specific slots, so it's
> pretty hard to see a real situation where overriding this would make
> sense.

Sure, I don't see that either, but for the sake of keeping the
documentation simple, I would still force this.

> > @@ -1460,12 +1421,9 @@ void conf(struct ctx *c, int argc, char **argv)
> >  				die("Invalid --fd: %s", optarg);
> >  
> >  			c->one_off = true;
> > -
> > +			*c->sock_path = 0;  
> 
> This makes sense, although we weren't actually giving an error for
> that case previously.

We did, but later:

	if (*c->sock_path && c->fd_tap >= 0)
		die("Options --socket and --fd are mutually exclusive");

as I'm dropping this now, we need to make sure that c->sock_path is
cleared.

> >  			break;
> >  		case 'I':
> > -			if (*c->pasta_ifn)
> > -				die("Multiple --ns-ifname options given");
> > -
> >  			ret = snprintf(c->pasta_ifn, IFNAMSIZ, "%s",
> >  				       optarg);
> >  			if (ret <= 0 || ret >= IFNAMSIZ)
> > @@ -1473,18 +1431,12 @@ void conf(struct ctx *c, int argc, char **argv)
> >  
> >  			break;
> >  		case 'p':
> > -			if (*c->pcap)
> > -				die("Multiple --pcap options given");
> > -  
> 
> Again, I'm unsure about this one, since I think the least surprising
> behaviour would be to write to _all_ the listed pcap files.

...but we clearly don't want to implement that. And if we give an error
here, we should also specifically document that multiple --pcap options
are not allowed, at this point, which just looks like unnecessary bloat
in the man page to me.

> >  			ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg);
> >  			if (ret <= 0 || ret >= (int)sizeof(c->pcap))
> >  				die("Invalid pcap path: %s", optarg);
> >  
> >  			break;
> >  		case 'P':
> > -			if (*c->pidfile)
> > -				die("Multiple --pid options given");
> > -
> >  			ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s",
> >  				       optarg);
> >  			if (ret <= 0 || ret >= (int)sizeof(c->pidfile))
> > @@ -1492,9 +1444,6 @@ void conf(struct ctx *c, int argc, char **argv)
> >  
> >  			break;
> >  		case 'm':
> > -			if (c->mtu)
> > -				die("Multiple --mtu options given");
> > -
> >  			errno = 0;
> >  			c->mtu = strtol(optarg, NULL, 0);
> >  
> > @@ -1544,14 +1493,12 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			if (c->mode == MODE_PASTA)
> >  				c->no_copy_routes = 1;
> >  
> > -			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw)		&&
> > -			    inet_pton(AF_INET6, optarg, &c->ip6.gw)	&&
> > +			if (inet_pton(AF_INET6, optarg, &c->ip6.gw)     &&
> >  			    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw)	&&
> >  			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.gw))
> >  				break;
> >  
> > -			if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw)		&&
> > -			    inet_pton(AF_INET, optarg, &c->ip4.gw)	&&
> > +			if (inet_pton(AF_INET, optarg, &c->ip4.gw)	&&
> >  			    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw)	&&
> >  			    !IN4_IS_ADDR_BROADCAST(&c->ip4.gw)		&&
> >  			    !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw))
> > @@ -1560,15 +1507,11 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			die("Invalid gateway address: %s", optarg);
> >  			break;
> >  		case 'i':
> > -			if (ifi4 || ifi6)
> > -				die("Redundant interface: %s", optarg);
> > -
> >  			if (!(ifi4 = ifi6 = if_nametoindex(optarg)))
> >  				die_perror("Invalid interface name %s", optarg);
> >  			break;
> >  		case 'o':
> > -			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out)	  &&
> > -			    inet_pton(AF_INET6, optarg, &c->ip6.addr_out) &&
> > +			if (inet_pton(AF_INET6, optarg, &c->ip6.addr_out) &&
> >  			    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out)	  &&
> >  			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.addr_out)	  &&
> >  			    !IN6_IS_ADDR_V4MAPPED(&c->ip6.addr_out)	  &&
> > @@ -1576,8 +1519,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			    !IN6_IS_ADDR_MULTICAST(&c->ip6.addr_out))
> >  				break;
> >  
> > -			if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out)	 &&
> > -			    inet_pton(AF_INET, optarg, &c->ip4.addr_out) &&
> > +			if (inet_pton(AF_INET, optarg, &c->ip4.addr_out) &&
> >  			    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out)	 &&
> >  			    !IN4_IS_ADDR_BROADCAST(&c->ip4.addr_out)	 &&
> >  			    !IN4_IS_ADDR_MULTICAST(&c->ip4.addr_out))
> > @@ -1588,18 +1530,23 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			break;
> >  		case 'D':
> >  			if (!strcmp(optarg, "none")) {
> > -				if (c->no_dns)
> > -					die("Redundant DNS options");
> > +				c->no_dns = 1;
> >  
> > -				if (dns4 - c->ip4.dns || dns6 - c->ip6.dns)
> > -					die("Conflicting DNS options");
> > +				dns4 = &c->ip4.dns[0];
> > +				memset(c->ip4.dns, 0, sizeof(c->ip4.dns));
> > +				c->ip4.dns[0]    = (struct in_addr){ 0 };
> > +				c->ip4.dns_match = (struct in_addr){ 0 };
> > +				c->ip4.dns_host  = (struct in_addr){ 0 };
> > +
> > +				dns6 = &c->ip6.dns[0];
> > +				memset(c->ip6.dns, 0, sizeof(c->ip6.dns));
> > +				c->ip6.dns_match = (struct in6_addr){ 0 };
> > +				c->ip6.dns_host  = (struct in6_addr){ 0 };
> >  
> > -				c->no_dns = 1;
> >  				break;
> >  			}
> >  
> > -			if (c->no_dns)
> > -				die("Conflicting DNS options");
> > +			c->no_dns = 0;
> >  
> >  			if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) &&
> >  			    inet_pton(AF_INET, optarg, &dns4_tmp)) {
> > @@ -1617,18 +1564,14 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			break;
> >  		case 'S':
> >  			if (!strcmp(optarg, "none")) {
> > -				if (c->no_dns_search)
> > -					die("Redundant DNS search options");
> > +				c->no_dns_search = 1;
> >  
> > -				if (dnss != c->dns_search)
> > -					die("Conflicting DNS search options");
> > +				memset(c->dns_search, 0, sizeof(c->dns_search));
> >  
> > -				c->no_dns_search = 1;
> >  				break;
> >  			}
> >  
> > -			if (c->no_dns_search)
> > -				die("Conflicting DNS search options");
> > +			c->no_dns_search = 0;
> >  
> >  			if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) {
> >  				ret = snprintf(dnss->n, sizeof(*c->dns_search),
> > @@ -1644,17 +1587,16 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			break;
> >  		case '4':
> >  			v4_only = true;
> > +			v6_only = false;
> >  			break;
> >  		case '6':
> >  			v6_only = true;
> > +			v4_only = false;
> >  			break;
> >  		case '1':
> >  			if (c->mode == MODE_PASTA)
> >  				die("--one-off is for passt mode only");
> >  
> > -			if (c->one_off)
> > -				die("Redundant --one-off option");
> > -
> >  			c->one_off = true;
> >  			break;
> >  		case 't':
> > @@ -1673,12 +1615,6 @@ void conf(struct ctx *c, int argc, char **argv)
> >  		}
> >  	} while (name != -1);
> >  
> > -	if (v4_only && v6_only)
> > -		die("Options ipv4-only and ipv6-only are mutually exclusive");  
> 
> This could now be an ASSERT() instead of an error message.

But it can't happen, right? I mean, it's very clear that when we set
one, we clear the other one (hunk above).

> > -	if (*c->sock_path && c->fd_tap >= 0)
> > -		die("Options --socket and --fd are mutually exclusive");
> > -
> >  	if (c->mode == MODE_PASTA && !c->pasta_conf_ns) {
> >  		if (copy_routes_opt)
> >  			die("--no-copy-routes needs --config-net");
> > diff --git a/passt.1 b/passt.1
> > index 31e528e..6dfa670 100644
> > --- a/passt.1
> > +++ b/passt.1
> > @@ -73,6 +73,8 @@ for performance reasons.
> >  
> >  .SH OPTIONS
> >  
> > +\fBIf conflicting or multiple options are given, the last one takes effect.\fR
> > +
> >  .TP
> >  .BR \-d ", " \-\-debug
> >  Be verbose, don't log to the system logger.  
> 

-- 
Stefano