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

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

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

On Fri, Jul 10, 2026 at 05:31:23PM -0400, Jon Maloy wrote:
> > > -	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);
> > >   	}
> > > -	ip4->addr_seen = ip4->addr;
> > > -
> > > +	a = fwd_get_addr(c, AF_INET, CONF_ADDR_ANY, 0);
> > 
> > Nit: maybe assert(a), since I doubt static checkers will be able to
> > reason that the fwd_set_addr() above means this fwd_get_addr() can't
> > fail.  Also, I think that go inside the if block, since the other case
> > means we already have a perfectly good a.
> 
> I think my conclusion was that the static checker ignores assert() as a
> conditonal test, but I can try it.

I'm not really sure what you mean by this.  I don't know for certain
with this particular case and each static checker.  I do know that
there are a *heap* of cases where an assert() can fix static checker
warnings because it lets the checker know we can't have reached this
point in the "bad" condition.

[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
> > > + * @flags:	CONF_ADDR_* flags
> > > + */
> > > +struct guest_addr {
> > > +	union inany_addr addr;
> > > +	uint8_t prefix_len;
> > > +	uint8_t flags;
> > > +#define CONF_ADDR_USER		BIT(0)		/* User set via -a */
> > > +#define CONF_ADDR_HOST		BIT(1)		/* From host interface */
> > > +#define CONF_ADDR_GENERATED	BIT(2)		/* Generated by PASST/PASTA */
> > > +#define CONF_ADDR_LINKLOCAL	BIT(3)		/* Link-local address */
> > > +#define CONF_ADDR_ANY		0xff		/* Match any flag */
> > 
> > In general I like CONF_ADDR_ANY instead of 0 being special-cased.  But
> > with LINKLOCAL in the mix it doesn't quite work.  ANY meaning "USER or
> > HOST or GENERATED" makes sense.  ANY meaning "USER or HOST or
> > GENERATED or LINKLOCAL" is nonsense.  I suggest you move LINKLOCAL
> > into a different field, as Stefano has suggested to handle this (maybe
> > "provenance" for USER/HOST/GENERATED and "flags" for LINKLOCAL?).  Or
> > you could remove the LINKLOCAL flag from the structure entirely: the
> > information is already present in the address itself.
> 
> I was hoping to postpone this to a commit after this series, but I see your
> point. I can try with a 'scope' field, as suggested earlier, or just skip it
> altogheter and using an is_link_local() function.

Sorry, I thought there was already an inany_is_linklocal() function,
but I see that there's only inany_is_linklocal6().  So that will need
to be extended to handle IPv4 as well.

[snip]
> > > diff --git a/pasta.c b/pasta.c
> > > index 4e7ee542..9ef3ac00 100644
> > > --- a/pasta.c
> > > +++ b/pasta.c
> > > @@ -330,6 +330,8 @@ void pasta_ns_conf(struct ctx *c)
> > >   	if (c->pasta_conf_ns) {
> > >   		unsigned int flags = IFF_UP;
> > > +		const struct guest_addr *a;
> > > +		int plen = 0;
> > >   		if (c->mtu)
> > >   			nl_link_set_mtu(nl_sock_ns, c->pasta_ifi, c->mtu);
> > > @@ -341,10 +343,17 @@ void pasta_ns_conf(struct ctx *c)
> > >   		if (c->ifi4) {
> > 
> > Nit: I think you can reduce the very deep nesting here, by moving the
> > fwd_get_addr() and changing the if below to..
> 
> This is already done in a later patch, by introducing two subfuntions.
> However, this has caused lot of trouble at each rebase, so I think I will
> move it to a separate, standalone patch before this series.

Sounds like a good idea.

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