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

[PATCH v2] 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.

On top of this, Podman lets users specify other pasta options in its
containers.conf(5) file, as well as on the command line.

The options from the configuration files are appended together with
the ones from the command line, which makes it impossible for users to
override options from the configuration file, if duplicated options
are refused, unless Podman takes care of sorting them, which is
clearly not sustainable.

For --debug and --trace, somebody took care of this on Podman side at:
  https://github.com/containers/common/pull/2052

but this doesn't fix the issue with other options, and we'll have
anyway older versions of Podman around, too.

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.

Notable exception: --stderr still conflicts with --log-file, because
users might have the expectation that they don't actually conflict.
But they do conflict in the existing implementation, so it's safer
to make sure that the users notice that.

Suggested-by: Paul Holzinger <pholzing@redhat.com>
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2:
  - report --log-file and --stderr as conflicting, but accept
    multiple options if they don't conflict
  - for --log-file and --pcap, make it clear in the man page that
    the general principle still applies: only the last one (of each)
    takes effect
  - add some more context about Podman and how the list of pasta
    options is built there, in the commit message

 conf.c  | 134 ++++++++++++++++----------------------------------------
 passt.1 |  17 +++++++
 2 files changed, 55 insertions(+), 96 deletions(-)

diff --git a/conf.c b/conf.c
index 94b3ed6..c5f13ac 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");
 
@@ -1204,7 +1201,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 },
@@ -1221,6 +1217,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 };
@@ -1265,6 +1262,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)
@@ -1325,24 +1324,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);
 
@@ -1356,9 +1344,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))
@@ -1366,9 +1351,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))
@@ -1395,62 +1377,47 @@ 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);
 
@@ -1458,12 +1425,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)
@@ -1471,18 +1435,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))
@@ -1490,9 +1448,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);
 
@@ -1542,14 +1497,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))
@@ -1558,16 +1511,12 @@ 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("Invalid interface name %s: %s", optarg,
 				    strerror(errno));
 			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)	  &&
@@ -1575,8 +1524,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))
@@ -1587,18 +1535,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)) {
@@ -1616,18 +1569,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),
@@ -1643,17 +1592,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':
@@ -1672,12 +1620,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 7676fe3..7b9915b 100644
--- a/passt.1
+++ b/passt.1
@@ -73,6 +73,9 @@ for performance reasons.
 
 .SH OPTIONS
 
+Unless otherwise noted below, \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.
@@ -97,10 +100,21 @@ Log to standard error too.
 Default is to log to the system logger only, if started from an interactive
 terminal, and to both system logger and standard error otherwise.
 
+This option cannot be specified together with \fB--log-file\fR, because there
+might be a reasonable expectation that messages are logged to standard error as
+well as to a file, which is however not supported.
+
 .TP
 .BR \-l ", " \-\-log-file " " \fIPATH\fR
 Log to file \fIPATH\fR, not to standard error, and not to the system logger.
 
+This option cannot be specified together with \fB--stderr\fR, because there
+might be a reasonable expectation that messages are logged to a file as well as
+to standard error, which is however not supported.
+
+Specifying this option multiple times does \fInot\fR lead to multiple log files:
+the last given option takes effect.
+
 .TP
 .BR \-\-log-size " " \fISIZE\fR
 Limit log file size to \fISIZE\fR bytes. When the log file is full, make room
@@ -128,6 +142,9 @@ Show version and exit.
 Capture tap-facing (that is, guest-side or namespace-side) network packets to
 \fIfile\fR in \fBpcap\fR format.
 
+Specifying this option multiple times does \fInot\fR lead to multiple capture
+files: the last given option takes effect.
+
 .TP
 .BR \-P ", " \-\-pid " " \fIfile
 Write own PID to \fIfile\fR once initialisation is done, before forking to
-- 
@@ -73,6 +73,9 @@ for performance reasons.
 
 .SH OPTIONS
 
+Unless otherwise noted below, \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.
@@ -97,10 +100,21 @@ Log to standard error too.
 Default is to log to the system logger only, if started from an interactive
 terminal, and to both system logger and standard error otherwise.
 
+This option cannot be specified together with \fB--log-file\fR, because there
+might be a reasonable expectation that messages are logged to standard error as
+well as to a file, which is however not supported.
+
 .TP
 .BR \-l ", " \-\-log-file " " \fIPATH\fR
 Log to file \fIPATH\fR, not to standard error, and not to the system logger.
 
