public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: passt-dev@passt.top
Cc: Paul Holzinger <pholzing@redhat.com>,
	David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH] conf: Accept duplicate and conflicting options, the last one wins
Date: Tue, 18 Jun 2024 18:17:23 +0200	[thread overview]
Message-ID: <20240618161723.1896519-1-sbrivio@redhat.com> (raw)

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


             reply	other threads:[~2024-06-18 16:17 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-18 16:17 Stefano Brivio [this message]
2024-06-19  1:50 ` [PATCH] conf: Accept duplicate and conflicting options, the last one wins David Gibson
2024-06-19  7:45   ` Stefano Brivio
2024-06-20  0:03     ` David Gibson
2024-06-19  9:06 ` Paul Holzinger
2024-06-19 10:22   ` Stefano Brivio

Reply instructions:

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

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

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

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

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

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).