From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202502 header.b=ezWq45fu; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id E163A5A0274 for ; Thu, 20 Feb 2025 01:39:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740011936; bh=uhqS74TJG2wWFHWXdU8rbl0shzVVxWjsYPtyuDDTBYg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ezWq45fu1Kd/cVemW0rTbXmNEoje0pT4SP43OqFZSGIQX/kkaVRrFBUy2hC90BhUV OQIXMUK9+pjm6zrbqBdr8nEJpPcXIfdFkpwjUoj1INuOWFW8M+N9wNMsr8i02sVSMl XAUVKu7lU7esmeuqQeoGrHB2MtW++AnXWhVjYZHTf00ynuSkghP+c2ENxJovK4hwAS LcLBN5qG5rxfRu5Uj+Y/Or6LMBXkSUQ9WFNUnvhzSQX5sfC/tVq2q9uhTGC7ow9aqa 41/kCa+czGZdtresefutcyOX10c8pTSNnCi4YE/MEqs7fh4dggFSKf4Qj+hJxYDkC6 1xPPe/x/rBF7A== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YyvX00pmyz4x2g; Thu, 20 Feb 2025 11:38:56 +1100 (AEDT) Date: Thu, 20 Feb 2025 11:25:26 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] conf: Unify several paths in conf_ports() Message-ID: References: <20250219035444.4067664-1-david@gibson.dropbear.id.au> <20250219063732.1e32dfb7@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="RzbLOH21PCgmQa1R" Content-Disposition: inline In-Reply-To: <20250219063732.1e32dfb7@elisabeth> Message-ID-Hash: 7FLXILRXA3EWNKEBHLX5GHAM4H3DCRDA X-Message-ID-Hash: 7FLXILRXA3EWNKEBHLX5GHAM4H3DCRDA 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.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: --RzbLOH21PCgmQa1R Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 19, 2025 at 06:37:32AM +0100, Stefano Brivio wrote: > On Wed, 19 Feb 2025 14:54:44 +1100 > David Gibson wrote: >=20 > > In conf_ports() we have three different paths which actually do the set= up > > of an individual forwarded port: one for the "all" case, one for the > > exclusions only case and one for the range of ports with possible > > exclusions case. > >=20 > > We can unify those cases using a new helper which handles a single range > > of ports, with a bitmap of exclusions. Although this is slightly longer > > (largely due to the new helpers function comment), it reduces duplicated > > logic. It will also make future improvements to the tracking of port > > forwards easier. > >=20 > > Signed-off-by: David Gibson > > --- > > conf.c | 175 +++++++++++++++++++++++++++++---------------------------- > > 1 file changed, 90 insertions(+), 85 deletions(-) > >=20 > > diff --git a/conf.c b/conf.c > > index 18017f51..f862845b 100644 > > --- a/conf.c > > +++ b/conf.c > > @@ -123,6 +123,75 @@ static int parse_port_range(const char *s, char **= endptr, > > return 0; > > } > > =20 > > +/** > > + * conf_ports_range_except() - Set up forwarding for a range of ports = minus a > > + * bitmap of exclusions > > + * @c: Execution context > > + * @optname: Short option name, t, T, u, or U > > + * @optarg: Option argument (port specification) > > + * @fwd: Pointer to @fwd_ports to be updated > > + * @addr: Listening address > > + * @ifname: Listening interface > > + * @first: First port to forward > > + * @last: Last port to forward > > + * @exclude: Bitmap of ports to exclude > > + * @to: Port to translate @first to when forwarding > > + * @weak: Ignore errors, as long as at least one port is mapped > > + */ > > +static void conf_ports_range_except(const struct ctx *c, char optname, > > + const char *optarg, struct fwd_ports *fwd, > > + const union inany_addr *addr, > > + const char *ifname, > > + uint16_t first, uint16_t last, > > + const uint8_t *exclude, uint16_t to, > > + bool weak) >=20 > ...ouch, bool anything_else? I think this patch is strictly better than > the existing duplication, so I'm fine with it as it is. I don't see any > obvious way to make the prototype smaller (other than things that would > reduce readability by hiding information), so... yeah. I just wanted to > mention that the prototype is a bit heavy. Yes, it certainly is. Like you, I concluded it's still better than the existing duplication. > > +{ > > + bool bound_one =3D false; > > + unsigned i; > > + int ret; > > + > > + if (first =3D=3D 0) { > > + die("Can't forward port 0 for option '-%c %s'", > > + optname, optarg); > > + } > > + > > + for (i =3D first; i <=3D last; i++) { > > + if (bitmap_isset(exclude, i)) > > + continue; > > + > > + if (bitmap_isset(fwd->map, i)) { > > + warn( > > +"Altering mapping of already mapped port number: %s", optarg); > > + } > > + > > + bitmap_set(fwd->map, i); > > + fwd->delta[i] =3D to - first; > > + > > + if (optname =3D=3D 't') > > + ret =3D tcp_sock_init(c, addr, ifname, i); > > + else if (optname =3D=3D 'u') > > + ret =3D udp_sock_init(c, 0, addr, ifname, i); > > + else > > + /* No way to check in advance for -T and -U */ > > + ret =3D 0; > > + > > + if (ret =3D=3D -ENFILE || ret =3D=3D -EMFILE) { > > + die("Can't open enough sockets for port specifier: %s", > > + optarg); > > + } > > + > > + if (!ret) { > > + bound_one =3D true; > > + } else if (!weak) { > > + die("Failed to bind port %u (%s) for option '-%c %s'", > > + i, strerror_(-ret), optname, optarg); > > + } > > + } > > + > > + if (!bound_one) > > + die("Failed to bind any port for '-%c %s'", optname, optarg); > > +} > > + > > /** > > * conf_ports() - Parse port configuration options, initialise UDP/TCP= sockets > > * @c: Execution context > > @@ -135,10 +204,9 @@ static void conf_ports(const struct ctx *c, char o= ptname, const char *optarg, > > { > > union inany_addr addr_buf =3D inany_any6, *addr =3D &addr_buf; > > char buf[BUFSIZ], *spec, *ifname =3D NULL, *p; > > - bool exclude_only =3D true, bound_one =3D false; > > uint8_t exclude[PORT_BITMAP_SIZE] =3D { 0 }; > > + bool exclude_only =3D true; > > unsigned i; > > - int ret; > > =20 > > if (!strcmp(optarg, "none")) { > > if (fwd->mode) > > @@ -173,32 +241,14 @@ static void conf_ports(const struct ctx *c, char = optname, const char *optarg, > > =20 > > fwd->mode =3D FWD_ALL; > > =20 > > - /* Skip port 0. It has special meaning for many socket APIs, so > > - * trying to bind it is not really safe. > > - */ > > - for (i =3D 1; i < NUM_PORTS; i++) { > > + /* Also exclude ephemeral ports */ >=20 > "Also" implies the reader figures out that conf_ports_range_except() > skips port zero while looking at this line, which is not really > something you can expect, I guess. Well.. it's not conf_ports_range_except() excluding it, but the fact we past '1', not '0' as the @first parameter. > We could probably just drop this comment, or change it as you did > below. But yeah, I'll update it like below. >=20 > > + for (i =3D 0; i < NUM_PORTS; i++) > > if (fwd_port_is_ephemeral(i)) > > - continue; > > - > > - bitmap_set(fwd->map, i); > > - if (optname =3D=3D 't') { > > - ret =3D tcp_sock_init(c, NULL, NULL, i); > > - if (ret =3D=3D -ENFILE || ret =3D=3D -EMFILE) > > - goto enfile; > > - if (!ret) > > - bound_one =3D true; > > - } else if (optname =3D=3D 'u') { > > - ret =3D udp_sock_init(c, 0, NULL, NULL, i); > > - if (ret =3D=3D -ENFILE || ret =3D=3D -EMFILE) > > - goto enfile; > > - if (!ret) > > - bound_one =3D true; > > - } > > - } > > - > > - if (!bound_one) > > - goto bind_all_fail; > > - > > + bitmap_set(exclude, i); >=20 > I would add an extra blank line here, otherwise it's quite confusing. Good call, done. > > + conf_ports_range_except(c, optname, optarg, fwd, > > + NULL, NULL, > > + 1, NUM_PORTS - 1, exclude, > > + 1, true); > > return; > > } > > =20 > > @@ -275,37 +325,14 @@ static void conf_ports(const struct ctx *c, char = optname, const char *optarg, > > } while ((p =3D next_chunk(p, ','))); > > =20 > > if (exclude_only) { > > - /* Skip port 0. It has special meaning for many socket APIs, so > > - * trying to bind it is not really safe. > > - */ > > - for (i =3D 1; i < NUM_PORTS; i++) { > > - if (fwd_port_is_ephemeral(i) || > > - bitmap_isset(exclude, i)) > > - continue; > > - > > - bitmap_set(fwd->map, i); > > - > > - if (optname =3D=3D 't') { > > - ret =3D tcp_sock_init(c, addr, ifname, i); > > - if (ret =3D=3D -ENFILE || ret =3D=3D -EMFILE) > > - goto enfile; > > - if (!ret) > > - bound_one =3D true; > > - } else if (optname =3D=3D 'u') { > > - ret =3D udp_sock_init(c, 0, addr, ifname, i); > > - if (ret =3D=3D -ENFILE || ret =3D=3D -EMFILE) > > - goto enfile; > > - if (!ret) > > - bound_one =3D true; > > - } else { > > - /* No way to check in advance for -T and -U */ > > - bound_one =3D true; > > - } > > - } > > - > > - if (!bound_one) > > - goto bind_all_fail; > > - > > + /* Exclude ephemeral ports */ > > + for (i =3D 0; i < NUM_PORTS; i++) > > + if (fwd_port_is_ephemeral(i)) > > + bitmap_set(exclude, i); >=20 > Same here, a blank line would be nice. Done. > > + conf_ports_range_except(c, optname, optarg, fwd, > > + addr, ifname, > > + 1, NUM_PORTS - 1, exclude, > > + 1, true); > > return; > > } > > =20 > > @@ -334,40 +361,18 @@ static void conf_ports(const struct ctx *c, char = optname, const char *optarg, > > if ((*p !=3D '\0') && (*p !=3D ',')) /* Garbage after the ranges */ > > goto bad; > > =20 > > - for (i =3D orig_range.first; i <=3D orig_range.last; i++) { > > - if (bitmap_isset(fwd->map, i)) > > - warn( > > -"Altering mapping of already mapped port number: %s", optarg); > > - > > - if (bitmap_isset(exclude, i)) > > - continue; > > - > > - bitmap_set(fwd->map, i); > > - > > - fwd->delta[i] =3D mapped_range.first - orig_range.first; > > - > > - ret =3D 0; > > - if (optname =3D=3D 't') > > - ret =3D tcp_sock_init(c, addr, ifname, i); > > - else if (optname =3D=3D 'u') > > - ret =3D udp_sock_init(c, 0, addr, ifname, i); > > - if (ret) > > - goto bind_fail; > > - } > > + conf_ports_range_except(c, optname, optarg, fwd, > > + addr, ifname, > > + orig_range.first, orig_range.last, > > + exclude, > > + mapped_range.first, false); > > } while ((p =3D next_chunk(p, ','))); > > =20 > > return; > > -enfile: > > - die("Can't open enough sockets for port specifier: %s", optarg); > > bad: > > die("Invalid port specifier %s", optarg); > > mode_conflict: > > die("Port forwarding mode '%s' conflicts with previous mode", optarg); > > -bind_fail: > > - die("Failed to bind port %u (%s) for option '-%c %s', exiting", > > - i, strerror_(-ret), optname, optarg); > > -bind_all_fail: > > - die("Failed to bind any port for '-%c %s', exiting", optname, optarg); > > } > > =20 > > /** >=20 --=20 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/~dgibson --RzbLOH21PCgmQa1R Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAme2dmcACgkQzQJF27ox 2GdG6A//fahCg+qwVdE+uxaZR0xl+g2ZLafjg5v/seJluL3ZeX47xPyscOWS8Egn sLFbOnKzaq53OJqftwJLbwUf2hrn6b8P7b+Irw0nG1bqakWCRV58kR4UH9vgZT0B BNJp0zUi3UAiJ1jDeTz7/zZbtT3Wo7sg27BRm3Pu6BA10dLPpvM05i+wezkK4o+R O6DH7qDjgjCrnzZeifYGn6vyt6WTUv7EiAIiK/BZNO/eA0uw74BLPfVasreY4SZo mco7pha+cyXWxYitWX2Jrm1tRl0LnJa4XepcissT8+tHPewsUFb0acRyV91K49Dq dXMzPaPgII06SI1FUXCcxwYz8NRuEbJ5JVeEeH4lImv9h16Ay1naYppHJXPw5q8q VhN3uI6Jheroo9go3OTetyiJp5wBomoL8QSBF2ZJE8+PTxLWVUiztdQb1VA9DBsR zQBo6BinomY3T9gYgBeTtnx530Go7FBF1wWqAvgXwDpM9uWhQdqOpyCEEhwSsG+p tBvDCzsUPf/wjwy8ImnhigAabJAPYdeDLBTSzw63gKaBjIfx6TQx8jcFSzhwxw9b BYgHmxe1DpCzjPDQFr1KqaC4YwasoySWZLoBpxFzBnLlQqpUrS2HEydF34GUj1mM idZxedX/mQfQncR0seOeAapJq/qc+TEDrCv8/y14QC7kXfAtW7M= =CTmH -----END PGP SIGNATURE----- --RzbLOH21PCgmQa1R--