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

David Gibson
2026-04-09T00:48:04Z

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

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

> 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

> 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
* @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
*/

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