Re: [PATCH 5/7] Clarify semantics of c->v4 and c->v6 variables

[Stefano Brivio <sbrivio@redhat.com>]

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

On Fri, 22 Jul 2022 15:31:16 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> The v4 and v6 fields of the context structure can be confusing, because
> they change meaning part way through the code:  Before conf_ip(), they are
> booleans which indicate whether the -4 or -6 options have been given.
> After conf_ip() they are DISABLED|ENABLED|PROBE enums which indicate
> whether the IP version is available (which means both that it was allowed
> on the command line and we were able to configure it).  The PROBE variant
> of the enum is only used locally within conf_ip() and since recent changes
> there it no longer has a real purpose different from ENABLED.
> 
> Simplify this all by making the context fields always just a boolean
> indicating the availability of the IP version.  They both default to 1, but
> can be set to 0 by either command line options or configuration failures.
> We use some local variables in conf() for tracking the state of the command
> line options on their own.
> 
> Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
>  conf.c | 59 ++++++++++++++++++++--------------------------------------
>  util.h |  6 ------
>  2 files changed, 20 insertions(+), 45 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index f5b761f..63f0f3d 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -26,6 +26,7 @@
>  #include <pwd.h>
>  #include <stdlib.h>
>  #include <stdint.h>
> +#include <stdbool.h>
>  #include <unistd.h>
>  #include <syslog.h>
>  #include <time.h>
> @@ -615,40 +616,25 @@ static int conf_ns_opt(struct ctx *c,
>   */
>  static void conf_ip(struct ctx *c)
>  {
> -	int v4, v6;
> -
>  	if (c->v4) {
> -		c->v4		= IP_VERSION_ENABLED;
> -		v4		= IP_VERSION_PROBE;
> -		v6 = c->v6	= IP_VERSION_DISABLED;
> -	} else if (c->v6) {
> -		c->v6		= IP_VERSION_ENABLED;
> -		v6		= IP_VERSION_PROBE;
> -		v4 = c->v4	= IP_VERSION_DISABLED;
> -	} else {
> -		c->v4 = c->v6	= IP_VERSION_ENABLED;
> -		v4 = v6		= IP_VERSION_PROBE;
> -	}
> -
> -	if (v4 != IP_VERSION_DISABLED) {
>  		if (!c->ifi4)
>  			c->ifi4 = nl_get_ext_if(AF_INET);
>  		if (!c->ifi4) {
>  			warn("No external routable interface for IPv4");
> -			v4 = IP_VERSION_DISABLED;
> +			c->v4 = 0;
>  		}
>  	}
>  
> -	if (v6 != IP_VERSION_DISABLED) {
> +	if (c->v6) {
>  		if (!c->ifi6)
>  			c->ifi6 = nl_get_ext_if(AF_INET6);
>  		if (!c->ifi6) {
>  			warn("No external routable interface for IPv6");
> -			v6 = IP_VERSION_DISABLED;
> +			c->v6 = 0;
>  		}
>  	}
>  
> -	if (v4 != IP_VERSION_DISABLED) {
> +	if (c->v4) {
>  		if (!c->gw4)
>  			nl_route(0, c->ifi4, AF_INET, &c->gw4);
>  
> @@ -676,7 +662,7 @@ static void conf_ip(struct ctx *c)
>  			nl_link(0, c->ifi4, c->mac, 0, 0);
>  	}
>  
> -	if (v6 != IP_VERSION_DISABLED) {
> +	if (c->v6) {
>  		int prefix_len = 0;
>  
>  		if (IN6_IS_ADDR_UNSPECIFIED(&c->gw6))
> @@ -694,25 +680,18 @@ static void conf_ip(struct ctx *c)
>  	}
>  
>  	if (!c->gw4 || !c->addr4 || MAC_IS_ZERO(c->mac))
> -		v4 = IP_VERSION_DISABLED;
> -	else
> -		v4 = IP_VERSION_ENABLED;
> +		c->v4 = 0;
>  
>  	if (IN6_IS_ADDR_UNSPECIFIED(&c->gw6) ||
>  	    IN6_IS_ADDR_UNSPECIFIED(&c->addr6) ||
>  	    IN6_IS_ADDR_UNSPECIFIED(&c->addr6_ll) ||
>  	    MAC_IS_ZERO(c->mac))
> -		v6 = IP_VERSION_DISABLED;
> -	else
> -		v6 = IP_VERSION_ENABLED;
> +		c->v6 = 0;
>  
> -	if ((v4 == IP_VERSION_DISABLED) && (v6 == IP_VERSION_DISABLED)) {
> +	if (!c->v4 && !c->v6) {
>  		err("External interface not usable");
>  		exit(EXIT_FAILURE);
>  	}
> -
> -	c->v4 = v4;
> -	c->v6 = v6;
>  }
>  
>  /**
> @@ -1054,8 +1033,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"no-ndp",	no_argument,		&c->no_ndp,	1 },
>  		{"no-ra",	no_argument,		&c->no_ra,	1 },
>  		{"no-map-gw",	no_argument,		&c->no_map_gw,	1 },
> -		{"ipv4-only",	no_argument,		&c->v4,		'4' },
> -		{"ipv6-only",	no_argument,		&c->v6,		'6' },
> +		{"ipv4-only",	no_argument,		NULL,		'4' },
> +		{"ipv6-only",	no_argument,		NULL,		'6' },
>  		{"tcp-ports",	required_argument,	NULL,		't' },
>  		{"udp-ports",	required_argument,	NULL,		'u' },
>  		{"tcp-ns",	required_argument,	NULL,		'T' },
> @@ -1083,6 +1062,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  	struct in6_addr *dns6 = c->dns6;
>  	int name, ret, mask, b, i;
>  	uint32_t *dns4 = c->dns4;
> +	bool v4_only = false, v6_only = false;

Moved before declaration of *dnss on merge, for coding style
consistency, to keep declarations from longest to shortest (what they
call reverse Christmas tree in the kernel networking area, which is
pretty much the style I followed here). Rationale similar to:

  https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

-- 
Stefano