Re: [PATCH 06/18] fwd: Better split forwarding rule specification from associated sockets

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 8 Apr 2026 11:30:29 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Apr 08, 2026 at 01:14:46AM +0200, Stefano Brivio wrote:
> > On Tue,  7 Apr 2026 13:16:18 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > 6dad076df037 ("fwd: Split forwarding rule specification from its
> > > implementation state") created struct fwd_rule_state with a forwarding rule
> > > plus the table of sockets used for its implementation.  It turns out this
> > > is quite awkward for sharing rule parsing code between passt and the
> > > upcoming configuration client.  
> > 
> > Indeed, I hated it, in that short moment I had to fiddle with it. Thanks
> > for coming up with a cleaner solution.  
> 
> Yeah, mea culpa.  Seemed like a good idea at the time, but it wasn't.
> 
> [snip]
> > >  /**
> > > - * struct fwd_table - Table of forwarding rules (per initiating pif)
> > > + * struct fwd_table - Forwarding state (per initiating pif)
> > >   * @count:	Number of forwarding rules
> > >   * @rules:	Array of forwarding rules
> > > + * @rulesocks:	Pointers to socket arrays per-rule  
> > 
> > I don't see this as particularly descriptive (which sockets? What's
> > the array size?). I'm thinking of something like:
> > 
> >  @socks_ref:	Per-rule pointers to associated @socks, @sock_count of them  
> 
> There are @count of them, not @sock_count...

Oops, "of course"...

> which I guess just
> emphasises the need for a better description.  How's this:
> 
>  * struct fwd_table - Forwarding state (per initiating pif)
>  * @count:	Number of forwarding rules
>  * @rules:	Array of forwarding rules
>  * @rulesocks:	Array of @count pointers within @socks giving the start of the
>  *		corresponding rule's listening sockets within the larger array

"Array of @count pointers" is ambiguous in English as it might refer to
pointers to @count. It obviously doesn't, but it might take a couple of
readings to realise that. Simple fix: "Array of pointers (@count of
them) ...".

For the rest, yes, it's better, but I started wondering if we could
simplify the representation a bit by, either:

1. storing indices instead of int *, or

2. storing start and end. I'm not sure if it's usable, but it would
   actually look easier to describe

if neither of these applies (I didn't really think it through), maybe
this is slightly more intuitive:

 * Pointers to entry in @socks (@count of them) with first socket for each rule

? If not, I think the version you just proposed is better than the
original and sufficiently clear anyway.

>  * @sock_count:	Number of entries used in @socks (for all rules combined)
>  * @socks:	Listening sockets for forwarding
> 
> > >   * @sock_count:	Number of entries used in @socks
> > >   * @socks:	Listening sockets for forwarding
> > >   */
> > >  struct fwd_table {
> > >  	unsigned count;
> > > -	struct fwd_rule_state rules[MAX_FWD_RULES];
> > > +	struct fwd_rule rules[MAX_FWD_RULES];
> > > +	int *rulesocks[MAX_FWD_RULES];
> > >  	unsigned sock_count;
> > >  	int socks[MAX_LISTEN_SOCKS];
> > >  };  
> > 
> > -- 
> > Stefano
> >   
> 

-- 
Stefano