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

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

[Stefano Brivio]

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.

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>
---
 conf.c  | 140 +++++++++++++++-----------------------------------------
 passt.1 |   2 +
 2 files changed, 40 insertions(+), 102 deletions(-)

diff --git a/conf.c b/conf.c
index 2a6f05c..75b46c0 100644
--- a/conf.c
+++ b/conf.c
@@ -517,9 +517,6 @@ static void conf_netns_opt(char *netns, const char *arg)
 static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
 			  int optind, int argc, char *argv[])
 {
-	if (*netns_only && *userns)
-		die("Both --userns and --netns-only given");
-
 	if (*netns && optind != argc)
 		die("Both --netns and PID or command given");
 
@@ -1206,7 +1203,6 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"udp-ns",	required_argument,	NULL,		'U' },
 		{"userns",	required_argument,	NULL,		2 },
 		{"netns",	required_argument,	NULL,		3 },
-		{"netns-only",	no_argument,		&netns_only,	1 },
 		{"ns-mac-addr",	required_argument,	NULL,		4 },
 		{"dhcp-dns",	no_argument,		NULL,		5 },
 		{"no-dhcp-dns",	no_argument,		NULL,		6 },
@@ -1223,6 +1219,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"config-net",	no_argument,		NULL,		17 },
 		{"no-copy-routes", no_argument,		NULL,		18 },
 		{"no-copy-addrs", no_argument,		NULL,		19 },
+		{"netns-only",	no_argument,		NULL,		20 },
 		{ 0 },
 	};
 	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
@@ -1267,6 +1264,8 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (ret <= 0 || ret >= (int)sizeof(userns))
 				die("Invalid userns: %s", optarg);
 
+			netns_only = 0;
+
 			break;
 		case 3:
 			if (c->mode != MODE_PASTA)
@@ -1327,24 +1326,13 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->no_netns_quit = 1;
 			break;
 		case 11:
-			if (c->trace)
-				die("Multiple --trace options given");
-
-			if (c->quiet)
-				die("Either --trace or --quiet");
-
 			c->trace = c->debug = 1;
+			c->quiet = 0;
 			break;
 		case 12:
-			if (runas)
-				die("Multiple --runas options given");
-
 			runas = optarg;
 			break;
 		case 13:
-			if (logsize)
-				die("Multiple --log-size options given");
-
 			errno = 0;
 			logsize = strtol(optarg, NULL, 0);
 
@@ -1358,9 +1346,6 @@ void conf(struct ctx *c, int argc, char **argv)
 			fprintf(stdout, VERSION_BLOB);
 			exit(EXIT_SUCCESS);
 		case 15:
-			if (*c->ip4.ifname_out)
-				die("Redundant outbound interface: %s", optarg);
-
 			ret = snprintf(c->ip4.ifname_out,
 				       sizeof(c->ip4.ifname_out), "%s", optarg);
 			if (ret <= 0 || ret >= (int)sizeof(c->ip4.ifname_out))
@@ -1368,9 +1353,6 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			break;
 		case 16:
-			if (*c->ip6.ifname_out)
-				die("Redundant outbound interface: %s", optarg);
-
 			ret = snprintf(c->ip6.ifname_out,
 				       sizeof(c->ip6.ifname_out), "%s", optarg);
 			if (ret <= 0 || ret >= (int)sizeof(c->ip6.ifname_out))
@@ -1397,62 +1379,41 @@ void conf(struct ctx *c, int argc, char **argv)
 			warn("--no-copy-addrs will be dropped soon");
 			c->no_copy_addrs = copy_addrs_opt = true;
 			break;
-		case 'd':
-			if (c->debug)
-				die("Multiple --debug options given");
-
-			if (c->quiet)
-				die("Either --debug or --quiet");
+		case 20:
+			if (c->mode != MODE_PASTA)
+				die("--netns-only is for pasta mode only");
 
+			netns_only = 1;
+			*userns = 0;
+			break;
+		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;
 		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;
 			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);
 
@@ -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;
 			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");
-
 			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");
-
-	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.
-- 
@@ -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.
-- 
2.43.0

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

[David Gibson]

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

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.  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.

>  		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.

> @@ -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.

>  			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.

>  			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.

> -	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.

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

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

[Stefano Brivio]

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

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

[Paul Holzinger]

On 18/06/2024 18:17, 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.
To give more context: Podman allows to specify pasta options in the 
containers.conf file and on the cli. In podman we just append config 
file + cli and pass all options to pasta. As such it would be very 
difficult for users to overwrite a single option from the config file as 
pasta errors without this patch. The alternative is for Podman to parse 
the cli args and fix them before handing them to pasta but this is 
clearly not maintainable on our end.
>
> 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.
>
> 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>
PS: The patch doesn't apply to current HEAD on https://passt.top/passt/ 
(should I use another tree?) and git doesn't offer any merge conflict 
resolution for applying patches either??? So I cannot test this locally.

-- 
Paul Holzinger

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

[Stefano Brivio]

On Wed, 19 Jun 2024 11:06:37 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> On 18/06/2024 18:17, 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.  
> To give more context: Podman allows to specify pasta options in the 
> containers.conf file and on the cli. In podman we just append config 
> file + cli and pass all options to pasta. As such it would be very 
> difficult for users to overwrite a single option from the config file as 
> pasta errors without this patch. The alternative is for Podman to parse 
> the cli args and fix them before handing them to pasta but this is 
> clearly not maintainable on our end.

Right, I forgot to explain, I'll add this in v2 (recycling from your
paragraph).

> > 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.
> >
> > 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>  
>
> PS: The patch doesn't apply to current HEAD on https://passt.top/passt/ 
> (should I use another tree?) and git doesn't offer any merge conflict 
> resolution for applying patches either??? So I cannot test this locally.

My bad, I based this patch on other patches I had just posted, I'll
rebase this on HEAD for v2.

If you want to apply it anyway, git's three-way merge fixes this up,
given that there are no actual conflicts:

  $ curl https://archives.passt.top/passt-dev/20240618161723.1896519-1-sbrivio@redhat.com/raw | git am -3
    % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                   Dload  Upload   Total   Spent    Left  Speed
  100 12963    0 12963    0     0  37441      0 --:--:-- --:--:-- --:--:-- 37465
  Applying: conf: Accept duplicate and conflicting options, the last one wins
  Using index info to reconstruct a base tree...
  M	conf.c
  M	passt.1
  Falling back to patching base and 3-way merge...
  Auto-merging passt.1
  Auto-merging conf.c

-- 
Stefano

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

[David Gibson]

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

On Wed, Jun 19, 2024 at 09:45:30AM +0200, Stefano Brivio wrote:
> 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).

Hmmmmm...  I mean, I get that we generally want to make things easy
for KubeVirt.  But I don't think we want to implement otherwise more
surprising behaviour, just so they can avoid a '2> /dev/null'.

> > 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.

Faor enough.

> > > @@ -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.

Good point.

> > >  			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.

I suppose.

> > >  			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.  
> > 
> 

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