Re: [PATCH v7 07/13] netlink, pasta: refactor function pasta_ns_conf()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 13070 bytes --]

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.
> 
> We extract address and route configuration into helper functions
> pasta_conf_addrs() and pasta_conf_routes(), reducing nesting
> and improving readability.
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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.

> 
> ---
> 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(-)
> 
> 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 family
>   * @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 = 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,
>  
>  		len = offsetof(struct req_t, set.a6) + sizeof(req.set.a6);
>  
> -		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 = rta_len;
> -		req.set.a4.rta_l.rta_type = IFA_LOCAL;
> -		memcpy(&req.set.a6.a, addr, sizeof(req.set.a6.a));
> +		req.set.a6.rta_l.rta_type = IFA_LOCAL;
> +		memcpy(&req.set.a6.a, &addr->a6, sizeof(req.set.a6.a));
>  		req.set.a6.rta_a.rta_len = rta_len;
>  		req.set.a6.rta_a.rta_type = IFA_ADDRESS;
>  	} else {
> +		const struct in_addr *v4 = 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 = RTA_LENGTH(sizeof(req.set.a4.l));
>  
>  		len = offsetof(struct req_t, set.a4) + sizeof(req.set.a4);
>  
> -		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 = rta_len;
>  		req.set.a4.rta_l.rta_type = 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 = rta_len;
>  		req.set.a4.rta_a.rta_type = 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 @@
>  
>  #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");
>  }
>  
> +/**
> + * pasta_conf_addrs() - Configure addresses for one address family in namespace
> + * @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 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)
> +{
> +	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 = 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 namespace
> + * @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 = (af == 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 = 0;
> +	unsigned int flags = IFF_UP;
> +	int rc;
>  
>  	rc = 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));
>  
> -	if (c->pasta_conf_ns) {
> -		unsigned int flags = 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 |= 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 = inany_prefix_len(&a->addr,
> -								a->prefix_len);
> -					rc = nl_addr_set(nl_sock_ns,
> -							 c->pasta_ifi, AF_INET,
> -							 inany_v4(&a->addr),
> -							 plen);
> -					if (rc < 0)
> -						break;
> -				}
> -			} else {
> -				rc = nl_addr_dup(nl_sock, c->ifi4,
> -						 nl_sock_ns, c->pasta_ifi,
> -						 AF_INET);
> -			}
> -
> -			if (c->ifi4 == -1 && rc == -ENOTSUP) {
> -				warn("IPv4 not supported, disabling");
> -				c->ifi4 = 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 = nl_route_set_def(nl_sock_ns, c->pasta_ifi,
> -						      AF_INET,
> -						      &c->ip4.guest_gw);
> -			} else {
> -				rc = 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 = 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 = 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 = nl_addr_set(nl_sock_ns,
> -							 c->pasta_ifi,
> -							 AF_INET6, &a->addr.a6,
> -							 a->prefix_len);
> -					if (rc < 0)
> -						break;
> -				}
> -			} else {
> -				rc = 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 = nl_route_set_def(nl_sock_ns, c->pasta_ifi,
> -						      AF_INET6,
> -						      &c->ip6.guest_gw);
> -			} else {
> -				rc = nl_route_dup(nl_sock, c->ifi6,
> -						  nl_sock_ns, c->pasta_ifi,
> -						  AF_INET6);
> -			}
> -
> -			if (c->ifi6 == -1 && rc == -ENOTSUP) {
> -				warn("IPv6 not supported, disabling");
> -				c->ifi6 = 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 |= 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 = pasta_conf_addrs(c, AF_INET, c->ifi4, c->ip4.no_copy_addrs);
> +	if (c->ifi4 == -1 && rc == -ENOTSUP) {
> +		warn("IPv4 not supported, disabling");
> +		c->ifi4 = 0;
> +	} else if (rc < 0) {
> +		die("Couldn't set IPv4 address(es): %s", strerror_(-rc));
> +	} else if (c->ifi4) {
> +		rc = 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:
>  
> -	proto_update_l2_buf(c->guest_mac);
> +	if (!c->ifi6)
> +		return;
> +
> +	rc = 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 = 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 = pasta_conf_addrs(c, AF_INET6, c->ifi6, c->ip6.no_copy_addrs);
> +	if (c->ifi6 == -1 && rc == -ENOTSUP) {
> +		warn("IPv6 not supported, disabling");
> +		c->ifi6 = 0;
> +	} else if (rc < 0) {
> +		die("Couldn't set IPv6 address(es): %s", strerror_(-rc));
> +	} else {
> +		rc = 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));
> +	}
>  }
>  
>  /**
> -- 
> 2.52.0
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]