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

[Stefano Brivio <sbrivio@redhat.com>]

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?

> +	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.

> +	} 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.
>  	 */

-- 
Stefano