Re: [PATCH 1/2] fwd: Refactor tests in fwd_nat_from_tap() for clarity

[David Gibson <david@gibson.dropbear.id.au>]

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

On Wed, Jul 24, 2024 at 08:38:59AM +0200, Stefano Brivio wrote:
> On Wed, 24 Jul 2024 16:20:57 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently, we start by handling the common case, where we don't translate
> > the destination address, then we modify the tgt side for the special cases.
> > In the process we do comparisons on the tentatively set fields in tgt,
> > which obscures the fact that tgt should be an essentially pure function of
> > ini, and risks people examining fields of tgt that are not yet initialized.
> > 
> > To make this clearer, do all our tests on 'ini', constructing tgt from
> > scratch on that basis.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  fwd.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fwd.c b/fwd.c
> > index 8c1f3d91..bd16ac42 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -169,22 +169,22 @@ void fwd_scan_ports_init(struct ctx *c)
> >  uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> >  			 const struct flowside *ini, struct flowside *tgt)
> >  {
> > -	tgt->eaddr = ini->faddr;
> > -	tgt->eport = ini->fport;
> > -
> > -	if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> > -	    inany_equals4(&tgt->eaddr, &c->ip4.dns_match)) {
> 
> These blocks are all one-liners now and curly brackets could go away, I
> guess?

Good point, done.

> > +	if (proto == IPPROTO_UDP && ini->fport == 53 &&
> > +	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
> >  		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
> > -	} else if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> > -		   inany_equals6(&tgt->eaddr, &c->ip6.dns_match)) {
> > +	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
> > +		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
> >  		tgt->eaddr.a6 = c->ip6.dns_host;
> > -	} else if (!c->no_map_gw) {
> > -		if (inany_equals4(&tgt->eaddr, &c->ip4.gw))
> > -			tgt->eaddr = inany_loopback4;
> > -		else if (inany_equals6(&tgt->eaddr, &c->ip6.gw))
> > +	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
> > +		tgt->eaddr = inany_loopback4;
> > +	} else if (!c->no_map_gw && inany_equals6(&ini->faddr, &c->ip6.gw)) {
> >  			tgt->eaddr = inany_loopback6;
> 
> Resulting excess tab here.

Fixed.

> > +	} else {
> > +		tgt->eaddr = ini->faddr;
> >  	}
> >  
> > +	tgt->eport = ini->fport;
> > +
> >  	/* The relevant addr_out controls the host side source address.  This
> >  	 * may be unspecified, which allows the kernel to pick an address.
> >  	 */
> 

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