Re: [PATCH v2 7/9] pasta: Unify address configuration paths using address array

[Stefano Brivio <sbrivio@redhat.com>]

On Sun, 18 Jan 2026 17:16:10 -0500
Jon Maloy <jmaloy@redhat.com> wrote:

> Until now, pasta has maintained two separate code paths for
> setting up addresses in the guest namespace:
> 
> - "copy addrs" path: When --config-net is used without -a, pasta
>    would call nl_addr_dup() to copy addresses directly from the host
>    template interface to the guest.
> 
> - "no_copy_addrs" path: When -a is specified, pasta would iterate
>    through the addrs[] array and call nl_addr_set() for each entry.
> 
> This commit unifies these paths by adding a new nl_addr_get_all()
> function that retrieves all addresses from an interface into the
> addrs[] array, marking them with INANY_ADDR_HOST flag.
> 
> We modify conf_ip4() and conf_ip6() to always populate the address
> array, either from command-line -a options or from host interface
> discovery.
> 
> This gives us a single code path for all address configuration scenarios
> and a more consistent address handling regardless of source.

I'm not entirely sure why this is needed, at least in the context of
this series or of the netlink monitor, but if it really is, it's
probably going to be a bit more complicated than this.

The main aspect that's missing here is that nl_addr_dup() took care of
copying most 'ifa_flags', at least those that made sense to copy, and
also adding a fundamental one for IPv6, IFA_F_NODAD, see d03c4e20202b
("netlink: Use IFA_F_NODAD also while duplicating addresses from the host").

> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c    | 82 ++++++++++++++++++++++++++++-------------------------
>  netlink.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  netlink.h |  5 ++++
>  passt.h   |  4 ---
>  pasta.c   | 36 ++++++++++--------------
>  5 files changed, 147 insertions(+), 64 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 32a754d..22c2222 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -745,6 +745,8 @@ static int conf_addr_prefix_len(char *arg, union inany_addr *addr,
>   */
>  static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
>  {
> +	int i;
> +
>  	if (!ifi)
>  		ifi = nl_get_ext_if(nl_sock, AF_INET);
>  
> @@ -764,25 +766,25 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
>  	}
>  
>  	if (!ip4->addr_count) {
> -		struct in_addr addr;
> -		int prefix_len = 0;
> -		int rc = nl_addr_get(nl_sock, ifi, AF_INET,
> -				     &addr, &prefix_len, NULL);
> -		if (rc < 0) {
> +		int count = nl_addr_get_all(nl_sock, ifi, AF_INET,
> +					    ip4->addrs, IP4_MAX_ADDRS, NULL);
> +		if (count < 0) {
>  			debug("Couldn't discover IPv4 address: %s",
> -			      strerror_(-rc));
> +			      strerror_(-count));
> +			return 0;
> +		}
> +		if (count == 0) {
> +			debug("No IPv4 address on interface");
>  			return 0;
>  		}
> -		ip4->addrs[0].addr = inany_from_v4(addr);
> -		ip4->addrs[0].prefix_len = prefix_len;
> -		ip4->addrs[0].flags = INANY_ADDR_HOST;
> -		ip4->addr_count = 1;
> +		ip4->addr_count = count;
>  	}
>  
> -	if (!ip4->addrs[0].prefix_len) {
> -		const struct in_addr *a4 = inany_v4(&ip4->addrs[0].addr);
> +	for (i = 0; i < (int)ip4->addr_count; i++) {

clang-tidy doesn't like this because:

/home/sbrivio/passt/conf.c:792:18: error: redundant cast to the same type [google-readability-casting,-warnings-as-errors]
  792 |         for (i = 0; i < (int)ip4->addr_count; i++) {
      |                         ^
note: this fix will not be applied because it overlaps with another fix
/home/sbrivio/passt/conf.c:792:18: error: redundant explicit casting to the same type 'int' as the sub-expression, remove this casting [readability-redundant-casting,-warnings-as-errors]
/home/sbrivio/passt/passt.h:88:6: note: source type originates from referencing this member
   88 |         int addr_count;
      |         ~~~ ^

> +		const struct in_addr *a4 = inany_v4(&ip4->addrs[i].addr);
>  
> -		ip4->addrs[0].prefix_len = ip4_default_prefix_len(a4);
> +		if (!ip4->addrs[i].prefix_len)
> +			ip4->addrs[i].prefix_len = ip4_default_prefix_len(a4);
>  	}
>  
>  	ip4->addr_seen = *inany_v4(&ip4->addrs[0].addr);
> @@ -807,7 +809,7 @@ static void conf_ip4_local(struct ip4_ctx *ip4)
>  	ip4->addrs[0].prefix_len = IP4_LL_PREFIX_LEN;
>  	ip4->addr_count = 1;
>  
> -	ip4->no_copy_addrs = ip4->no_copy_routes = true;
> +	ip4->no_copy_routes = true;
>  }
>  
>  /**
> @@ -819,7 +821,6 @@ static void conf_ip4_local(struct ip4_ctx *ip4)
>   */
>  static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
>  {
> -	int prefix_len = 0;
>  	int rc;
>  
>  	if (!ifi)
> @@ -839,18 +840,29 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
>  		}
>  	}
>  
> -	rc = nl_addr_get(nl_sock, ifi, AF_INET6,
> -			 ip6->addr_count ? NULL : &ip6->addrs[0].addr.a6,
> -			 &prefix_len, &ip6->our_tap_ll);
> -	if (rc < 0) {
> -		debug("Couldn't discover IPv6 address: %s", strerror_(-rc));
> -		return 0;
> -	}
> -
>  	if (!ip6->addr_count) {
> -		ip6->addrs[0].prefix_len = prefix_len ? prefix_len : 64;
> -		ip6->addrs[0].flags = INANY_ADDR_HOST;
> -		ip6->addr_count = 1;
> +		int count = nl_addr_get_all(nl_sock, ifi, AF_INET6,
> +					    ip6->addrs, IP6_MAX_ADDRS,
> +					    &ip6->our_tap_ll);
> +		if (count < 0) {
> +			debug("Couldn't discover IPv6 address: %s",
> +			      strerror_(-count));
> +			return 0;
> +		}
> +		if (count == 0) {
> +			debug("No IPv6 address on interface");
> +			return 0;
> +		}
> +		ip6->addr_count = count;
> +	} else {
> +		/* Even with -a, we still need the link-local for our_tap_ll */
> +		rc = nl_addr_get(nl_sock, ifi, AF_INET6,
> +				 NULL, NULL, &ip6->our_tap_ll);
> +		if (rc < 0) {
> +			debug("Couldn't discover IPv6 link-local: %s",
> +			      strerror_(-rc));
> +			return 0;
> +		}
>  	}
>  
>  	ip6->addr_seen = ip6->addrs[0].addr.a6;
> @@ -872,7 +884,7 @@ static void conf_ip6_local(struct ip6_ctx *ip6)
>  {
>  	ip6->our_tap_ll = ip6->guest_gw = IP6_LL_GUEST_GW;
>  
> -	ip6->no_copy_addrs = ip6->no_copy_routes = true;
> +	ip6->no_copy_routes = true;
>  }
>  
>  /**
> @@ -1723,8 +1735,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			if (c->mode != MODE_PASTA)
>  				die("--no-copy-addrs is for pasta mode only");
>  
> -			warn("--no-copy-addrs will be dropped soon");
> -			c->ip4.no_copy_addrs = c->ip6.no_copy_addrs = true;
> +			warn("--no-copy-addrs is deprecated and has no effect");

This needs a documentation update, including an update to usage().

By the way, at a first look I thought it would be too soon for this,
but actually it's been almost three years, and the option has been
introduced as deprecated right away... so I guess it's fine.

>  			copy_addrs_opt = true;
>  			break;
>  		case 20:
> @@ -1883,8 +1894,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			union inany_addr addr;
>  			const struct in_addr *a4;
>  			int prefix_len = 0;
> -			unsigned int i;
> -			int af;
> +			int af, i;
>  
>  			af = conf_addr_prefix_len(optarg, &addr, &prefix_len);
>  
> @@ -1901,11 +1911,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  					die("Too many IPv6 addresses");
>  
>  				c->ip6.addrs[i].addr.a6 = addr.a6;
> -				c->ip6.addrs[i].prefix_len = prefix_len;
> +				c->ip6.addrs[i].prefix_len = prefix_len ? prefix_len : 64;
>  				c->ip6.addrs[i].flags = INANY_ADDR_CONFIGURED;
>  				c->ip6.addr_count++;
> -				if (c->mode == MODE_PASTA)
> -					c->ip6.no_copy_addrs = true;
>  				break;
>  			}
>  
> @@ -1918,19 +1926,17 @@ void conf(struct ctx *c, int argc, char **argv)
>  					die("Too many IPv4 addresses");
>  
>  				c->ip4.addrs[i].addr = inany_from_v4(*a4);
> -				c->ip4.addrs[i].prefix_len = prefix_len;
>  				c->ip4.addrs[i].flags = INANY_ADDR_CONFIGURED;
> -				if (i == 0 && prefix_len) {
> +				if (prefix_len) {
>  					if (prefix_from_opt)
>  						die("Can't mix CIDR with -n");
> +					c->ip4.addrs[i].prefix_len = prefix_len;
>  					prefix_from_cidr = true;
>  				} else {
>  					prefix_len = ip4_default_prefix_len(a4);
>  				}
>  				c->ip4.addrs[0].prefix_len = prefix_len;
>  				c->ip4.addr_count++;
> -				if (c->mode == MODE_PASTA)
> -					c->ip4.no_copy_addrs = true;
>  				break;
>  			}
>  
> diff --git a/netlink.c b/netlink.c
> index 82a2f0c..8c4412c 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -35,6 +35,7 @@
>  #include "passt.h"
>  #include "log.h"
>  #include "ip.h"
> +#include "inany.h"
>  #include "netlink.h"
>  #include "epoll_ctl.h"
>  
> @@ -812,6 +813,89 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
>  	return status;
>  }
>  
> +/**
> + * nl_addr_get_all() - Read all addresses for a given interface into an array
> + * @s:		Netlink socket
> + * @ifi:	Interface index
> + * @af:	Address family

Missing tab for the usual alignment.

> + * @addrs:	Array of address entries to fill
> + * @max_addrs:	Maximum number of addresses to store
> + * @addr_ll:	Fill with link-local address if non-NULL
> + *
> + * Return: number of addresses found on success, negative error code on failure
> + */
> +int nl_addr_get_all(int s, unsigned int ifi, sa_family_t af,
> +		    struct inany_addr_entry *addrs, int max_addrs,
> +		    void *addr_ll)
> +{
> +	struct req_t {
> +		struct nlmsghdr nlh;
> +		struct ifaddrmsg ifa;
> +	} req = {
> +		.ifa.ifa_family = af,
> +		.ifa.ifa_index  = ifi,
> +	};
> +	struct nlmsghdr *nh;
> +	char buf[NLBUFSIZ];
> +	ssize_t status;
> +	int count = 0;
> +	uint32_t seq;
> +
> +	seq = nl_send(s, &req, RTM_GETADDR, NLM_F_DUMP, sizeof(req));
> +	nl_foreach_oftype(nh, status, s, buf, seq, RTM_NEWADDR) {
> +		struct ifaddrmsg *ifa = (struct ifaddrmsg *)NLMSG_DATA(nh);
> +		struct rtattr *rta;
> +		size_t na;
> +
> +		if (ifa->ifa_index != ifi || ifa->ifa_flags & IFA_F_DEPRECATED)
> +			continue;
> +
> +		/* Add link-local address if requested */

Why "Add"? This is actually fetching it.

> +		if (ifa->ifa_scope == RT_SCOPE_LINK &&
> +		    addr_ll && af == AF_INET6) {

I didn't really think it through, but it would be probably easier to
express this as a "skip" (continue;) clause rather than a direct one.

> +			for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh);
> +			     RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) {
> +				if (rta->rta_type == IFA_ADDRESS) {
> +					memcpy(addr_ll, RTA_DATA(rta),
> +					       RTA_PAYLOAD(rta));
> +					break;
> +				}
> +			}
> +			continue;
> +		}
> +
> +		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
> +		     rta = RTA_NEXT(rta, na)) {
> +			if (af == AF_INET  && rta->rta_type != IFA_LOCAL)
> +				continue;
> +
> +			if (af == AF_INET6 && rta->rta_type != IFA_ADDRESS)
> +				continue;
> +
> +			if (count >= max_addrs)

I guess we should print a warning here.

> +				break;
> +
> +			if (af == AF_INET) {
> +				struct in_addr a4;
> +
> +				memcpy(&a4, RTA_DATA(rta), sizeof(a4));
> +				addrs[count].addr = inany_from_v4(a4);
> +			} else {
> +				memcpy(&addrs[count].addr.a6, RTA_DATA(rta),
> +				       sizeof(addrs[count].addr.a6));
> +			}
> +			addrs[count].prefix_len = ifa->ifa_prefixlen;
> +			addrs[count].flags = INANY_ADDR_HOST;
> +			count++;
> +		}
> +	}
> +
> +	if (status < 0)
> +		return status;
> +
> +	return count;
> +}
> +
>  /**
>   * nl_addr_get_ll() - Get first IPv6 link-local address for a given interface
>   * @s:		Netlink socket
> diff --git a/netlink.h b/netlink.h
> index 8f1e9b9..c983922 100644
> --- a/netlink.h
> +++ b/netlink.h
> @@ -6,6 +6,8 @@
>  #ifndef NETLINK_H
>  #define NETLINK_H
>  
> +struct inany_addr_entry;
> +
>  extern int nl_sock;
>  extern int nl_sock_ns;
>  
> @@ -17,6 +19,9 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  		 int s_dst, unsigned int ifi_dst, sa_family_t af);
>  int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
>  		void *addr, int *prefix_len, void *addr_l);
> +int nl_addr_get_all(int s, unsigned int ifi, sa_family_t af,
> +		    struct inany_addr_entry *addrs, int max_addrs,
> +		    void *addr_ll);
>  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,
> diff --git a/passt.h b/passt.h
> index 9c0c3fe..929b474 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -81,7 +81,6 @@ enum passt_modes {
>   * @addr_out:		Optional source address for outbound traffic
>   * @ifname_out:		Optional interface name to bind outbound sockets to
>   * @no_copy_routes:	Don't copy all routes when configuring target namespace
> - * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   */
>  struct ip4_ctx {
>  	/* PIF_TAP addresses */
> @@ -103,7 +102,6 @@ struct ip4_ctx {
>  	char ifname_out[IFNAMSIZ];
>  
>  	bool no_copy_routes;
> -	bool no_copy_addrs;
>  };
>  
>  /**
> @@ -124,7 +122,6 @@ struct ip4_ctx {
>   * @addr_out:		Optional source address for outbound traffic
>   * @ifname_out:		Optional interface name to bind outbound sockets to
>   * @no_copy_routes:	Don't copy all routes when configuring target namespace
> - * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   */
>  struct ip6_ctx {
>  	/* PIF_TAP addresses */
> @@ -147,7 +144,6 @@ struct ip6_ctx {
>  	char ifname_out[IFNAMSIZ];
>  
>  	bool no_copy_routes;
> -	bool no_copy_addrs;
>  };
>  
>  #include <netinet/if_ether.h>
> diff --git a/pasta.c b/pasta.c
> index 27ce6a7..faf1a73 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -337,21 +337,15 @@ void pasta_ns_conf(struct ctx *c)
>  		nl_link_set_flags(nl_sock_ns, c->pasta_ifi, flags, flags);
>  
>  		if (c->ifi4) {
> -			if (c->ip4.no_copy_addrs) {
> -				int i;
> -
> -				for (i = 0; i < c->ip4.addr_count; i++) {
> -					rc = nl_addr_set(nl_sock_ns,
> -							 c->pasta_ifi, AF_INET,
> -							 inany_v4(&c->ip4.addrs[i].addr),
> -							 c->ip4.addrs[i].prefix_len);
> -					if (rc < 0)
> -						break;
> -				}
> -			} else {
> -				rc = nl_addr_dup(nl_sock, c->ifi4,
> -						 nl_sock_ns, c->pasta_ifi,
> -						 AF_INET);
> +			int i;
> +
> +			for (i = 0; i < c->ip4.addr_count; i++) {
> +				rc = nl_addr_set(nl_sock_ns,
> +						 c->pasta_ifi, AF_INET,
> +						 inany_v4(&c->ip4.addrs[i].addr),
> +						 c->ip4.addrs[i].prefix_len);
> +				if (rc < 0)
> +					break;
>  			}
>  
>  			if (rc < 0) {
> @@ -392,7 +386,7 @@ void pasta_ns_conf(struct ctx *c)
>  			nl_link_set_flags(nl_sock_ns, c->pasta_ifi,
>  					  0, IFF_NOARP);
>  
> -			if (c->ip6.no_copy_addrs) {
> +		{
>  				struct in6_addr *a;
>  				int i;
>  
> @@ -400,16 +394,14 @@ void pasta_ns_conf(struct ctx *c)
>  					a = &c->ip6.addrs[i].addr.a6;
>  					if (IN6_IS_ADDR_UNSPECIFIED(a))
>  						continue;
> +
>  					rc = nl_addr_set(nl_sock_ns,
> -							 c->pasta_ifi,
> -							 AF_INET6, a, 64);
> +							 c->pasta_ifi, AF_INET6,
> +							 a,
> +							 c->ip6.addrs[i].prefix_len);
>  					if (rc < 0)
>  						break;
>  				}
> -			} else {
> -				rc = nl_addr_dup(nl_sock, c->ifi6,
> -						 nl_sock_ns, c->pasta_ifi,
> -						 AF_INET6);

Now cppcheck mentions that:

netlink.c:1024:5: style: The function 'nl_addr_dup' is never used. [unusedFunction]
int nl_addr_dup(int s_src, unsigned int ifi_src,
    ^

>  			}
>  
>  			if (rc < 0) {

-- 
Stefano