Re: [PATCH v2 3/9] conf: Refactor conf_print() for multi-address support

[Stefano Brivio <sbrivio@redhat.com>]

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

> As a preparation for multiple address support, we refactor the
> conf_print() function to handle this properly.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c | 78 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 9fc5dca..3ecd1a0 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1199,20 +1199,28 @@ static void conf_print(const struct ctx *c)
>  				       buf4, sizeof(buf4)));
>  

I didn't really check why that starts being reported now, but, with
this patch, cppcheck now says that:

conf.c:1167:31: style: The scope of the variable 'ifn' can be reduced. [variableScope]
 char bufmac[ETH_ADDRSTRLEN], ifn[IFNAMSIZ];
                              ^

>  		if (!c->no_dhcp) {
> -			uint32_t mask;
> -
> -			mask = htonl(0xffffffff <<
> -				     (32 - c->ip4.addrs[0].prefix_len));
> -
> -			info("DHCP:");
> -			info("    assign: %s",
> -			     inet_ntop(AF_INET, inany_v4(&c->ip4.addrs[0].addr),
> -				       buf4, sizeof(buf4)));
> -			info("    mask: %s",
> -			     inet_ntop(AF_INET, &mask,        buf4, sizeof(buf4)));
> -			info("    router: %s",
> -			     inet_ntop(AF_INET, &c->ip4.guest_gw,
> -				       buf4, sizeof(buf4)));
> +			for (i = 0; i < c->ip4.addr_count; i++) {
> +				const struct inany_addr_entry *e;
> +				uint32_t mask;
> +
> +				e = &c->ip4.addrs[i];
> +				if (!(e->flags & INANY_ADDR_CONFIGURED) &&
> +				    c->ip4.addr_count > 1)
> +					continue;
> +
> +				mask = htonl(0xffffffff << (32 - e->prefix_len));
> +
> +				info("DHCP:");

Should this really be in a loop? We'll assign a single address via DHCP.

> +				info("    assign: %s",
> +				     inet_ntop(AF_INET, inany_v4(&e->addr),
> +					       buf4, sizeof(buf4)));
> +				info("    mask: %s",
> +				     inet_ntop(AF_INET, &mask, buf4, sizeof(buf4)));
> +				info("    router: %s",
> +				     inet_ntop(AF_INET, &c->ip4.guest_gw,
> +					       buf4, sizeof(buf4)));
> +				break;
> +			}
>  		}
>  
>  		for (i = 0; !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) {
> @@ -1230,30 +1238,34 @@ static void conf_print(const struct ctx *c)
>  	}
>  
>  	if (c->ifi6) {
> +		bool do_slaac = !c->no_ndp || !c->no_dhcpv6;

We can enable DHCPv6 and disable SLAAC though. The "SL" in SLAAC stands
for "stateless", DHCPv6 is stateful. We can enable both, one, or none.

The 'dns6' label was meant to avoid a variable like this one and
another block, maybe it would be more practical to keep it.

> +
>  		if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.map_host_loopback))
>  			info("    NAT to host ::1: %s",
>  			     inet_ntop(AF_INET6, &c->ip6.map_host_loopback,
>  				       buf6, sizeof(buf6)));
>  
> -		if (!c->no_ndp && !c->no_dhcpv6)
> -			info("NDP/DHCPv6:");
> -		else if (!c->no_dhcpv6)
> -			info("DHCPv6:");
> -		else if (!c->no_ndp)
> -			info("NDP:");
> -		else
> -			goto dns6;
> -
> -		info("    assign: %s",
> -		     inet_ntop(AF_INET6, &c->ip6.addrs[0].addr.a6,
> -			       buf6, sizeof(buf6)));
> -		info("    router: %s",
> -		     inet_ntop(AF_INET6, &c->ip6.guest_gw, buf6, sizeof(buf6)));
> -		info("    our link-local: %s",
> -		     inet_ntop(AF_INET6, &c->ip6.our_tap_ll,
> -			       buf6, sizeof(buf6)));
> -
> -dns6:
> +		if (do_slaac) {
> +			if (!c->no_ndp && !c->no_dhcpv6)
> +				info("NDP/DHCPv6:");
> +			else if (!c->no_dhcpv6)
> +				info("DHCPv6:");
> +			else
> +				info("NDP:");
> +
> +			for (i = 0; i < c->ip6.addr_count; i++) {
> +				info("    assign: %s",
> +				     inet_ntop(AF_INET6, &c->ip6.addrs[i].addr.a6,
> +					       buf6, sizeof(buf6)));

I don't see a matching change for neither NDP nor DHCPv6, so we
shouldn't really print more than one address (at least until this
point).

> +			}
> +			info("    router: %s",
> +			     inet_ntop(AF_INET6, &c->ip6.guest_gw,
> +				       buf6, sizeof(buf6)));
> +			info("    our link-local: %s",
> +			     inet_ntop(AF_INET6, &c->ip6.our_tap_ll,
> +				       buf6, sizeof(buf6)));
> +		}
> +
>  		for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
>  			if (!i)
>  				info("DNS:");

I reviewed up to 5/9 so far, no further comments until then.

-- 
Stefano