+This option cannot be specified together with \fB--stderr\fR, because there
+might be a reasonable expectation that messages are logged to a file as well as
+to standard error, which is however not supported.
+
+Specifying this option multiple times does \fInot\fR lead to multiple log files:
+the last given option takes effect.
+
 .TP
 .BR \-\-log-size " " \fISIZE\fR
 Limit log file size to \fISIZE\fR bytes. When the log file is full, make room
@@ -128,6 +142,9 @@ Show version and exit.
 Capture tap-facing (that is, guest-side or namespace-side) network packets to
 \fIfile\fR in \fBpcap\fR format.
 
+Specifying this option multiple times does \fInot\fR lead to multiple capture
+files: the last given option takes effect.
+
 .TP
 .BR \-P ", " \-\-pid " " \fIfile
 Write own PID to \fIfile\fR once initialisation is done, before forking to
-- 
2.43.0

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

[David Gibson]

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

On Wed, Jun 19, 2024 at 05:40:19PM +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.
> 
> On top of this, Podman lets users specify other pasta options in its
> containers.conf(5) file, as well as on the command line.
> 
> The options from the configuration files are appended together with
> the ones from the command line, which makes it impossible for users to
> override options from the configuration file, if duplicated options
> are refused, unless Podman takes care of sorting them, which is
> clearly not sustainable.
> 
> For --debug and --trace, somebody took care of this on Podman side at:
>   https://github.com/containers/common/pull/2052
> 
> but this doesn't fix the issue with other options, and we'll have
> anyway older versions of Podman around, too.
> 
> 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.
> 
> Notable exception: --stderr still conflicts with --log-file, because
> users might have the expectation that they don't actually conflict.
> But they do conflict in the existing implementation, so it's safer
> to make sure that the users notice that.
> 
> Suggested-by: Paul Holzinger <pholzing@redhat.com>
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2:
>   - report --log-file and --stderr as conflicting, but accept
>     multiple options if they don't conflict
>   - for --log-file and --pcap, make it clear in the man page that
>     the general principle still applies: only the last one (of each)
>     takes effect
>   - add some more context about Podman and how the list of pasta
>     options is built there, in the commit message
> 
>  conf.c  | 134 ++++++++++++++++----------------------------------------
>  passt.1 |  17 +++++++
>  2 files changed, 55 insertions(+), 96 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 94b3ed6..c5f13ac 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");
>  
> @@ -1204,7 +1201,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 },
> @@ -1221,6 +1217,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 };
> @@ -1265,6 +1262,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)
> @@ -1325,24 +1324,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);
>  
> @@ -1356,9 +1344,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))
> @@ -1366,9 +1351,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))
> @@ -1395,62 +1377,47 @@ 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);
>  
> @@ -1458,12 +1425,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)
> @@ -1471,18 +1435,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))
> @@ -1490,9 +1448,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);
>  
> @@ -1542,14 +1497,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))
> @@ -1558,16 +1511,12 @@ 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("Invalid interface name %s: %s", optarg,
>  				    strerror(errno));
>  			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)	  &&
> @@ -1575,8 +1524,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))
> @@ -1587,18 +1535,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)) {
> @@ -1616,18 +1569,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),
> @@ -1643,17 +1592,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':
> @@ -1672,12 +1620,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 7676fe3..7b9915b 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -73,6 +73,9 @@ for performance reasons.
>  
>  .SH OPTIONS
>  
> +Unless otherwise noted below, \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.
> @@ -97,10 +100,21 @@ Log to standard error too.
>  Default is to log to the system logger only, if started from an interactive
>  terminal, and to both system logger and standard error otherwise.
>  
> +This option cannot be specified together with \fB--log-file\fR, because there
> +might be a reasonable expectation that messages are logged to standard error as
> +well as to a file, which is however not supported.
> +
>  .TP
>  .BR \-l ", " \-\-log-file " " \fIPATH\fR
>  Log to file \fIPATH\fR, not to standard error, and not to the system logger.
>  
> +This option cannot be specified together with \fB--stderr\fR, because there
> +might be a reasonable expectation that messages are logged to a file as well as
> +to standard error, which is however not supported.
> +
> +Specifying this option multiple times does \fInot\fR lead to multiple log files:
> +the last given option takes effect.
> +
>  .TP
>  .BR \-\-log-size " " \fISIZE\fR
>  Limit log file size to \fISIZE\fR bytes. When the log file is full, make room
> @@ -128,6 +142,9 @@ Show version and exit.
>  Capture tap-facing (that is, guest-side or namespace-side) network packets to
>  \fIfile\fR in \fBpcap\fR format.
>  
> +Specifying this option multiple times does \fInot\fR lead to multiple capture
> +files: the last given option takes effect.
> +
>  .TP
>  .BR \-P ", " \-\-pid " " \fIfile
>  Write own PID to \fIfile\fR once initialisation is done, before forking to

