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=dwjsVSIE; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 18D795A0265 for ; Tue, 24 Mar 2026 06:49:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1774331387; bh=vBJ8wc3dFVRUFLM1EriuXApenHPGES8TzjVzX4W7hKU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dwjsVSIEcQE7yJDVN/MlegqUG3HoA6wfQeYDg2F9XaDTmHDiTce77xigGWyteHyT5 zMHdEKvmMjcO9WybUYkLglwqletlAvsAVgAsyzv4CXIGmQE2GGNGpMzc0Wr9bhiY3e xcUQq9bOkUCmhOhlesaFFdfJ2qEdszxPyldDCHs8Xb9sCmuEGnq6WEY01l0LZjiXJb 4Ry2YfIT7GVX/VzYPCRmiaTlHoMuHHi/xgoZYB2l9ra/nqRbTi5lfOZ6EIe0DVPruO T635tRIZUbBPzbxYvwC9MDtBo4xuofJKurwnB6p/EDH8xKgfF6zquxHWDKsVk7UthC VZJRmS9l2ntiQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ffzdR30Mrz4wBF; Tue, 24 Mar 2026 16:49:47 +1100 (AEDT) Date: Tue, 24 Mar 2026 16:49:41 +1100 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v6 07/13] ip: refactor function pasta_ns_conf() Message-ID: References: <20260322004333.365713-1-jmaloy@redhat.com> <20260322004333.365713-8-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Hbc8N4wpEJtIHVY+" Content-Disposition: inline In-Reply-To: <20260322004333.365713-8-jmaloy@redhat.com> Message-ID-Hash: NLYH3QAHXZFL5AS2XIGNEFYNXPVTRBT2 X-Message-ID-Hash: NLYH3QAHXZFL5AS2XIGNEFYNXPVTRBT2 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, dgibson@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: --Hbc8N4wpEJtIHVY+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Nit: starting the subject with "ip:" when the patch doesn't touch ip.c doesn't make a lot of sense. I think "netlink, pasta:" would make more sense. On Sat, Mar 21, 2026 at 08:43:27PM -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 > --- > netlink.c | 13 +-- > netlink.h | 2 +- > pasta.c | 232 +++++++++++++++++++++++++++--------------------------- > 3 files changed, 124 insertions(+), 123 deletions(-) >=20 > diff --git a/netlink.c b/netlink.c > index 2727eec..fcdc983 100644 > --- a/netlink.c > +++ b/netlink.c > @@ -870,7 +870,7 @@ int nl_addr_get_ll(int s, unsigned int ifi, struct in= 6_addr *addr) > * Return: 0 on success, negative error code on failure > */ > int nl_addr_set(int s, unsigned int ifi, sa_family_t af, You should be able to extract the family from the inany and remove this parameter, no? (Pre-existing: arguably we should set ifa_scope based on the address, rather than assuming RT_SCOPE_UNIVERSE, too) > - const void *addr, int prefix_len) > + const union inany_addr *addr, int prefix_len) > { > struct req_t { > struct nlmsghdr nlh; > @@ -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); > 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..26f5ef7 100644 > --- a/netlink.h > +++ b/netlink.h > @@ -23,7 +23,7 @@ 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); > + 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 6307c65..b8d7cf4 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,73 @@ 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) Eventually, I think it would be nicer to set all the addresses (v4 and v6) from a single loop. However, I can see that this is a useful interim step, because doing that straight away would make things a bit awkward in the caller. > + * @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, af) { > + int rc; > + > + /* Skip link-local, kernel auto-configures */ Hrm... conceptually it's an array of *guest* addresses, even if take =66rom the host. So link-local addresses should probably be excluded =66rom the list before they're inserted, rather than when we're reading it. > + if (a->flags & CONF_ADDR_LINKLOCAL) > + continue; > + > + rc =3D nl_addr_set(nl_sock_ns, c->pasta_ifi, af, &a->addr, > + inany_prefix_len(a)); > + 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) > + 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,123 +389,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; > - > - 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, AF_INET) { > - rc =3D nl_addr_set(nl_sock_ns, > - c->pasta_ifi, AF_INET, > - inany_v4(&a->addr), > - inany_prefix_len(a)); > - 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, 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)); > - } > - } > + if (!c->pasta_conf_ns) > + goto done; > + > + 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); > + > + /* 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)); > + } > + > + if (!c->ifi6) > + goto done; > + > + 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)); > } > -ipv6_done: > =20 > +done: > proto_update_l2_buf(c->guest_mac); Could we move this up before the if (c->pasta_conf_ns), rather than having an awkward goto? > } > =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 --Hbc8N4wpEJtIHVY+ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmnCJfQACgkQzQJF27ox 2GchNg//YUeGhvvrS1O/7fAo6IRcQOU/46KZOBuXfFMrTwaN29hCzgVrV8l/xU2u +633kvFxU5b/M3LTnhVO/YpeWQCpnGhWTrvtluIEavlfqPHHgbYiHQwshSmK3Aq6 Kn9PA8x32t99efIgNEtFXsaFS6GRpRQRwfuTYTVMWVy2FKPneNii6hdiatxKgZ1a Ghs3mOzPDVoJp18Do6mFM/nbuNk/glfYkn7L61NGDYY5foFng0aEb+B6EA8aUk6u weKVcrSyJCb/seXt8CTNdfp6UKPZT3WaVjDCO0al6nfVgMt1/dfM72neWl6vi5LM eooodFsvbh8IpTdzqxPtDyzMmXwY8ykvx+9t9BOO+xiPcdKOr6Z096XjLHJGs/Cz G6P4XwW9Ie6+Irl1/SKuPwzqiCVLRLJH6+FD7RT0QeeJRhkewb+1N+qk3TARPK9C PVAq1SFA4Mh2BCjJmtLilgHESJwa0ltobpoqL+aEdckD6/16lSQMl9A8iueSyBRb vqz+m2v5DOETjhsdCHPof+BJG4Wkpa20+Ng0dGbiWG+RFJtfXOATm4kHDA4sjZqO N6XuZ7+U7sqK+qt3kUvUKRVoPOmLu7vkcOSdFnX4NoLqxrA9KGm9+V6fxDMOgIS0 kOCwjf6bfse1bxNVMbiWj9K4JGShZu8+mh5Iv9ImvBvwljDs/ho= =WC/k -----END PGP SIGNATURE----- --Hbc8N4wpEJtIHVY+--