Re: [PATCH] conf: Unify several paths in conf_ports()

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 19 Feb 2025 14:54:44 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In conf_ports() we have three different paths which actually do the setup
> of an individual forwarded port: one for the "all" case, one for the
> exclusions only case and one for the range of ports with possible
> exclusions case.
> 
> We can unify those cases using a new helper which handles a single range
> of ports, with a bitmap of exclusions.  Although this is slightly longer
> (largely due to the new helpers function comment), it reduces duplicated
> logic.  It will also make future improvements to the tracking of port
> forwards easier.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c | 175 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 90 insertions(+), 85 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 18017f51..f862845b 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -123,6 +123,75 @@ static int parse_port_range(const char *s, char **endptr,
>  	return 0;
>  }
>  
> +/**
> + * conf_ports_range_except() - Set up forwarding for a range of ports minus a
> + *                             bitmap of exclusions
> + * @c:		Execution context
> + * @optname:	Short option name, t, T, u, or U
> + * @optarg:	Option argument (port specification)
> + * @fwd:	Pointer to @fwd_ports to be updated
> + * @addr:	Listening address
> + * @ifname:	Listening interface
> + * @first:	First port to forward
> + * @last:	Last port to forward
> + * @exclude:	Bitmap of ports to exclude
> + * @to:		Port to translate @first to when forwarding
> + * @weak:	Ignore errors, as long as at least one port is mapped
> + */
> +static void conf_ports_range_except(const struct ctx *c, char optname,
> +				    const char *optarg, struct fwd_ports *fwd,
> +				    const union inany_addr *addr,
> +				    const char *ifname,
> +				    uint16_t first, uint16_t last,
> +				    const uint8_t *exclude, uint16_t to,
> +				    bool weak)

...ouch, bool anything_else? I think this patch is strictly better than
the existing duplication, so I'm fine with it as it is. I don't see any
obvious way to make the prototype smaller (other than things that would
reduce readability by hiding information), so... yeah. I just wanted to
mention that the prototype is a bit heavy.

> +{
> +	bool bound_one = false;
> +	unsigned i;
> +	int ret;
> +
> +	if (first == 0) {
> +		die("Can't forward port 0 for option '-%c %s'",
> +		    optname, optarg);
> +	}
> +
> +	for (i = first; i <= last; i++) {
> +		if (bitmap_isset(exclude, i))
> +			continue;
> +
> +		if (bitmap_isset(fwd->map, i)) {
> +			warn(
> +"Altering mapping of already mapped port number: %s", optarg);
> +		}
> +
> +		bitmap_set(fwd->map, i);
> +		fwd->delta[i] = to - first;
> +
> +		if (optname == 't')
> +			ret = tcp_sock_init(c, addr, ifname, i);
> +		else if (optname == 'u')
> +			ret = udp_sock_init(c, 0, addr, ifname, i);
> +		else
> +			/* No way to check in advance for -T and -U */
> +			ret = 0;
> +
> +		if (ret == -ENFILE || ret == -EMFILE) {
> +			die("Can't open enough sockets for port specifier: %s",
> +			    optarg);
> +		}
> +
> +		if (!ret) {
> +			bound_one = true;
> +		} else if (!weak) {
> +			die("Failed to bind port %u (%s) for option '-%c %s'",
> +			    i, strerror_(-ret), optname, optarg);
> +		}
> +	}
> +
> +	if (!bound_one)
> +		die("Failed to bind any port for '-%c %s'", optname, optarg);
> +}
> +
>  /**
>   * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
>   * @c:		Execution context
> @@ -135,10 +204,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  {
>  	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
>  	char buf[BUFSIZ], *spec, *ifname = NULL, *p;
> -	bool exclude_only = true, bound_one = false;
>  	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
> +	bool exclude_only = true;
>  	unsigned i;
> -	int ret;
>  
>  	if (!strcmp(optarg, "none")) {
>  		if (fwd->mode)
> @@ -173,32 +241,14 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  
>  		fwd->mode = FWD_ALL;
>  
> -		/* Skip port 0.  It has special meaning for many socket APIs, so
> -		 * trying to bind it is not really safe.
> -		 */
> -		for (i = 1; i < NUM_PORTS; i++) {
> +		/* Also exclude ephemeral ports */

"Also" implies the reader figures out that conf_ports_range_except()
skips port zero while looking at this line, which is not really
something you can expect, I guess.

We could probably just drop this comment, or change it as you did below.

> +		for (i = 0; i < NUM_PORTS; i++)
>  			if (fwd_port_is_ephemeral(i))
> -				continue;
> -
> -			bitmap_set(fwd->map, i);
> -			if (optname == 't') {
> -				ret = tcp_sock_init(c, NULL, NULL, i);
> -				if (ret == -ENFILE || ret == -EMFILE)
> -					goto enfile;
> -				if (!ret)
> -					bound_one = true;
> -			} else if (optname == 'u') {
> -				ret = udp_sock_init(c, 0, NULL, NULL, i);
> -				if (ret == -ENFILE || ret == -EMFILE)
> -					goto enfile;
> -				if (!ret)
> -					bound_one = true;
> -			}
> -		}
> -
> -		if (!bound_one)
> -			goto bind_all_fail;
> -
> +				bitmap_set(exclude, i);

