Re: [PATCH v6 02/13] ip: Introduce unified multi-address data structures

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 3 Apr 2026 12:25:44 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Mar 30, 2026 at 11:57:10PM +0200, Stefano Brivio wrote:
> > On Sat, 21 Mar 2026 20:43:22 -0400
> > Jon Maloy <jmaloy@redhat.com> wrote:
> >   
> > > As preparation for supporting multiple addresses per interface,
> > > we replace the single addr/prefix_len fields with an array. The
> > > array consists of a new struct inany_addr_entry containing an
> > > address and prefix length, both in inany_addr format.
> > > 
> > > Despite some code refactoring, there are only two real functional
> > > changes:
> > > - The indicated IPv6 prefix length is now properly stored, instead
> > >   of being ignored and overridden with the hardcoded value 64, as
> > >   as has been the case until now.
> > > - Since even IPv4 addresses now are stored in IPv6 format, we
> > >   also store the corresponding prefix length in that format,
> > >   i.e. using the range [96,128] instead of [0,32].
> > > 
> > > Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> > > 
> > > ---
> > > v2: -Using inany_addr instead of protocol specific addresses as
> > >      entry address field.
> > > 
> > > v3: -Merging into one array, directly in struct ctx
> > >     -Changed prefix_len and flags fields in struct inany_addr_entry
> > >      to uint8_t, since that makes the struct directly migratable
> > > 
> > > v4: -Updated according to changes in previous commits
> > >     -Updated according to feedback from David G.
> > >     -Squashed IP4_MASK macro commit into this one
> > > 
> > > v6: -Renamed and moved some definitions
> > >     -Introduced fwd_set_addr() and fwd_get_addr() already in this commit
> > >     -Eliminated first_v4/v6() functions, replaced with fwd_get_addr()
> > >     -Some other changes as suggested by David G.
> > >     -I kept the flag CONF_ADDR_LINKLOCAL, since it will be
> > >      needed later in an address selection function.  
> 
> [snip]
> > > -	if (!ip4->prefix_len) {
> > > -		in_addr_t addr = ntohl(ip4->addr.s_addr);
> > > -		if (IN_CLASSA(addr))
> > > -			ip4->prefix_len = (32 - IN_CLASSA_NSHIFT);
> > > -		else if (IN_CLASSB(addr))
> > > -			ip4->prefix_len = (32 - IN_CLASSB_NSHIFT);
> > > -		else if (IN_CLASSC(addr))
> > > -			ip4->prefix_len = (32 - IN_CLASSC_NSHIFT);
> > > -		else
> > > -			ip4->prefix_len = 32;
> > > +		fwd_set_addr(c, &inany_from_v4(addr), CONF_ADDR_HOST,
> > > +			     prefix_len + 96);  
> > 
> > Somewhat symmetric to David's comment to inany_prefix_len():
> > fwd_set_addr() is already checking if the address is IPv4 or IPv6, so
> > you could pass the actual prefix length.  
> 
> I disagree - in fact I suspect the + 96 is there because of my earlier
> feedback.  When a prefix length is associated with an inany, it should
> always be encoded as for an IPv6 address, since an inany essentialy is
> an IPv6 address.  There are cases where we can directly use an inany
> as an IPv6 address, without caring that it might be (dual stack socket
> interfaces).  If we need a prefix length in that context, we need to
> be able to treat it as an IPv6 prefix length, without having to adjust
> it for the case of a v4-mapped address.
> 
> The +96 here is representing that we're converting from a strictly
> IPv4 addr+prefix_len to an inany addr+prefix_len.

Hmm, I see. I considered the prefix length argument in this case a
separate bit of information compared to the inany stuff, nothing
else than a "prefix length" argument. But I see why you might consider
that strictly in conjunction with the inany argument.

> > Having to add 96 for IPv4 looks rather bug-prone for the future.  
> 
> It definitely has gotchas, but I think the ones we get by
> inconsistently encoding the prefix length for an inany are worse.
> 
> I do think the gotches here could be significantly mitigated by having
> variants of inany_v4(), inany_from_v4() and the like which convert an
> entire prefix (i.e. address and prefix length in the same call).

Ah, yes, that would definitely help. I'm not sure how complicated it
is. Maybe we could as a start have a INANY_PREFIX_LEN_FROM_IPV4(x)
macro or something. But I'd say let's leave + 96 here for the moment if
we risk adding more interruptions / complexity.

> [snip]
> > > +/**
> > > + * fwd_get_addr() - Get guest address entry matching criteria
> > > + * @c:		Execution context
> > > + * @af:		Address family (AF_INET, AF_INET6, or 0 for any)
> > > + * @incl:	Flags that must be present (any-match)
> > > + * @excl:	Flags that must not be present
> > > + *
> > > + * Return: first address entry matching criteria, or NULL
> > > + */
> > > +const struct guest_addr *fwd_get_addr(const struct ctx *c, sa_family_t af,
> > > +				      uint8_t incl, uint8_t excl)
> > > +{
> > > +	const struct guest_addr *a;
> > > +
> > > +	for_each_addr(a, c, af) {
> > > +		if (incl && !(a->flags & incl))
> > > +			continue;
> > > +		if (a->flags & excl)
> > > +			continue;  
> > 
> > Slightly less generic, but maybe good enough for this purpose: you
> > could admit a set of flags, or a negation of a flag (for example
> > ~CONF_ADDR_USER), in a single argument.  
> 
> I don't think that will work, because we almost immediately have
> callers that need both an included and excluded flag.

Right, I realised later when I reached 9/13.

> [snip]
> > > +
> > > +/**
> > > + * struct guest_addr - Unified IPv4/IPv6 address entry
> > > + * @addr:	IPv4 (as mapped) or IPv6 address
> > > + * @prefix_len:	Prefix length in IPv6/IPv4-mapped [0,128]/[96,128] format  
> > 
> > ...do we want to introduce a separate type for inany_addr plus prefix
> > length? I'm not sure.  
> 
> Yeah, I've been wondering that too.  It would make some of those
> encoding conversions more elegant.

-- 
Stefano