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=202602 header.b=IMEE2B6K; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 2A8415A0262 for ; Tue, 26 May 2026 04:03:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1779761021; bh=3SDzlhDCY1Daa64/XGpSZxRpTCCD3ObEvFoVFQSKCdE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IMEE2B6KKnA/hcf3AOHnm37dvpHSgQ8ATj+on63xX+SYy0escNrkBFEQY2hvs0V+U jJH0LiUEk9DC8Kvj+nzWgK1nbSAs/Uxgb3/58hIVD0pRcw5sdoq+URn84BBYy6ExaD 8c+TAIkLvGJ9UNb78nZKoKlJ2reRurmvpaXvgbDORhKz33fYHJF79w8RU/+2HoIFtn sXA15lzTmiJBC13xg0AMbGP2dXsDNmJ09RQLLmo+g3OisBPK6vU7kbvo9vDGXBquNq kwv9t9Oje5l6NFI+s+TTkYzJAStWk4WVIwbo/LLxvbUUok3NqVM07xqOIjETeglzYY NhpFiDJ67nWYA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gPbdT0Tfxz4wCP; Tue, 26 May 2026 12:03:41 +1000 (AEST) Date: Tue, 26 May 2026 11:58:23 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v7 07/13] netlink, pasta: refactor function pasta_ns_conf() Message-ID: References: <20260413005319.3295910-1-jmaloy@redhat.com> <20260413005319.3295910-8-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="fFKrS7gkHX6Z8Jj8" Content-Disposition: inline In-Reply-To: <20260413005319.3295910-8-jmaloy@redhat.com> Message-ID-Hash: R4VQDN5QOG7LPLJWBH6YCRIOMDIXMQUY X-Message-ID-Hash: R4VQDN5QOG7LPLJWBH6YCRIOMDIXMQUY 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: --fFKrS7gkHX6Z8Jj8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Apr 12, 2026 at 08:53:13PM -0400, Jon Maloy wrote: > After the previous changes in this series it becomes possible > to simplify the pasta_ns_conf() function. >=20 > We extract address and route configuration into helper functions > pasta_conf_addrs() and pasta_conf_routes(), reducing nesting > and improving readability. >=20 > To allow pasta_conf_addrs() to handle both address families > uniformly, we change nl_addr_set() to take a union inany_addr pointer > instead of void pointer, moving the address family handling into > the function itself. >=20 > We also fix a bug where the IPv6 code path incorrectly wrote to > req.set.a4.rta_l.rta_type instead of req.set.a6.rta_l.rta_type. >=20 > Signed-off-by: Jon Maloy Reviewed-by: David Gibson Very happy to see the confusing gotos gone. Couple of minor suggestions below, plus noted a pre-existing oddity that might be worth altering / commenting as a separate matter. >=20 > --- > 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. > --- > netlink.c | 17 ++-- > netlink.h | 4 +- > pasta.c | 232 ++++++++++++++++++++++++++---------------------------- > 3 files changed, 123 insertions(+), 130 deletions(-) >=20 > diff --git a/netlink.c b/netlink.c > index 3f5a812..3d25212 100644 > --- a/netlink.c > +++ b/netlink.c > @@ -863,15 +863,15 @@ int nl_addr_get_ll(int s, unsigned int ifi, struct = in6_addr *addr) > * nl_addr_set() - Set IP addresses for given interface and address fami= ly > * @s: Netlink socket > * @ifi: Interface index > - * @af: Address family > * @addr: Global address to set > * @prefix_len: Mask or prefix length to set > * > * Return: 0 on success, negative error code on failure > */ > -int nl_addr_set(int s, unsigned int ifi, sa_family_t af, > - const void *addr, int prefix_len) > +int nl_addr_set(int s, unsigned int ifi, const union inany_addr *addr, > + int prefix_len) > { > + sa_family_t af =3D inany_af(addr); > struct req_t { > struct nlmsghdr nlh; > struct ifaddrmsg ifa; > @@ -905,21 +905,22 @@ int nl_addr_set(int s, unsigned int ifi, sa_family_= t af, > =20 > len =3D offsetof(struct req_t, set.a6) + sizeof(req.set.a6); > =20 > - memcpy(&req.set.a6.l, addr, sizeof(req.set.a6.l)); > + memcpy(&req.set.a6.l, &addr->a6, sizeof(req.set.a6.l)); > req.set.a6.rta_l.rta_len =3D rta_len; > - req.set.a4.rta_l.rta_type =3D IFA_LOCAL; > - memcpy(&req.set.a6.a, addr, sizeof(req.set.a6.a)); > + req.set.a6.rta_l.rta_type =3D IFA_LOCAL; > + memcpy(&req.set.a6.a, &addr->a6, sizeof(req.set.a6.a)); > req.set.a6.rta_a.rta_len =3D rta_len; > req.set.a6.rta_a.rta_type =3D IFA_ADDRESS; > } else { > + const struct in_addr *v4 =3D inany_v4(addr); I suspect the static checkers won't be able to deduce that this cannot be NULL, because of the earlier inany_af() call - in a sense you're checking the address family twice. I think it would be more elegant (and avoid possible checker false positives) to have the if based on the return value from inany_v4(), then set req.ifa.ifa_family to constant values in each branch > size_t rta_len =3D RTA_LENGTH(sizeof(req.set.a4.l)); > =20 > len =3D offsetof(struct req_t, set.a4) + sizeof(req.set.a4); > =20 > - memcpy(&req.set.a4.l, addr, sizeof(req.set.a4.l)); > + memcpy(&req.set.a4.l, v4, sizeof(req.set.a4.l)); > req.set.a4.rta_l.rta_len =3D rta_len; > req.set.a4.rta_l.rta_type =3D IFA_LOCAL; > - memcpy(&req.set.a4.a, addr, sizeof(req.set.a4.a)); > + memcpy(&req.set.a4.a, v4, sizeof(req.set.a4.a)); > req.set.a4.rta_a.rta_len =3D rta_len; > req.set.a4.rta_a.rta_type =3D IFA_ADDRESS; > } > diff --git a/netlink.h b/netlink.h > index 3af6d58..ec859b7 100644 > --- a/netlink.h > +++ b/netlink.h > @@ -22,8 +22,8 @@ int nl_route_dup(int s_src, unsigned int ifi_src, > int nl_addr_get_all(struct ctx *c, int s, unsigned int ifi, sa_family_t = af); > bool nl_neigh_mac_get(int s, const union inany_addr *addr, int ifi, > unsigned char *mac); > -int nl_addr_set(int s, unsigned int ifi, sa_family_t af, > - const void *addr, int prefix_len); > +int nl_addr_set(int s, unsigned int ifi, const union inany_addr *addr, > + int prefix_len); > int nl_addr_get_ll(int s, unsigned int ifi, struct in6_addr *addr); > int nl_addr_set_ll_nodad(int s, unsigned int ifi); > int nl_addr_dup(int s_src, unsigned int ifi_src, > diff --git a/pasta.c b/pasta.c > index b3936f5..f949dc2 100644 > --- a/pasta.c > +++ b/pasta.c > @@ -46,6 +46,7 @@ > =20 > #include "util.h" > #include "passt.h" > +#include "conf.h" > #include "isolation.h" > #include "netlink.h" > #include "log.h" > @@ -303,13 +304,69 @@ 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 addresses from c->addrs; if false, copy from h= ost > + * > + * 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) > +{ > + const struct guest_addr *a; > + > + if (!ifi) > + return 0; > + > + if (!no_copy) > + return nl_addr_dup(nl_sock, ifi, nl_sock_ns, c->pasta_ifi, af); > + > + for_each_addr(a, c->addrs, c->addr_count, af) { > + int rc; > + > + rc =3D nl_addr_set(nl_sock_ns, c->pasta_ifi, &a->addr, > + inany_prefix_len(&a->addr, a->prefix_len)); > + if (rc < 0) > + return rc; > + } > + 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 =3D (af =3D=3D AF_INET) ? > + (const void *)&c->ip4.guest_gw : (const void *)&c->ip6.guest_gw; > + > + if (!ifi) > + return 0; > + > + if (no_copy) It'd be nicer to have the !no_copy branch first for consistency with pasta_conf_addrs(). > + return nl_route_set_def(nl_sock_ns, c->pasta_ifi, af, gw); > + > + return nl_route_dup(nl_sock, ifi, nl_sock_ns, c->pasta_ifi, af); > +} > + > /** > * 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,127 +385,62 @@ 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; > - const struct guest_addr *a; > - int plen; > - > - 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) { > - for_each_addr(a, c->addrs, c->addr_count, AF_INET) { > - plen =3D inany_prefix_len(&a->addr, > - a->prefix_len); > - rc =3D nl_addr_set(nl_sock_ns, > - c->pasta_ifi, AF_INET, > - inany_v4(&a->addr), > - plen); > - if (rc < 0) > - break; > - } > - } 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) { > - for_each_addr(a, c->addrs, c->addr_count, AF_INET6) { > - rc =3D nl_addr_set(nl_sock_ns, > - c->pasta_ifi, > - AF_INET6, &a->addr.a6, > - a->prefix_len); > - if (rc < 0) > - break; > - } > - } 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; Pre-existing, but this looks weird. Why are we setting a property for IPv6 next to the IPv4 configuration code? And does IFF_NOARP actually affect IPv6 DAD? I thought that was the IFA_F_NODAD flag which we use elsewhere. > + > + nl_link_set_flags(nl_sock_ns, c->pasta_ifi, flags, flags); > + > + /* IPv4 configuration */ > + 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): %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): %s", strerror_(-rc)); > } > -ipv6_done: > =20 > - proto_update_l2_buf(c->guest_mac); > + if (!c->ifi6) > + return; > + > + 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; > + } else if (rc < 0) { > + die("Couldn't set IPv6 address(es): %s", strerror_(-rc)); > + } else { > + 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): %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 --fFKrS7gkHX6Z8Jj8 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoU/jEACgkQzQJF27ox 2GfMSQ/9EDFKESMhu5lkp0ERfyP+oI3mh/muXONgT8vfneaC7rc9rYtSJ5ohZY3M MLXO0jZjQUj1zZu9jcW3hwapj+AxPkSawr8mWEHPS/7pSZZ6XPUtP1PQQXcJeOTZ NxJt0L7kEs8PpmtupiZWCR9hbFQuSkqeqk1CewCiN/jZLQdogbj/KFZq2utvCTlc YZOXujbSyrFheNMyfIRlnPU8gqDwKMUlff3f2aEDdnd32Sf91RPEw+0eE1qBmBB4 8csXqmCY2sSJpoBHBgyta/HNFUIVKjb/rQmgVKAU+RikVvUjRbX2qO0cc9ltL4gl 5vMj3lhl8uRK3akQZt83+8l52EZuNeitizwoUNl9C9VObn4ssFtCfUOiaHhMtSkl vrU5itjGqLKYP35miz5aVJedGMvn+6IGALehj7/8mEMXWO4E5w/NJb9bV4MZbeBh 9x4kfKIY/M9Y6SN0afMpI5GP6CclbvpQEuH220hSwPQQVMZTFNguYWVI8L9K+IeA LPOCnIXibcMmKW5qTt9wobOcV1W+EUurkGpxPXyHWppQZ0urcKA7d/008Bel/u4B yE96Ouud2M9L4oCmPQ8kMuClOaCf8ouu6QSsb1cqWwC3Pu3KrpwtsqZ5/H/UWMWS 0mQyUDvV3Qi1JmkwnVvPU6DFWEb7wtXCAhfDVMtsASiadgd9DaA= =l9Sv -----END PGP SIGNATURE----- --fFKrS7gkHX6Z8Jj8--