On Fri, 26 Aug 2022 14:58:33 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Both the -D (--dns) and -S (--search) options take an optional argument. If the argument is omitted the option is disabled entirely. However, handling the optional argument requires some ugly special case handling if it's the last option on the command line, and has potential ambiguity with non-option arguments used with pasta. It can also make it more confusing to read command lines. Simplify the logic here by replacing the non-argument versions with an explicit "-D none" or "-S none". Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- conf.c | 18 ++++-------------- passt.1 | 7 ++++--- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/conf.c b/conf.c index 4eb9e3d..6770be9 100644 --- a/conf.c +++ b/conf.c @@ -1022,8 +1022,8 @@ void conf(struct ctx *c, int argc, char **argv) {"mac-addr", required_argument, NULL, 'M' }, {"gateway", required_argument, NULL, 'g' }, {"interface", required_argument, NULL, 'i' }, - {"dns", optional_argument, NULL, 'D' }, - {"search", optional_argument, NULL, 'S' }, + {"dns", required_argument, NULL, 'D' }, + {"search", required_argument, NULL, 'S' }, {"no-tcp", no_argument, &c->no_tcp, 1 }, {"no-udp", no_argument, &c->no_udp, 1 }, {"no-icmp", no_argument, &c->no_icmp, 1 }, @@ -1077,16 +1077,6 @@ void conf(struct ctx *c, int argc, char **argv) name = getopt_long(argc, argv, optstring, options, NULL); - if ((name == 'D' || name == 'S') && !optarg && - optind < argc && *argv[optind] && *argv[optind] != '-') { - if (c->mode == MODE_PASTA) { - if (conf_ns_opt(c, nsdir, userns, argv[optind])) - optarg = argv[optind++]; - } else { - optarg = argv[optind++]; - } - } - switch (name) { case -1: case 0: @@ -1403,7 +1393,7 @@ void conf(struct ctx *c, int argc, char **argv) usage(argv[0]); } - if (!optarg) { + if (!strcmp(optarg, "none")) { c->no_dns = 1; break; } @@ -1430,7 +1420,7 @@ void conf(struct ctx *c, int argc, char **argv) usage(argv[0]); } - if (!optarg) { + if (!strcmp(optarg, "none")) { c->no_dns_search = 1; break; }For these two hunks, clang-tidy reported something I missed in my review: /home/sbrivio/passt/conf.c:1417:9: error: Null pointer passed to 1st parameter expecting 'nonnull' [clang-analyzer-core.NonNullParamChecker,-warnings-as-errors] if (!strcmp(optarg, "none")) { ^ ~~~~~~ [...] /home/sbrivio/passt/conf.c:1411:8: note: Assuming field 'no_dns' is 0 if (c->no_dns || ^~~~~~~~~ /home/sbrivio/passt/conf.c:1411:8: note: Left side of '||' is false /home/sbrivio/passt/conf.c:1412:9: note: Assuming 'optarg' is null (!optarg && (dns4 - c->ip4.dns || dns6 - c->ip6.dns))) { ^~~~~~~ /home/sbrivio/passt/conf.c:1412:9: note: Left side of '&&' is true /home/sbrivio/passt/conf.c:1412:21: note: Left side of '||' is false (!optarg && (dns4 - c->ip4.dns || dns6 - c->ip6.dns))) { ...the validation logic needs to be updated here. I'm taking the liberty of fixing this up on merge, with this diff on top: diff --git a/conf.c b/conf.c index 162c2dd..e6d1c62 100644 --- a/conf.c +++ b/conf.c @@ -1408,17 +1408,26 @@ void conf(struct ctx *c, int argc, char **argv) } break; case 'D': - if (c->no_dns || - (!optarg && (dns4 - c->ip4.dns || dns6 - c->ip6.dns))) { - err("Empty and non-empty DNS options given"); - usage(argv[0]); - } - if (!strcmp(optarg, "none")) { + if (c->no_dns) { + err("Redundant DNS options"); + usage(argv[0]); + } + + if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) { + err("Conflicting DNS options"); + usage(argv[0]); + } + c->no_dns = 1; break; } + if (c->no_dns) { + err("Conflicting DNS options"); + usage(argv[0]); + } + if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) && inet_pton(AF_INET, optarg, dns4)) { dns4++; @@ -1435,17 +1444,26 @@ void conf(struct ctx *c, int argc, char **argv) usage(argv[0]); break; case 'S': - if (c->no_dns_search || - (!optarg && dnss != c->dns_search)) { - err("Empty and non-empty DNS search given"); - usage(argv[0]); - } - if (!strcmp(optarg, "none")) { + if (c->no_dns_search) { + err("Redundant DNS search options"); + usage(argv[0]); + } + + if (dnss != c->dns_search) { + err("Conflicting DNS search options"); + usage(argv[0]); + } + c->no_dns_search = 1; break; } + if (c->no_dns_search) { + err("Conflicting DNS search options"); + usage(argv[0]); + } + if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) { ret = snprintf(dnss->n, sizeof(*c->dns_search), "%s", optarg); -- Stefano