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: 5168 bytes --]

On Fri, Apr 03, 2026 at 08:20:12AM +0200, Stefano Brivio wrote:
> 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.

I mean, fwd_set_addr() is essentially storing a addr+prefix_len as a
prefix in the table, so I think it's reasonable to consider the passed
address and length to be linked.

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

I think it's not too bad to (conditionally) get an IPv4+prefix_len
from an inany+prefix_len.  The other way is more awkward, but also
less common.

As noted, having explicit types representing prefixes would probably
help.

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