Re: [PATCH v3 07/11] conf: Allow multiple -a/--address options per address family

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

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

On Fri, Jan 30, 2026 at 04:44:43PM -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 unified addrs[]
>   array, with limit checks for IP4_MAX_ADDRS and IP6_MAX_ADDRS.

I don't see any point to separate v4 and v6 limits, now there's a
unified array.

> - Each address specified via -a, but with no prefix length indicated,
>   gets a class-based default prefix length.

That's not new, is it?  I think we want to be careful with this.  We
need to maintain compatibility with previously working options, but I
think we want to strongly encourage prefix lengths to be explicitly
specified.  Address classes are very anachronistic now, so I don't
think we want to add any more uses of them than we have to for
compatibility.

> - If no -a option is given, addresses/prefix lengths are inherited from
>   the template interface.
> - If a prefix length is to be added, it has to be done in CIDR format,
>   except for the very first address.

I don't really follow what that means.

> - We configure all indicated addresses in the namespace interface using
>   the for_each_addr() macro.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v2: - Adapted to previous code changes
> v3: - Adapted to single-array strategy
>     - Changes according to feedback from S. Brivio and G Gibson.
> ---
>  conf.c  | 23 ++++++++++++++++-------
>  pasta.c | 21 ++++++++++++++-------
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index bb6bcf8..d73a3dd 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -803,13 +803,13 @@ static unsigned int conf_ip6(unsigned int ifi, struct ctx *c)
>  	}
>  
>  	e = first_v6(c);
> -	c->ip6.addr_seen = e->addr.a6;
> +	if (e)
> +		c->ip6.addr_seen = e->addr.a6;

This seems like it belongs in an earlier patch.

>  
>  	if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.guest_gw))
>  		c->ip6.our_tap_ll = c->ip6.guest_gw;
>  
> -	if (IN6_IS_ADDR_UNSPECIFIED(&e->addr.a6) ||
> -	    IN6_IS_ADDR_UNSPECIFIED(&c->ip6.our_tap_ll))

Missed this in an earlier patch, but !!e and an unspecified address
shouldn't be possible, no?

> +	if (!count_v6(c) || IN6_IS_ADDR_UNSPECIFIED(&c->ip6.our_tap_ll))
>  		return 0;
>  
>  	return ifi;
> @@ -901,9 +901,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[/PREFIXLEN]\n"
> -		"    can be specified zero to two times (for IPv4 and IPv6)\n"
> +		"    can be specified multiple times (limit: %d IPv4, %d IPv6)\n"
>  		"    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"
> @@ -1836,6 +1838,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  				die("Can't mix CIDR with -n");
>  
>  			if (af == AF_INET) {
> +				if (count_v4(c) >= IP4_MAX_ADDRS)
> +					die("Too many IPv4 addresses");
> +
>  				e = &c->addrs[c->addr_count];
>  				e->addr = addr;
>  				e->prefix_len = prefix_len ? prefix_len :
> @@ -1845,6 +1850,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  				if (c->mode == MODE_PASTA)
>  					c->ip4.no_copy_addrs = true;
>  			} else if (af == AF_INET6) {
> +				if (count_v6(c) >= IP6_MAX_ADDRS)
> +					die("Too many IPv6 addresses");
> +
>  				e = &c->addrs[c->addr_count];
>  				e->addr = addr;
>  				e->prefix_len = prefix_len ? prefix_len : 64;
> @@ -1861,6 +1869,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  			struct inany_addr_entry *e;
>  			int plen;
>  
> +			if (count_v4(c) > 1)
> +				die("-n can only be used with first address");
>  			if (prefix_from_cidr)
>  				die("Can't use both -n and CIDR prefix length");
>  			plen = conf_ip4_prefix(optarg);
> @@ -2156,8 +2166,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  	if (!c->ifi6) {
>  		c->no_ndp = 1;
>  		c->no_dhcpv6 = 1;
> -	} else if (!first_v6(c) ||
> -		   IN6_IS_ADDR_UNSPECIFIED(&first_v6(c)->addr.a6)) {
> +	} else if (!count_v6(c)) {
>  		c->no_dhcpv6 = 1;
>  	}
>  
> diff --git a/pasta.c b/pasta.c
> index de0ba14..8cb5873 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -312,10 +312,14 @@ static void pasta_ns_conf_ip4(struct ctx *c)
>  	int rc = 0;
>  
>  	if (c->ip4.no_copy_addrs) {
> -		struct inany_addr_entry *e = first_v4(c);
> +		const struct inany_addr_entry *e;
>  
> -		rc = nl_addr_set(nl_sock_ns, c->pasta_ifi, AF_INET,
> -				 inany_v4(&e->addr), e->prefix_len - 96);
> +		for_each_addr(c, e, AF_INET) {
> +			rc = nl_addr_set(nl_sock_ns, c->pasta_ifi, AF_INET,
> +					 inany_v4(&e->addr), e->prefix_len - 96);
> +			if (rc < 0)
> +				break;
> +		}

As noted on the previous patch, I think a single pass through the
array makes sense, only changing the inner part of the loop for v4
vs. v6 (we could potentially update nl_addr_set() to take an inany,
which might make it cleaner).

>  	} else {
>  		rc = nl_addr_dup(nl_sock, c->ifi4,
>  				 nl_sock_ns, c->pasta_ifi, AF_INET);
> @@ -346,7 +350,6 @@ static void pasta_ns_conf_ip4(struct ctx *c)
>   */
>  static void pasta_ns_conf_ip6(struct ctx *c)
>  {
> -	struct inany_addr_entry *e;
>  	int rc = 0;
>  
>  	rc = nl_addr_get_ll(nl_sock_ns, c->pasta_ifi, &c->ip6.addr_ll_seen);
> @@ -365,11 +368,15 @@ static void pasta_ns_conf_ip6(struct ctx *c)
>  	nl_link_set_flags(nl_sock_ns, c->pasta_ifi, 0, IFF_NOARP);
>  
>  	if (c->ip6.no_copy_addrs) {
> -		e = first_v6(c);
> +		const struct inany_addr_entry *e;
>  
> -		if (e && !IN6_IS_ADDR_UNSPECIFIED(&e->addr.a6)) {
> +		for_each_addr(c, e, AF_INET6) {
> +			if (IN6_IS_ADDR_UNSPECIFIED(&e->addr.a6))
> +				continue;
>  			rc = nl_addr_set(nl_sock_ns, c->pasta_ifi,
> -					 AF_INET6, &e->addr.a6, 64);
> +					 AF_INET6, &e->addr.a6, e->prefix_len);
> +			if (rc < 0)
> +				break;
>  		}
>  	} else {
>  		rc = nl_addr_dup(nl_sock, c->ifi6,
> -- 
> 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 --]