Re: [RFC 03/12] conf: Allow multiple -a/--address options per address family

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

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

On Sun, Dec 14, 2025 at 08:54:32PM -0500, Jon Maloy wrote:
> We enable configuration of multiple IPv4 and IPv6 addresses by allowing
> repeated use of the -a/--address option.
> 
> - We update option parsing to append addresses to the addrs[] array.
> 
> - Each address specified via -a does initially get a class-based default
>   prefix.
> 
> - If no -a option is given, address and prefix are inherited from
>   the template interface, just like now.
> 
> - The -n/--netmask option applies only to the first address, in addrs[0].

I know this is temporary, but this seems a really bad semantic.
Might be worth merging this patch with the -n patch.

> - We configure all indicated addresses in the namespace interface.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c  | 74 ++++++++++++++++++++++++++++++++++++++-------------------
>  pasta.c | 24 ++++++++++++++-----
>  2 files changed, 68 insertions(+), 30 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 31acc20..e9f217b 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -741,7 +741,7 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4,
>  
>  	ip4->our_tap_addr = ip4->guest_gw;
>  
> -	if (IN4_IS_ADDR_UNSPECIFIED(&ip4->addrs[0].addr))
> +	if (!ip4->addr_count)
>  		return 0;
>  
>  	return ifi;
> @@ -756,6 +756,7 @@ static void conf_ip4_local(struct ip4_ctx *ip4)
>  	ip4->addr_seen = ip4->addrs[0].addr = IP4_LL_GUEST_ADDR;
>  	ip4->our_tap_addr = ip4->guest_gw = IP4_LL_GUEST_GW;
>  	ip4->addrs[0].prefix_len = IP4_LL_PREFIX_LEN;
> +	ip4->addr_count = 1;

Doesn't this belong in an earlier patch?

Also, kind of pre-existing, but I don't think conf_ip4_local() should
overwrite an address specified with -a.  I think we want to change
that to either be skipped if -a is used, or to add the link local
address to the ones given explicitly.

>  
>  	ip4->no_copy_addrs = ip4->no_copy_routes = true;
>  }
> @@ -810,8 +811,7 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6,
>  	if (IN6_IS_ADDR_LINKLOCAL(&ip6->guest_gw))
>  		ip6->our_tap_ll = ip6->guest_gw;
>  
> -	if (IN6_IS_ADDR_UNSPECIFIED(&ip6->addrs[0].addr) ||
> -	    IN6_IS_ADDR_UNSPECIFIED(&ip6->our_tap_ll))
> +	if (!ip6->addr_count || IN6_IS_ADDR_UNSPECIFIED(&ip6->our_tap_ll))
>  		return 0;
>  
>  	return ifi;
> @@ -903,9 +903,11 @@ static void usage(const char *name, FILE *f, int status)
>  		"    default: 65520: maximum 802.3 MTU minus 802.3 header\n"
>  		"                    length, rounded to 32 bits (IPv4 words)\n"
>  		"  -a, --address ADDR	Assign IPv4 or IPv6 address ADDR\n"
> -		"    can be specified zero to two times (for IPv4 and IPv6)\n"
> +		"    can be specified multiple times (limit: %d IPv4, %d IPv6)\n"

man page might need an update too.

>  		"    default: use addresses from interface with default route\n"
> -		"  -n, --netmask MASK	Assign IPv4 MASK, dot-decimal or bits\n"
> +		"  -n, --netmask MASK	Assign IPv4 MASK, dot-decimal or bits\n",
> +		IP4_MAX_ADDRS, IP6_MAX_ADDRS);
> +	FPRINTF(f,
>  		"    default: netmask from matching address on the host\n"
>  		"  -M, --mac-addr ADDR	Use source MAC address ADDR\n"
>  		"    default: 9a:55:9a:55:9a:55 (locally administered)\n"
> @@ -1159,9 +1161,11 @@ static void conf_print(const struct ctx *c)
>  				     (32 - c->ip4.addrs[0].prefix_len));
>  
>  			info("DHCP:");
> -			info("    assign: %s",
> -			     inet_ntop(AF_INET, &c->ip4.addrs[0].addr,
> -				       buf4, sizeof(buf4)));
> +			for (i = 0; i < (int)c->ip4.addr_count; i++) {
> +				info("    assign: %s",
> +				     inet_ntop(AF_INET, &c->ip4.addrs[i].addr,
> +					       buf4, sizeof(buf4)));
> +			}

