Re: [PATCH 6/6] fwd: Unify TCP and UDP forwarding tables

On Tue, Mar 10, 2026 at 08:33:17PM +0100, Stefano Brivio wrote:

Two nits and one thing that might be a bit more substantial:

On Tue, 10 Mar 2026 15:16:05 +1100 David Gibson wrote:

...

Currently TCP and UDP each have their own forwarding tables. This is awkward in a few places, where we need switch statements to select the correct table. More importantly, it would make things awkward and messy to extend to other protocols in future, which we're likely to want to do.

Merge the TCP and UDP tables into a single table per (source) pif, with the protocol given in each rule entry.

Signed-off-by: David Gibson [snip] static bool fwd_rule_match(const struct fwd_rule *rule, - const struct flowside *ini) + const struct flowside *ini, uint8_t proto) { - return inany_matches(&ini->oaddr, fwd_rule_addr(rule)) && - ini->oport >= rule->first && ini->oport <= rule->last; + return rule->proto == proto && + inany_matches(&ini->oaddr, fwd_rule_addr(rule)) && + ini->oport >= rule->first && ini->oport <= rule->last;

Nit: we usually align things for return clauses like we do with function calls (like in the existing version), that is, here:

return rule->proto == proto && inany_matches(&ini->oaddr, fwd_rule_addr(rule)) && ini->oport >= rule->first && ini->oport <= rule->last;

Ah, right. That's one of the handful of cases where I didn't figure out how to make emacs auto-indent match our style. I tend to forget because it doesn't arise very often [snip]

...

diff --git a/fwd.h b/fwd.h index 1af13ad4..f6f0fa81 100644 --- a/fwd.h +++ b/fwd.h @@ -31,11 +31,12 @@ bool fwd_port_is_ephemeral(in_port_t port); * @first: First port number to forward * @last: Last port number to forward * @to: Target port for @first, port n goes to @to + (n - @first) - * @socks: Array of listening sockets for this entry + * @proto: Protocol to forward * @flags: Flag mask * FWD_DUAL_STACK_ANY - match any IPv4 or IPv6 address (@addr should be ::) * FWD_WEAK - Don't give an error if binds fail for some forwards * FWD_SCAN - Only forward if the matching port in the target is listening + * @socks: Array of listening sockets for this entry * * FIXME: @addr and @ifname currently ignored for outbound tables */ @@ -45,11 +46,12 @@ struct fwd_rule { in_port_t first; in_port_t last; in_port_t to; - int *socks; + uint8_t proto;

Nit: as a result, the comment to struct fwd_table should change to:

* struct fwd_table - Table of forwarding rules, per initiating pif

as those are not per-protocol anymore.

Good point, done.

And the comment to MAX_LISTEN_SOCKS ("Maximum number of listening sockets (per pif & protocol)") should also be changed but, at this point, shouldn't we double that? Or at least NUM_PORTS * 5?

Oh, good catch, I didn't think of that. I've updated it to NUM_PORTS * 5, and fixed the comment.

The rest of the series good to me, so I could apply it with the couple of style fixes I pointed out if you agree... but I guess you should change MAX_LISTEN_SOCKS (unless I'm missing something), so maybe a respin would be more convenient at that point.

Will do. -- 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