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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
[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