Re: [PATCH v7 02/13] passt, pasta: Introduce unified multi-address data structures

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

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

On Sat, Jun 20, 2026 at 12:09:51AM +0200, Stefano Brivio wrote:
> On Sun, 12 Apr 2026 20:53:08 -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 a lot of 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
> >   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.
> > 
> > v7: -Introduced CONF_ADDR_GENERATED flag
> >     -Other fixes based on feedback from David and Stefano.
> >     -I changed signature of inany_prefix_len(), but I did not change
> >      its semantics, since the premise of David's comment is wrong: the
> >      caller does *not* explicitly know he is dealing with an IPv4 address.
> >      In fact, there are examples later in this series where it may be an
> >      IPv6 address, and the caller just trusts he gets the return value in
> >      the appropriate format.
> >     -Introduced the inverse of inany_prefix_len(), called inany_prefix_len6()
> >      which always returns the prefix in IPv6 or mapped IPv4 format.
> >      The name of the function isn't great, but any alternative I came up
> >      with became too long to be practical.
> > ---
> >  arp.c    |  12 ++++-
> >  conf.c   | 143 ++++++++++++++++++++++++++++++-------------------------
> >  dhcp.c   |  14 ++++--
> >  dhcpv6.c |  15 ++++--
> >  fwd.c    | 109 ++++++++++++++++++++++++++++++++++--------
> >  fwd.h    |   4 ++
> >  inany.h  |  41 ++++++++++++++++
> >  ip.h     |   2 +
> >  ndp.c    |  16 +++++--
> >  passt.h  |  67 ++++++++++++++++++++++----
> >  pasta.c  |  25 ++++++----
> >  tap.c    |   7 ++-
> >  12 files changed, 340 insertions(+), 115 deletions(-)
> > 
> > diff --git a/arp.c b/arp.c
> > index bb042e9..a7fd82f 100644
> > --- a/arp.c
> > +++ b/arp.c
> > @@ -41,6 +41,8 @@
> >  static bool ignore_arp(const struct ctx *c,
> >  		       const struct arphdr *ah, const struct arpmsg *am)
> >  {
> > +	const struct guest_addr *a;
> > +
> >  	if (ah->ar_hrd != htons(ARPHRD_ETHER)	||
> >  	    ah->ar_pro != htons(ETH_P_IP)	||
> >  	    ah->ar_hln != ETH_ALEN		||
> > @@ -54,7 +56,8 @@ static bool ignore_arp(const struct ctx *c,
> >  		return true;
> >  
> >  	/* Don't resolve the guest's assigned address, either. */
> > -	if (!memcmp(am->tip, &c->ip4.addr, sizeof(am->tip)))
> > +	a = fwd_get_addr(c, AF_INET, 0, 0);
> 
> I guess it's not strictly needed right now to avoid breaking things,
> but, eventually, if we support multiple assigned / configured /
> observed addresses for the guest, we should make sure we don't resolve
> any of them. That is, we should eventually pass am->tip to a lookup
> function. It might be in scope for this series but not necessarily.

I think one of the later patches already does that.

I do wonder if for supporting multiple guest addresses it makes sense
at some point to switch from resolving everything _except_ a known
guest address to only resolving addresses that passt "owns" on the
guest link (basically the gateway address, maybe some NATs).

[snip]
> > +
> > +	if (c->addr_count >= MAX_GUEST_ADDRS)
> > +		return;
> > +
> > +	a = &c->addrs[c->addr_count++];
> > +
> > +found:
> > +	a->addr = *addr;
> > +	a->prefix_len = inany_prefix_len6(addr, prefix_len);
> > +	a->flags = flags;
> > +}
> > +
> > +/**
> > + * fwd_get_addr() - Get guest address entry matching criteria
> > + * @c:		Execution context
> > + * @af:		Address family (AF_INET, AF_INET6, or 0 for any)
> 
> I think AF_UNSPEC (that you seem to use later?) is more common to denote
> "any". Not a strong preference, 0 might have some advantages as well.

As it happens, AF_UNSPEC has the value 0, but I agree using the name
is better.

> > + * @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->addrs, c->addr_count, af) {
> > +		if (incl && !(a->flags & incl))
> 
> Regardless of my CONF_ADDR_PICK_ANY suggestion above, it might be a bit
> surprising that 0 matches all flags (but so does 0xff). If it needs to
> be like that for whatever reason, I think that deserves a mention in
> the comment to @incl ("0 means any").

Yeah, I also don't love that special case.

[snip]
> > +/**
> > + * for_each_addr() - Iterate over addresses in array
> > + * @a:		Pointer variable for current entry (struct guest_addr *)
> > + * @addrs:	Array of guest addresses (e.g., c->addrs)
> > + * @count:	Number of addresses (e.g., c->addr_count)
> > + * @af:		Address family filter: AF_INET, AF_INET6, or 0 for all
> > + */
> > +#define for_each_addr(a, addrs, count, af)				\
> > +	for (int i_ = next_addr_idx_((addrs), (count), 0, (af));	\
> > +	     i_ < (count) && ((a) = &(addrs)[i_], true);		\
> 
> Why do you need that ", true"? Isn't an extra () pair enough to silence
> warnings about evaluating the assignment as a condition? I find it a
> bit hard to read like that.

I don't think it's there just for the warning.  That's the loop
condition, so you don't want it to stop if &(addrs)[i_] happens to
have value 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 --]