I would add an extra blank line here, otherwise it's quite confusing.

> +		conf_ports_range_except(c, optname, optarg, fwd,
> +					NULL, NULL,
> +					1, NUM_PORTS - 1, exclude,
> +					1, true);
>  		return;
>  	}
>  
> @@ -275,37 +325,14 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  	} while ((p = next_chunk(p, ',')));
>  
>  	if (exclude_only) {
> -		/* Skip port 0.  It has special meaning for many socket APIs, so
> -		 * trying to bind it is not really safe.
> -		 */
> -		for (i = 1; i < NUM_PORTS; i++) {
> -			if (fwd_port_is_ephemeral(i) ||
> -			    bitmap_isset(exclude, i))
> -				continue;
> -
> -			bitmap_set(fwd->map, i);
> -
> -			if (optname == 't') {
> -				ret = tcp_sock_init(c, addr, ifname, i);
> -				if (ret == -ENFILE || ret == -EMFILE)
> -					goto enfile;
> -				if (!ret)
> -					bound_one = true;
> -			} else if (optname == 'u') {
> -				ret = udp_sock_init(c, 0, addr, ifname, i);
> -				if (ret == -ENFILE || ret == -EMFILE)
> -					goto enfile;
> -				if (!ret)
> -					bound_one = true;
> -			} else {
> -				/* No way to check in advance for -T and -U */
> -				bound_one = true;
> -			}
> -		}
> -
> -		if (!bound_one)
> -			goto bind_all_fail;
> -
> +		/* Exclude ephemeral ports */
> +		for (i = 0; i < NUM_PORTS; i++)
> +			if (fwd_port_is_ephemeral(i))
> +				bitmap_set(exclude, i);

Same here, a blank line would be nice.

> +		conf_ports_range_except(c, optname, optarg, fwd,
> +					addr, ifname,
> +					1, NUM_PORTS - 1, exclude,
> +					1, true);
>  		return;
>  	}
>  
> @@ -334,40 +361,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		if ((*p != '\0')  && (*p != ',')) /* Garbage after the ranges */
>  			goto bad;
>  
> -		for (i = orig_range.first; i <= orig_range.last; i++) {
> -			if (bitmap_isset(fwd->map, i))
> -				warn(
> -"Altering mapping of already mapped port number: %s", optarg);
> -
> -			if (bitmap_isset(exclude, i))
> -				continue;
> -
> -			bitmap_set(fwd->map, i);
> -
> -			fwd->delta[i] = mapped_range.first - orig_range.first;
> -
> -			ret = 0;
> -			if (optname == 't')
> -				ret = tcp_sock_init(c, addr, ifname, i);
> -			else if (optname == 'u')
> -				ret = udp_sock_init(c, 0, addr, ifname, i);
> -			if (ret)
> -				goto bind_fail;
> -		}
> +		conf_ports_range_except(c, optname, optarg, fwd,
> +					addr, ifname,
> +					orig_range.first, orig_range.last,
> +					exclude,
> +					mapped_range.first, false);
>  	} while ((p = next_chunk(p, ',')));
>  
>  	return;
> -enfile:
> -	die("Can't open enough sockets for port specifier: %s", optarg);
>  bad:
>  	die("Invalid port specifier %s", optarg);
>  mode_conflict:
>  	die("Port forwarding mode '%s' conflicts with previous mode", optarg);
> -bind_fail:
> -	die("Failed to bind port %u (%s) for option '-%c %s', exiting",
> -	    i, strerror_(-ret), optname, optarg);
> -bind_all_fail:
> -	die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
>  }
>  
>  /**

-- 
Stefano