On Thu, Apr 09, 2026 at 09:40:57AM +0200, Stefano Brivio wrote:
> On Thu, 9 Apr 2026 10:47:54 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Apr 08, 2026 at 11:39:55PM +0200, Stefano Brivio wrote:
> > > 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) ...".  
> > 
> > Good point.
> > 
> > > 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  
> > 
> > We could do that, but it makes lookups of a rule's sockets more
> > awkward for minimal benefit
> 
> I see.
> 
> > > 2. storing start and end. I'm not sure if it's usable, but it would
> > >    actually look easier to describe  
> > 
> > We could do that, but it means maintaining redundant information that
> > we never actually have a reason to consult
> 
> Right... then let's just make this clear I suppose...
> 
> > > 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.  
> > 
> > I have this version now:
> > 
> > /**
> >  * struct fwd_table - Forwarding state (per initiating pif)
> >  * @count:	Number of forwarding rules
> >  * @rules:	Array of forwarding rules
> >  * @rulesocks:	Parallel array ro @rules (@count valid entries) of pointers to
> 
> s/ro/of/ (?)

Oops, fixed.

> >  *		@socks entries giving the start of the corresponding rule's
> >  *		sockets within the larger array
> >  * @sock_count:	Number of entries used in @socks (for all rules combined)
> >  * @socks:	Listening sockets for forwarding
> >  */
> > 
> 
> ...other than that it looks pretty clear to me.
> 
> -- 
> Stefano
> 

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