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

On Wed, Apr 08, 2026 at 11:39:55PM +0200, Stefano Brivio wrote:

On Wed, 8 Apr 2026 11:30:29 +1000 David Gibson 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 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