This is misleading.  We allow multiple addresses, but (at least as of
this patch) DHCP will only assign one of them.  In fact I'm not sure
we can assign multiple addresses with DHCP, short of rarely-supported
extensions.

>  			info("    mask: %s",
>  			     inet_ntop(AF_INET, &mask,        buf4, sizeof(buf4)));
>  			info("    router: %s",
> @@ -1198,9 +1202,11 @@ static void conf_print(const struct ctx *c)
>  		else
>  			goto dns6;
>  
> -		info("    assign: %s",
> -		     inet_ntop(AF_INET6, &c->ip6.addrs[0].addr,
> -			       buf6, sizeof(buf6)));
> +		for (i = 0; i < (int)c->ip6.addr_count; i++) {
> +			info("    assign: %s",
> +			     inet_ntop(AF_INET6, &c->ip6.addrs[i].addr,
> +				       buf6, sizeof(buf6)));
> +		}

Similar comments, though I think DHCPv6 can assign multiple addresses
(it's still not implemented as of this patch, though).

>  		info("    router: %s",
>  		     inet_ntop(AF_INET6, &c->ip6.guest_gw, buf6, sizeof(buf6)));
>  		info("    our link-local: %s",
> @@ -1517,6 +1523,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  	struct fqdn *dnss = c->dns_search;
>  	unsigned int ifi4 = 0, ifi6 = 0;
>  	const char *logfile = NULL;
> +	struct in6_addr addr6;
> +	struct in_addr addr4;
>  	size_t logsize = 0;
>  	char *runas = NULL;
>  	long fd_tap_opt;
> @@ -1821,23 +1829,41 @@ void conf(struct ctx *c, int argc, char **argv)
>  			break;
>  		}
>  		case 'a':
> -			if (inet_pton(AF_INET6, optarg,
> -				      &c->ip6.addrs[0].addr) &&
> -			    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addrs[0].addr) &&
> -			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.addrs[0].addr) &&
> -			    !IN6_IS_ADDR_V4MAPPED(&c->ip6.addrs[0].addr) &&
> -			    !IN6_IS_ADDR_V4COMPAT(&c->ip6.addrs[0].addr) &&
> -			    !IN6_IS_ADDR_MULTICAST(&c->ip6.addrs[0].addr)) {
> +			if (inet_pton(AF_INET6, optarg, &addr6) &&
> +			    !IN6_IS_ADDR_UNSPECIFIED(&addr6) &&
> +			    !IN6_IS_ADDR_LOOPBACK(&addr6) &&
> +			    !IN6_IS_ADDR_V4MAPPED(&addr6) &&
> +			    !IN6_IS_ADDR_V4COMPAT(&addr6) &&
> +			    !IN6_IS_ADDR_MULTICAST(&addr6)) {
> +				unsigned int i = c->ip6.addr_count;
> +
> +				if (i >= IP6_MAX_ADDRS)
> +					die("Too many IPv6 addresses");
> +
> +				c->ip6.addrs[i].addr = addr6;
> +				c->ip6.addrs[i].prefix_len = 64;
> +				c->ip6.addrs[i].permanent = true;
> +				c->ip6.addr_count++;
>  				if (c->mode == MODE_PASTA)
>  					c->ip6.no_copy_addrs = true;
>  				break;
>  			}
>  
> -			if (inet_pton(AF_INET, optarg, &c->ip4.addrs[0].addr) &&
> -			    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addrs[0].addr) &&
> -			    !IN4_IS_ADDR_BROADCAST(&c->ip4.addrs[0].addr) &&
> -			    !IN4_IS_ADDR_LOOPBACK(&c->ip4.addrs[0].addr) &&
> -			    !IN4_IS_ADDR_MULTICAST(&c->ip4.addrs[0].addr)) {
> +			if (inet_pton(AF_INET, optarg, &addr4) &&
> +			    !IN4_IS_ADDR_UNSPECIFIED(&addr4) &&
> +			    !IN4_IS_ADDR_BROADCAST(&addr4) &&
> +			    !IN4_IS_ADDR_LOOPBACK(&addr4) &&
> +			    !IN4_IS_ADDR_MULTICAST(&addr4)) {
> +				unsigned int i = c->ip4.addr_count;
> +
> +				if (i >= IP4_MAX_ADDRS)
> +					die("Too many IPv4 addresses");
> +
> +				c->ip4.addrs[i].addr = addr4;
> +				c->ip4.addrs[i].prefix_len =
> +					ip4_default_prefix_len(&addr4);
> +				c->ip4.addrs[i].permanent = true;
> +				c->ip4.addr_count++;
>  				if (c->mode == MODE_PASTA)
>  					c->ip4.no_copy_addrs = true;
>  				break;
> @@ -2135,7 +2161,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  	if (!c->ifi6) {
>  		c->no_ndp = 1;
>  		c->no_dhcpv6 = 1;
> -	} else if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addrs[0].addr)) {
> +	} else if (!c->ip6.addr_count) {
>  		c->no_dhcpv6 = 1;
>  	}
>  
> diff --git a/pasta.c b/pasta.c
> index 49b393c..fe2908f 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -329,10 +329,16 @@ void pasta_ns_conf(struct ctx *c)
>  
>  		if (c->ifi4) {
>  			if (c->ip4.no_copy_addrs) {
> -				rc = nl_addr_set(nl_sock_ns, c->pasta_ifi,
> -						 AF_INET,
> -						 &c->ip4.addrs[0].addr,
> -						 c->ip4.addrs[0].prefix_len);
> +				int i;
> +
> +				for (i = 0; i < c->ip4.addr_count; i++) {
> +					rc = nl_addr_set(nl_sock_ns,
> +							 c->pasta_ifi, AF_INET,
> +							 &c->ip4.addrs[i].addr,
> +							 c->ip4.addrs[i].prefix_len);
> +					if (rc < 0)
> +						break;
> +				}

Maybe it belongs in a different patch, but multiple address support
might less us partially unify the "copy addrs" and "no_copy_addrs"
paths.  Rather than copying addresses direct from the host into the
guest, we optionally add host addresses into the address list, then
unconditionally set everything in the list in the guest.

>  			} else {
>  				rc = nl_addr_dup(nl_sock, c->ifi4,
>  						 nl_sock_ns, c->pasta_ifi,
> @@ -378,12 +384,18 @@ void pasta_ns_conf(struct ctx *c)
>  					  0, IFF_NOARP);
>  
>  			if (c->ip6.no_copy_addrs) {
> -				struct in6_addr *a = &c->ip6.addrs[0].addr;
> +				struct in6_addr *a;
> +				int i;
>  
> -				if (!IN6_IS_ADDR_UNSPECIFIED(a)) {
> +				for (i = 0; i < c->ip6.addr_count; i++) {
> +					a = &c->ip6.addrs[i].addr;
> +					if (IN6_IS_ADDR_UNSPECIFIED(a))
> +						continue;
>  					rc = nl_addr_set(nl_sock_ns,
>  							 c->pasta_ifi,
>  							 AF_INET6, a, 64);
> +					if (rc < 0)
> +						break;
>  				}
>  			} else {
>  				rc = nl_addr_dup(nl_sock, c->ifi6,
> -- 
> 2.51.1
> 

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