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

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

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

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.

> 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).

[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.

[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.

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