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(a)gibson.dropbear.id.au> wrote: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'.On Tue, Jun 18, 2024 at 06:17:23PM +0200, Stefano Brivio wrote: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).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(a)redhat.com> Suggested-by: David Gibson <david(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)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.Faor enough.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.Sure, I don't see that either, but for the sake of keeping the documentation simple, I would still force this.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.Good point.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.@@ -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.I suppose....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.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.-- 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/~dgibsonBut it can't happen, right? I mean, it's very clear that when we set one, we clear the other one (hunk above).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.