From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 1F6FF5A004E for ; Wed, 19 Jun 2024 09:46:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1718783169; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VfXDybSNgaskYSKkdlBdy4UTYbqjhzoDG+OTP6bnOjk=; b=hFU+qaUphkS4UCbDPXe6E0cO8qdqjtwAKdaSwWksm4ypZDWEFf0BHnStFhWkYJOIMt5wnt pn+5bHnkz4t08RqL5GSmRXcq4/XAa3kD5c2z/5j/J0C8O4xWff38hjP2I7TuC8ihEL/zZQ FgnqX4fYwkHrNQmlMTVA5aZC9/eOObg= Received: from mail-yb1-f198.google.com (mail-yb1-f198.google.com [209.85.219.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-46-VUi3BP6eNrOMJ4NX8UFsyw-1; Wed, 19 Jun 2024 03:46:07 -0400 X-MC-Unique: VUi3BP6eNrOMJ4NX8UFsyw-1 Received: by mail-yb1-f198.google.com with SMTP id 3f1490d57ef6-dfdfe3d9fc8so12769142276.3 for ; Wed, 19 Jun 2024 00:46:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718783166; x=1719387966; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=VfXDybSNgaskYSKkdlBdy4UTYbqjhzoDG+OTP6bnOjk=; b=aaC8qNjb6OtpK9Q0qtIZ+hAz5wQ4v7GuCPX1OtC/6epv1wJrnKTHyyH2PlV9CKLtEA C+Ep42PvhpA3b1N0bFo6Mxi6R0dssz1YpYw1thqBd528rIExRutSB0lj2IfCcXnGwvn6 xFFyTDrXdp5VFjXDpbl1LZUrXbbSj1SLYZN4uq5G/s/GYPJG8xkOJaidJHG/fS18jYr6 GJDgnhslvK9u0wOdKPu8LNW5w1bJd0cxQlmDvAbbOYJJ2QGghskWgpSXYBd3Y5ieYsan oACz9LWZFCp4CbQWEnbGwZICpeSE65ZLVRpz50bUiZ2ekgCyiCplYpGO1EgyB3NLxGqw 4P1Q== X-Gm-Message-State: AOJu0YyTOG8OUDmhdUABvtw2h7lgrz5MWly+658jzSvPTcvXaRxVXmDR qDQuoELXwrpVRrCow+0gtD+In3+Hb497Gi5R1B9/fvDeOCsXeNdap+nkyMf3c/V/v00Ws+TeL7K RsEbI25xVhvb2f2/AsU2HBHShglrEzGKnOCV4xzDH6fU+JgcxCw== X-Received: by 2002:a25:3611:0:b0:e02:74aa:fa with SMTP id 3f1490d57ef6-e02be22d86amr2127455276.54.1718783166208; Wed, 19 Jun 2024 00:46:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEN/KRoZTfGTbWbSOM5ifl+3yj79kmosNBysodgz62YwoKvnWLL6x3xxIz48RZbagcGeeSxvA== X-Received: by 2002:a25:3611:0:b0:e02:74aa:fa with SMTP id 3f1490d57ef6-e02be22d86amr2127438276.54.1718783165643; Wed, 19 Jun 2024 00:46:05 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-441ef3dc327sm62921181cf.17.2024.06.19.00.46.04 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 19 Jun 2024 00:46:05 -0700 (PDT) Date: Wed, 19 Jun 2024 09:45:30 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH] conf: Accept duplicate and conflicting options, the last one wins Message-ID: <20240619094522.4515597e@elisabeth> In-Reply-To: References: <20240618161723.1896519-1-sbrivio@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: LNQSIRJZUKAFX42TQDSIM2FORGKKRQLB X-Message-ID-Hash: LNQSIRJZUKAFX42TQDSIM2FORGKKRQLB X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Paul Holzinger X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 19 Jun 2024 11:50:44 +1000 David Gibson 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 > > Suggested-by: David Gibson > > Signed-off-by: Stefano Brivio > > [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