-- 
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 v2] conf: Accept duplicate and conflicting options, the last one wins

[Paul Holzinger]

On 19/06/2024 17:40, 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.
>
> On top of this, Podman lets users specify other pasta options in its
> containers.conf(5) file, as well as on the command line.
>
> The options from the configuration files are appended together with
> the ones from the command line, which makes it impossible for users to
> override options from the configuration file, if duplicated options
> are refused, unless Podman takes care of sorting them, which is
> clearly not sustainable.
>
> For --debug and --trace, somebody took care of this on Podman side at:
>    https://github.com/containers/common/pull/2052
>
> but this doesn't fix the issue with other options, and we'll have
> anyway older versions of Podman around, too.
>
> 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.
>
> Notable exception: --stderr still conflicts with --log-file, because
> users might have the expectation that they don't actually conflict.
> But they do conflict in the existing implementation, so it's safer
> to make sure that the users notice that.
>
> Suggested-by: Paul Holzinger <pholzing@redhat.com>
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> v2:
>    - report --log-file and --stderr as conflicting, but accept
>      multiple options if they don't conflict
>    - for --log-file and --pcap, make it clear in the man page that
>      the general principle still applies: only the last one (of each)
>      takes effect
>    - add some more context about Podman and how the list of pasta
>      options is built there, in the commit message
>
>   conf.c  | 134 ++++++++++++++++----------------------------------------
>   passt.1 |  17 +++++++
>   2 files changed, 55 insertions(+), 96 deletions(-)

I have done some tests and while you changed --gateway to only take the 
last one --address was not changed:

./pasta --config-net -a 10.0.0.1 -a 10.0.0.2 ip a

Invalid address: 10.0.0.2

The error is clearly nonsensical as the address is valid. So either it 
should match the gateway behavior or the error must clearly state that 
multiple addresses are not accepted if this is what you want.

-- 
Paul Holzinger

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

[Stefano Brivio]

On Thu, 20 Jun 2024 11:06:48 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> On 19/06/2024 17:40, 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.
> >
> > On top of this, Podman lets users specify other pasta options in its
> > containers.conf(5) file, as well as on the command line.
> >
> > The options from the configuration files are appended together with
> > the ones from the command line, which makes it impossible for users to
> > override options from the configuration file, if duplicated options
> > are refused, unless Podman takes care of sorting them, which is
> > clearly not sustainable.
> >
> > For --debug and --trace, somebody took care of this on Podman side at:
> >    https://github.com/containers/common/pull/2052
> >
> > but this doesn't fix the issue with other options, and we'll have
> > anyway older versions of Podman around, too.
> >
> > 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.
> >
> > Notable exception: --stderr still conflicts with --log-file, because
> > users might have the expectation that they don't actually conflict.
> > But they do conflict in the existing implementation, so it's safer
> > to make sure that the users notice that.
> >
> > Suggested-by: Paul Holzinger <pholzing@redhat.com>
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > v2:
> >    - report --log-file and --stderr as conflicting, but accept
> >      multiple options if they don't conflict
> >    - for --log-file and --pcap, make it clear in the man page that
> >      the general principle still applies: only the last one (of each)
> >      takes effect
> >    - add some more context about Podman and how the list of pasta
> >      options is built there, in the commit message
> >
> >   conf.c  | 134 ++++++++++++++++----------------------------------------
> >   passt.1 |  17 +++++++
> >   2 files changed, 55 insertions(+), 96 deletions(-)  
> 
> I have done some tests and while you changed --gateway to only take the 
> last one --address was not changed:
> 
> ./pasta --config-net -a 10.0.0.1 -a 10.0.0.2 ip a
> 
> Invalid address: 10.0.0.2
> 
> The error is clearly nonsensical as the address is valid. So either it 
> should match the gateway behavior or the error must clearly state that 
> multiple addresses are not accepted if this is what you want.

Gosh, thanks, nice catch, v3 coming (in a while).

-- 
Stefano