From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Brivio To: passt-dev@passt.top Subject: Re: [PATCH 2/8] conf: Use "-D none" and "-S none" instead of missing empty option arguments Date: Tue, 30 Aug 2022 19:41:34 +0200 Message-ID: <20220830194134.7c79ce80@elisabeth> In-Reply-To: <20220826045839.1112152-3-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5991623811199248090==" --===============5991623811199248090== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Fri, 26 Aug 2022 14:58:33 +1000 David Gibson 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. >=20 > Simplify the logic here by replacing the non-argument versions with an > explicit "-D none" or "-S none". >=20 > Signed-off-by: David Gibson > --- > conf.c | 18 ++++-------------- > passt.1 | 7 ++++--- > 2 files changed, 8 insertions(+), 17 deletions(-) >=20 > 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) > =20 > name =3D getopt_long(argc, argv, optstring, options, NULL); > =20 > - if ((name =3D=3D 'D' || name =3D=3D 'S') && !optarg && > - optind < argc && *argv[optind] && *argv[optind] !=3D '-') { > - if (c->mode =3D=3D MODE_PASTA) { > - if (conf_ns_opt(c, nsdir, userns, argv[optind])) > - optarg =3D argv[optind++]; > - } else { > - optarg =3D argv[optind++]; > - } > - } > - > switch (name) { > case -1: > case 0: > @@ -1403,7 +1393,7 @@ void conf(struct ctx *c, int argc, char **argv) > usage(argv[0]); > } > =20 > - if (!optarg) { > + if (!strcmp(optarg, "none")) { > c->no_dns =3D 1; > break; > } > @@ -1430,7 +1420,7 @@ void conf(struct ctx *c, int argc, char **argv) > usage(argv[0]); > } > =20 > - if (!optarg) { > + if (!strcmp(optarg, "none")) { > c->no_dns_search =3D 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 paramete= r expecting 'nonnull' [clang-analyzer-core.NonNullParamChecker,-warnings-as-e= rrors] 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.d= ns))) { ^~~~~~~ /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.d= ns))) { ...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 =3D 1; break; } =20 + 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 !=3D 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 !=3D c->dns_search) { + err("Conflicting DNS search options"); + usage(argv[0]); + } + c->no_dns_search =3D 1; break; } =20 + if (c->no_dns_search) { + err("Conflicting DNS search options"); + usage(argv[0]); + } + if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) { ret =3D snprintf(dnss->n, sizeof(*c->dns_search), "%s", optarg); --=20 Stefano --===============5991623811199248090==--