Re: [PATCH v3 1/2] conf: Support CIDR notation for -a/--address option

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

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

On Thu, Dec 18, 2025 at 05:22:12PM -0500, Jon Maloy wrote:
> Extend the -a/--address option to accept addresses in CIDR notation
> (e.g., 192.168.1.1/24 or 2001:db8::1/64) as an alternative to using
> separate -a and -n options.
> 
> Add conf_addr_prefix_len() helper function that:
> - Parses address strings with optional /prefix_len suffix using inany_pton()
> - Validates prefix length based on address family (0-32 for IPv4,
>   0-128 for IPv6)
> - Returns address family via union inany_addr output parameter
> 
> For IPv4, the prefix length is stored in ip4.prefix_len when provided.
> Mixing -n and CIDR notation results in an error to catch likely user
> mistakes.
> 
> Also fix a bug in conf_ip4_prefix() that was incorrectly using the
> global 'optarg' instead of its 'arg' parameter.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 2942c8c..81a6ca3 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -682,7 +682,7 @@ static int conf_ip4_prefix(const char *arg)
>  			return -1;
>  	} else {
>  		errno = 0;
> -		len = strtoul(optarg, NULL, 0);
> +		len = strtoul(arg, NULL, 0);
>  		if (len > 32 || errno)
>  			return -1;
>  	}
> @@ -690,6 +690,50 @@ static int conf_ip4_prefix(const char *arg)
>  	return len;
>  }
>  
> +/**
> + * conf_addr_prefix_len() - Parse address with optional prefix length
> + * @arg:	Address string, optionally with /prefix_len suffix (modified)
> + * @addr:	Output for parsed address
> + * @prefix_len:	Output for prefix length (0 if not specified)

This makes me slightly nervous, because a 0-length prefix is
theoretically valid.  Telling the guest that the entire internet is on
it's LAN is pretty weird, but it does potentially have a use: it means
the guest can reach the world without having a default gateway (at the
cost of requiring many ARP entries in the guest).

> + *
> + * Return: AF_INET for IPv4, AF_INET6 for IPv6, -1 on error
> + */
> +static int conf_addr_prefix_len(char *arg, union inany_addr *addr,
> +				int *prefix_len)
> +{
> +	char *slash;
> +
> +	*prefix_len = 0;
> +
> +	/* Check for /prefix_len suffix */
> +	slash = strchr(arg, '/');
> +	if (slash) {
> +		unsigned long len;
> +		char *end;
> +
> +		*slash = '\0';
> +		errno = 0;
> +		len = strtoul(slash + 1, &end, 10);
> +		if (errno || *end)
> +			return -1;
> +
> +		*prefix_len = len;
> +	}
> +
> +	if (!inany_pton(arg, addr))
> +		return -1;
> +
> +	if (inany_v4(addr)) {
> +		if (*prefix_len > 32)
> +			return -1;
> +		return AF_INET;

Ah... sorry.. I just realised there's a subtle problem using
inany_pton() here.  The above is correct if the user entered an IPv4
address.  However, it's possible they could have explicitly entered a
v4-mapped IPv6 address.  inany_pton() will interpret that like a an
IPv4 address, which I think is correct, *except* that
	::ffff:192.168.1.1/112
should be interpreted like:
	192.168.1.1/16
not like:
	192.168.1.1/112

Maybe we actually want to create an inany_prefix_pton() or something.

> +	}
> +
> +	if (*prefix_len > 128)
> +		return -1;
> +	return AF_INET6;
> +}
> +
>  /**
>   * conf_ip4() - Verify or detect IPv4 support, get relevant addresses
>   * @ifi:	Host interface to attempt (0 to determine one)
> @@ -896,7 +940,7 @@ static void usage(const char *name, FILE *f, int status)
>  		"    a zero value disables assignment\n"
>  		"    default: 65520: maximum 802.3 MTU minus 802.3 header\n"
>  		"                    length, rounded to 32 bits (IPv4 words)\n"
> -		"  -a, --address ADDR	Assign IPv4 or IPv6 address ADDR\n"
> +		"  -a, --address ADDR	Assign IPv4 or IPv6 address ADDR[/PREFIXLEN]\n"
>  		"    can be specified zero to two times (for IPv4 and IPv6)\n"
>  		"    default: use addresses from interface with default route\n"
>  		"  -n, --netmask MASK	Assign IPv4 MASK, dot-decimal or bits\n"
> @@ -1499,6 +1543,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
>  	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
>  	bool copy_addrs_opt = false, copy_routes_opt = false;
> +	bool prefix_from_cidr = false, prefix_from_opt = false;
>  	enum fwd_ports_mode fwd_default = FWD_NONE;
>  	bool v4_only = false, v6_only = false;
>  	unsigned dns4_idx = 0, dns6_idx = 0;
> @@ -1808,23 +1853,39 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->mtu = mtu;
>  			break;
>  		}
> -		case 'a':
> -			if (inet_pton(AF_INET6, optarg, &c->ip6.addr)	&&
> -			    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr)	&&
> -			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.addr)		&&
> -			    !IN6_IS_ADDR_V4MAPPED(&c->ip6.addr)		&&
> -			    !IN6_IS_ADDR_V4COMPAT(&c->ip6.addr)		&&
> -			    !IN6_IS_ADDR_MULTICAST(&c->ip6.addr)) {
> +		case 'a': {
> +			union inany_addr addr;
> +			const struct in_addr *a4;
> +			int prefix_len = 0;
> +			int af;
> +
> +			af = conf_addr_prefix_len(optarg, &addr, &prefix_len);
> +
> +			if (af == AF_INET6 &&
> +			    !IN6_IS_ADDR_UNSPECIFIED(&addr.a6)	&&
> +			    !IN6_IS_ADDR_LOOPBACK(&addr.a6)	&&
> +			    !IN6_IS_ADDR_V4MAPPED(&addr.a6)	&&

The V4MAPPED test is no longer relevant, since if it was true,
conf_addr_prefix_len() could not return AF_INET6.

> +			    !IN6_IS_ADDR_V4COMPAT(&addr.a6)	&&
> +			    !IN6_IS_ADDR_MULTICAST(&addr.a6)) {
> +				c->ip6.addr = addr.a6;
>  				if (c->mode == MODE_PASTA)
>  					c->ip6.no_copy_addrs = true;
>  				break;
>  			}

Note that we do have inany_is_unicast(), inany_is_loopback() and
inany_is_unspecified() helpers that might allow us to simplify this.


> -			if (inet_pton(AF_INET, optarg, &c->ip4.addr)	&&
> -			    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr)	&&
> -			    !IN4_IS_ADDR_BROADCAST(&c->ip4.addr)	&&
> -			    !IN4_IS_ADDR_LOOPBACK(&c->ip4.addr)		&&
> -			    !IN4_IS_ADDR_MULTICAST(&c->ip4.addr)) {
> +			a4 = inany_v4(&addr);
> +			if (af == AF_INET && a4 &&
> +			    !IN4_IS_ADDR_UNSPECIFIED(a4)	&&
> +			    !IN4_IS_ADDR_BROADCAST(a4)	&&
> +			    !IN4_IS_ADDR_LOOPBACK(a4)	&&
> +			    !IN4_IS_ADDR_MULTICAST(a4)) {
> +				c->ip4.addr = *a4;
> +				if (prefix_len) {
> +					if (prefix_from_opt)
> +						die("Can't use both -n and CIDR prefix length");
> +					c->ip4.prefix_len = prefix_len;
> +					prefix_from_cidr = true;
> +				}
>  				if (c->mode == MODE_PASTA)
>  					c->ip4.no_copy_addrs = true;
>  				break;
> @@ -1832,11 +1893,14 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  			die("Invalid address: %s", optarg);
>  			break;
> +		}
>  		case 'n':
> +			if (prefix_from_cidr)
> +				die("Can't use both -n and CIDR prefix length");
>  			c->ip4.prefix_len = conf_ip4_prefix(optarg);
>  			if (c->ip4.prefix_len < 0)
>  				die("Invalid netmask: %s", optarg);
> -
> +			prefix_from_opt = true;
>  			break;
>  		case 'M':
>  			parse_mac(c->our_tap_mac, optarg);
> -- 
> 2.52.0
> 

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