From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 5CCFC5A0082 for ; Thu, 16 Feb 2023 06:43:25 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4PHP4K6BY2z4x8C; Thu, 16 Feb 2023 16:43:13 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1676526193; bh=jKmzFts0CaPGL97ET9YeZZx3BQn/Ap8W0HZS5dINxoY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ENKKfvMIqtBJsmiHskM4kVSNPk2Sflno3UR3cElxh2dafCxO0iUK3O2ew0NrDuwKO /toN2z7L2CU28WPvHNJ+l0vC4D0iKDccPPAfp/Z2luTkjwSDfve/wg+3wWaX/pNO65 1iE14xIBGUl5lAcwJVzX/63wwSkz0hWdaS2CkdeM= Date: Thu, 16 Feb 2023 16:36:23 +1100 From: David Gibson To: Laine Stump Subject: Re: [PATCH v4 4/9] make conf_ports() exit immediately after logging error Message-ID: References: <20230215082437.110151-1-laine@redhat.com> <20230215082437.110151-5-laine@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Kl6mQdigz35z3z3r" Content-Disposition: inline In-Reply-To: <20230215082437.110151-5-laine@redhat.com> Message-ID-Hash: RYBCZ5VDLHZEA5RJEXDJ3ET5ZJSQJKVH X-Message-ID-Hash: RYBCZ5VDLHZEA5RJEXDJ3ET5ZJSQJKVH X-MailFrom: dgibson@gandalf.ozlabs.org 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 X-Mailman-Version: 3.3.3 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: --Kl6mQdigz35z3z3r Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 15, 2023 at 03:24:32AM -0500, Laine Stump wrote: > Rather than having conf_ports() (possibly) log an error, and then > letting the caller log the entire usage() message and exit, just log > the errors and exit immediately (using die()). >=20 > For some errors, conf_ports would previously not log any specific > message, leaving it up to the user to determine the problem by > guessing. We replace all of those silent returns with die() > (logging a specific error), thus permitting us to make conf_ports() > return void, which simplifies the caller. >=20 > While modifying the two callers to conf_ports() to not check for a > return value, we can further simplify the code by removing the check > for a non-null optarg, as that is guaranteed to never happen (due to > prior calls to getopt_long() with "argument required" for all relevant > options - getopt_long() would have already caught this error). >=20 > Signed-off-by: Laine Stump Reviewed-by: David Gibson > --- > conf.c | 52 ++++++++++++++++++++++++++++------------------------ > 1 file changed, 28 insertions(+), 24 deletions(-) >=20 > diff --git a/conf.c b/conf.c > index ad8c249..0d4ff79 100644 > --- a/conf.c > +++ b/conf.c > @@ -173,11 +173,9 @@ static int parse_port_range(const char *s, char **en= dptr, > * @optname: Short option name, t, T, u, or U > * @optarg: Option argument (port specification) > * @fwd: Pointer to @port_fwd to be updated > - * > - * Return: -EINVAL on parsing error, 0 otherwise > */ > -static int conf_ports(const struct ctx *c, char optname, const char *opt= arg, > - struct port_fwd *fwd) > +static void conf_ports(const struct ctx *c, char optname, const char *op= targ, > + struct port_fwd *fwd) > { > char addr_buf[sizeof(struct in6_addr)] =3D { 0 }, *addr =3D addr_buf; > char buf[BUFSIZ], *spec, *ifname =3D NULL, *p; > @@ -187,23 +185,32 @@ static int conf_ports(const struct ctx *c, char opt= name, const char *optarg, > =20 > if (!strcmp(optarg, "none")) { > if (fwd->mode) > - return -EINVAL; > + goto mode_conflict; > + > fwd->mode =3D FWD_NONE; > - return 0; > + return; > } > =20 > if (!strcmp(optarg, "auto")) { > - if (fwd->mode || c->mode !=3D MODE_PASTA) > - return -EINVAL; > + if (fwd->mode) > + goto mode_conflict; > + > + if (c->mode !=3D MODE_PASTA) > + die("'auto' port forwarding is only allowed for pasta"); > + > fwd->mode =3D FWD_AUTO; > - return 0; > + return; > } > =20 > if (!strcmp(optarg, "all")) { > unsigned i; > =20 > - if (fwd->mode || c->mode !=3D MODE_PASST) > - return -EINVAL; > + if (fwd->mode) > + goto mode_conflict; > + > + if (c->mode !=3D MODE_PASST) > + die("'all' port forwarding is only allowed for passt"); > + > fwd->mode =3D FWD_ALL; > memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8); > =20 > @@ -214,11 +221,11 @@ static int conf_ports(const struct ctx *c, char opt= name, const char *optarg, > udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i); > } > =20 > - return 0; > + return; > } > =20 > if (fwd->mode > FWD_SPEC) > - return -EINVAL; > + die("Specific ports cannot be specified together with all/none/auto"); > =20 > fwd->mode =3D FWD_SPEC; > =20 > @@ -292,7 +299,7 @@ static int conf_ports(const struct ctx *c, char optna= me, const char *optarg, > udp_sock_init(c, 0, af, addr, ifname, i); > } > =20 > - return 0; > + return; > } > =20 > /* Now process base ranges, skipping exclusions */ > @@ -339,14 +346,13 @@ static int conf_ports(const struct ctx *c, char opt= name, const char *optarg, > } > } while ((p =3D next_chunk(p, ','))); > =20 > - return 0; > + return; > bad: > - err("Invalid port specifier %s", optarg); > - return -EINVAL; > - > + die("Invalid port specifier %s", optarg); > overlap: > - err("Overlapping port specifier %s", optarg); > - return -EINVAL; > + die("Overlapping port specifier %s", optarg); > +mode_conflict: > + die("Port forwarding mode '%s' conflicts with previous mode", optarg); > } > =20 > /** > @@ -1550,8 +1556,7 @@ void conf(struct ctx *c, int argc, char **argv) > =20 > if ((name =3D=3D 't' && (fwd =3D &c->tcp.fwd_in)) || > (name =3D=3D 'u' && (fwd =3D &c->udp.fwd_in.f))) { > - if (!optarg || conf_ports(c, name, optarg, fwd)) > - usage(argv[0]); > + conf_ports(c, name, optarg, fwd); > } > } while (name !=3D -1); > =20 > @@ -1589,8 +1594,7 @@ void conf(struct ctx *c, int argc, char **argv) > =20 > if ((name =3D=3D 'T' && (fwd =3D &c->tcp.fwd_out)) || > (name =3D=3D 'U' && (fwd =3D &c->udp.fwd_out.f))) { > - if (!optarg || conf_ports(c, name, optarg, fwd)) > - usage(argv[0]); > + conf_ports(c, name, optarg, fwd); > } > } while (name !=3D -1); > =20 --=20 David Gibson | 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/~dgibson --Kl6mQdigz35z3z3r Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmPtwNEACgkQzQJF27ox 2Gee9w//R6/PQs81vtFP+9+ysfUOuZAYKYAYhxOY2iS5YmVeTnDokvaLUaOp1Dvr lJdKNPK5cUSNUETsKd3XQylu0ubvBWYX06gSNFnxsd6MII/SpAlgB1ng4sjJEiZ5 uX8HXLQF6Z4RPkB5PONnaTTC28G89sWsx7mXq41VvSJwIchN8G3oyPACRfUFXNot 5vZrNwJ75rTTDK4ds7XdNyf5rE1IbezIhjd+o8EY3tV7A05KcIyRcIW1LTKmW5gn aZbqFaOyx8kgxBBxt5CUxGRutBQBA0ZqpHwx4IeuFVRM/uQriKcGtjnElt6wOHLx ZgM0msyYk+RXQyQU0OYvshRqc8EXAWTPzrCvX2OKZgZeydqgEfPKYbGTNvA45ko+ qRoJEzWbSKewN0MhVlxtAqDSmQifJtkHidTTnXf29cGKVMHrMnblmoYFU9Dn1IJ+ 9HBcwR+JOvHAuJQ2dF+cnj9d7y5Zya66DgGZhW2CPUG33R52ACexu+COZkjjw9dK Ypc6NGJpb2xN2qXxFCG2J1p1gBCWWbItkiyPu6U6T5pQDyqOBLvR+t2YtPAtEKqY OboVw2uMe2tFhxXj0NXtKoPiiquTOYbbJ7q7Lz01HLiU6LkSvkuguiFfc3kMzvna lagl2jVPhGRAHsrV62nzLrY+wdEpPdGVTMylPrTTz+1UwM2e0DU= =IJkk -----END PGP SIGNATURE----- --Kl6mQdigz35z3z3r--