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=202606 header.b=idsp7JgW; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 3525A5A0623 for ; Mon, 13 Jul 2026 07:39:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1783921140; bh=uzkcN/DUrBTxQqgB1DlkJ3RIVmsWw+CfDHZUrKrpdCc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=idsp7JgWXMG5Jb8rQ1mcUEQAPDvuCu/1fvh3eBtwfSSeYYJT3Au5kylURBxXR9ctJ o7VzXFBuAh4ghJF8ghl96u5SmnnoXO76x/jVcze69J7EPUlJO4n0y/vvfefxXCvGUU UQ4cINVKQ8OyDF/3x85nxaFnS+z+d85q/oM1uLHFPYExfUreH3AEirX3OgTj3LIUop uwUc5gl5EtKqyPaxWnBCOQ+W9iMSgvFimsSyUZUXYO1a90gvn//ZXIUbwjPpYQLBgQ Wz1IXusfXBw+kl8FdUdsNn4W313fbvehRpzHNceqzcn3cX3FGFOfOL2mQ6JOpVO4Im 7/qmJ4gKW4UpA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gzB7m286Cz4wHr; Mon, 13 Jul 2026 15:39:00 +1000 (AEST) Date: Mon, 13 Jul 2026 14:29:03 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v9] pasta: Refactor pasta_ns_conf() to reduce nesting Message-ID: References: <20260710235309.1789029-1-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="hGHQmnphmYnigFXU" Content-Disposition: inline In-Reply-To: <20260710235309.1789029-1-jmaloy@redhat.com> Message-ID-Hash: OB2Y7Z5F5JNVYENLTVM7BORDLGXQOXPO X-Message-ID-Hash: OB2Y7Z5F5JNVYENLTVM7BORDLGXQOXPO 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: sbrivio@redhat.com, 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: --hGHQmnphmYnigFXU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 10, 2026 at 07:53:09PM -0400, Jon Maloy wrote: > We extract address and route configuration into two new helper > functions pasta_conf_addrs() and pasta_conf_routes(), and replace > the outer if (c->pasta_conf_ns) block with an early return. This > reduces indentation and eliminates the goto labels. >=20 > There is no functional change in this commit. >=20 > Signed-off-by: Jon Maloy On the whole looks much nicer, but a couple of comments below. > --- > v7: -Removed redundant argument 'af' in nl_addr_set() > -Removed redundant label and 'goto's in pasta_ns_conf() > -Since I excluded addition of all LINKLOCAL addresses from host > address array in a previous commit I can now omit this test > in pasta_conf_addrs() as suggested by David. > -Here is the example of agnostic usage of inany_prefix_len() > I referred to in a previous commit. >=20 > v8: -Do prefix_len conversion inside nl_addr_set() instead of in > the calling function. > -In nl_addr_set(), branch on inany_prefix_v4() return value instead > of inany_af() to avoid redundant AF check and potential static > checker false positives. (David) > -Swap branch order in pasta_conf_routes() for consistency with > pasta_conf_addrs(). (David) > -Defer investigation of pre-existing IFF_NOARP / DAD interaction > oddity noted by David. >=20 > v9: - Broke out as standalone from multi-addr series. > - Adapted to code conditions before the introduction of > the unified address type. > --- > pasta.c | 226 +++++++++++++++++++++++++++++--------------------------- > 1 file changed, 117 insertions(+), 109 deletions(-) >=20 > diff --git a/pasta.c b/pasta.c > index 4e7ee542..fa9a341e 100644 > --- a/pasta.c > +++ b/pasta.c > @@ -303,13 +303,72 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t= gid, > die_perror("Failed to join network namespace"); > } > =20 > +/** > + * pasta_conf_addrs() - Configure addresses for one address family in na= mespace > + * @c: Execution context > + * @af: Address family (AF_INET or AF_INET6) > + * @ifi: Host interface index for this address family > + * @no_copy: If true, set configured address; if false, copy from host > + * > + * Return: 0 on success, negative error code on failure > + */ > +static int pasta_conf_addrs(struct ctx *c, sa_family_t af, int ifi, > + bool no_copy) > +{ > + if (!ifi) > + return 0; > + > + if (!no_copy) > + return nl_addr_dup(nl_sock, ifi, nl_sock_ns, c->pasta_ifi, af); > + > + if (af =3D=3D AF_INET) { I think this also needs to check !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr). > + return nl_addr_set(nl_sock_ns, c->pasta_ifi, AF_INET, > + &c->ip4.addr, c->ip4.prefix_len); > + } > + > + if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr)) > + return nl_addr_set(nl_sock_ns, c->pasta_ifi, AF_INET6, > + &c->ip6.addr, 64); > + > + return 0; > +} > + > +/** > + * pasta_conf_routes() - Configure routes for one address family in name= space > + * @c: Execution context > + * @af: Address family (AF_INET or AF_INET6) > + * @ifi: Host interface index for this address family > + * @no_copy: If true, set default route; if false, copy routes from host > + * > + * Return: 0 on success, negative error code on failure > + */ > +static int pasta_conf_routes(struct ctx *c, sa_family_t af, int ifi, > + bool no_copy) > +{ > + const void *gw; > + > + if (af =3D=3D AF_INET) > + gw =3D &c->ip4.guest_gw; > + else > + gw =3D &c->ip6.guest_gw; > + > + if (!ifi) > + return 0; > + > + if (!no_copy) > + return nl_route_dup(nl_sock, ifi, nl_sock_ns, c->pasta_ifi, af); > + > + return nl_route_set_def(nl_sock_ns, c->pasta_ifi, af, gw); > +} > + > /** > * pasta_ns_conf() - Set up loopback and tap interfaces in namespace as = needed > * @c: Execution context > */ > void pasta_ns_conf(struct ctx *c) > { > - int rc =3D 0; > + unsigned int flags =3D IFF_UP; > + int rc; > =20 > rc =3D nl_link_set_flags(nl_sock_ns, 1 /* lo */, IFF_UP, IFF_UP); > if (rc < 0) > @@ -328,116 +387,65 @@ void pasta_ns_conf(struct ctx *c) > die("Couldn't set MAC address in namespace: %s", > strerror_(-rc)); > =20 > - if (c->pasta_conf_ns) { > - unsigned int flags =3D IFF_UP; > - > - if (c->mtu) > - nl_link_set_mtu(nl_sock_ns, c->pasta_ifi, c->mtu); > - > - if (c->ifi6) /* Avoid duplicate address detection on link up */ > - flags |=3D IFF_NOARP; > - > - nl_link_set_flags(nl_sock_ns, c->pasta_ifi, flags, flags); > - > - if (c->ifi4) { > - if (c->ip4.no_copy_addrs) { > - rc =3D nl_addr_set(nl_sock_ns, c->pasta_ifi, > - AF_INET, > - &c->ip4.addr, > - c->ip4.prefix_len); > - } else { > - rc =3D nl_addr_dup(nl_sock, c->ifi4, > - nl_sock_ns, c->pasta_ifi, > - AF_INET); > - } > - > - if (c->ifi4 =3D=3D -1 && rc =3D=3D -ENOTSUP) { > - warn("IPv4 not supported, disabling"); > - c->ifi4 =3D 0; > - goto ipv4_done; > - } > - > - if (rc < 0) { > - die("Couldn't set IPv4 address(es) in namespace: %s", > - strerror_(-rc)); > - } > - > - if (c->ip4.no_copy_routes) { > - rc =3D nl_route_set_def(nl_sock_ns, c->pasta_ifi, > - AF_INET, > - &c->ip4.guest_gw); > - } else { > - rc =3D nl_route_dup(nl_sock, c->ifi4, nl_sock_ns, > - c->pasta_ifi, AF_INET); > - } > - > - if (rc < 0) { > - die("Couldn't set IPv4 route(s) in guest: %s", > - strerror_(-rc)); > - } > - } > -ipv4_done: > - > - if (c->ifi6) { > - rc =3D nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, > - &c->ip6.addr_ll_seen); > - if (rc < 0) { > - warn("Can't get LL address from namespace: %s", > - strerror_(-rc)); > - } > - > - rc =3D nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi); > - if (rc < 0) { > - warn("Can't set nodad for LL in namespace: %s", > - strerror_(-rc)); > - } > - > - /* We dodged DAD: re-enable neighbour solicitations */ > - nl_link_set_flags(nl_sock_ns, c->pasta_ifi, > - 0, IFF_NOARP); > - > - if (c->ip6.no_copy_addrs) { > - if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr)) { > - rc =3D nl_addr_set(nl_sock_ns, > - c->pasta_ifi, AF_INET6, > - &c->ip6.addr, 64); > - } > - } else { > - rc =3D nl_addr_dup(nl_sock, c->ifi6, > - nl_sock_ns, c->pasta_ifi, > - AF_INET6); > - } > - > - if (rc < 0) { > - die("Couldn't set IPv6 address(es) in namespace: %s", > - strerror_(-rc)); > - } > - > - if (c->ip6.no_copy_routes) { > - rc =3D nl_route_set_def(nl_sock_ns, c->pasta_ifi, > - AF_INET6, > - &c->ip6.guest_gw); > - } else { > - rc =3D nl_route_dup(nl_sock, c->ifi6, > - nl_sock_ns, c->pasta_ifi, > - AF_INET6); > - } > - > - if (c->ifi6 =3D=3D -1 && rc =3D=3D -ENOTSUP) { > - warn("IPv6 not supported, disabling"); > - c->ifi6 =3D 0; > - goto ipv6_done; > - } > - > - if (rc < 0) { > - die("Couldn't set IPv6 route(s) in guest: %s", > - strerror_(-rc)); > - } > - } > + proto_update_l2_buf(c->guest_mac); > + > + if (!c->pasta_conf_ns) > + return; > + > + if (c->mtu) > + nl_link_set_mtu(nl_sock_ns, c->pasta_ifi, c->mtu); > + > + if (c->ifi6) /* Avoid duplicate address detection on link up */ > + flags |=3D IFF_NOARP; > + > + nl_link_set_flags(nl_sock_ns, c->pasta_ifi, flags, flags); > + I realise it puts some of the nesting back in, but I think you still need the if (c->ifi4) around the next chunk - a configuration with only IPv6 configured is certainly supported. > + rc =3D pasta_conf_addrs(c, AF_INET, c->ifi4, c->ip4.no_copy_addrs); > + if (c->ifi4 =3D=3D -1 && rc =3D=3D -ENOTSUP) { > + warn("IPv4 not supported, disabling"); > + c->ifi4 =3D 0; > + } else if (rc < 0) { > + die("Couldn't set IPv4 address(es) in namespace: %s", > + strerror_(-rc)); > + } else if (c->ifi4) { > + rc =3D pasta_conf_routes(c, AF_INET, c->ifi4, > + c->ip4.no_copy_routes); > + if (rc < 0) > + die("Couldn't set IPv4 route(s) in guest: %s", > + strerror_(-rc)); > } > -ipv6_done: > =20 > - proto_update_l2_buf(c->guest_mac); > + if (!c->ifi6) > + return; For symmetry with IPv4 I'd put this chunk within the if, rather than using an early return here. > + > + rc =3D nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, > + &c->ip6.addr_ll_seen); > + if (rc < 0) > + warn("Can't get LL address from namespace: %s", > + strerror_(-rc)); > + > + rc =3D nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi); > + if (rc < 0) > + warn("Can't set nodad for LL in namespace: %s", > + strerror_(-rc)); > + > + /* We dodged DAD: re-enable neighbour solicitations */ > + nl_link_set_flags(nl_sock_ns, c->pasta_ifi, 0, IFF_NOARP); > + > + rc =3D pasta_conf_addrs(c, AF_INET6, c->ifi6, c->ip6.no_copy_addrs); > + if (c->ifi6 =3D=3D -1 && rc =3D=3D -ENOTSUP) { > + warn("IPv6 not supported, disabling"); > + c->ifi6 =3D 0; > + return; > + } else if (rc < 0) { > + die("Couldn't set IPv6 address(es) in namespace: %s", > + strerror_(-rc)); > + } > + > + rc =3D pasta_conf_routes(c, AF_INET6, c->ifi6, c->ip6.no_copy_routes); > + if (rc < 0) > + die("Couldn't set IPv6 route(s) in guest: %s", > + strerror_(-rc)); > } > =20 > /** > --=20 > 2.52.0 >=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 --hGHQmnphmYnigFXU Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpUaYYACgkQzQJF27ox 2GeK/Q//fVb/3hRNDkYQwYJyH/7XnAXsQL0pDsL9W2OI1KXxqY03Tqn0eFay6Y2y rr7TfeYJfg46Mq8hGu9ILj2hsOe79sGOkZRfcpqR9arhfmtSjK0WsqKgac+0ccZT k7Xf4eW8ZDSsIsTRBOd9a71FDi+6NhHn7J311AsetemxOItfL0B9cG8i2GuVHv3J /v3y0rwFMQSZvHoQ9n0VCPAB9Qjo+iXWYmyrwOLgS/igPNjQqj+3oasXOmjCqmge 7xQzf2v1+N1gBw0hJLBjnLcnVeSyj1v/2v2t2FEpPBFe3IHY4izmEspCDjzTT92/ Dhwz5u9dJR7t2DA9XFCp2gyEu1losTdxnHdiXUsuGGdWp0xSWUzqtUxMVGdL+c77 2oJ8CzgvdOXdnAPkXPDXYbE1p2bwi7TC4NKtc3tBkiPraECMBFGnXs3FYpWRJA/C n0WORkZkp0RjG/Y2RTcWYPk9mysbewHBpiZfNxqyNe2CGNdeVyssWkFYdy9DQ9xt p6uulm9iSxlI96V+A9/Mrr06s3fB/mvj210/1XDHhfpmsOYktV90KVOnOjPCR5d8 RNkpIn5TrjGiTsmrUNcvjXMUGGFBTz8zhsSXcy4pcO4qOMg/L3kujYN0oulwqKub ZBKj/hGxRae/LPFfHP4Sxxtc59qOmoTD18aCu/jjX1/LFIjhpj8= =Fv6z -----END PGP SIGNATURE----- --hGHQmnphmYnigFXU--