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

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

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

On Wed, Feb 04, 2026 at 07:56:35PM -0500, Jon Maloy wrote:
> 
> 
> On 2026-02-04 07:50, David Gibson wrote:
> > On Fri, Jan 30, 2026 at 04:44:37PM -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.
> > > 
> > > We add a new inany_prefix_pton() helper function that:
> > > - Parses address strings with optional /prefix_len suffix
> > > - Validates prefix length based on address family (0-32 for IPv4,
> > >    0-128 for IPv6), including handling of IPv4-to-IPv6 mapping case.
> > > - Returns identified address family, if any.
> > > 
> > > 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>
> > > 
> > > ---
> > > v3: Fixes after feedback from Laurent, David and Stefano
> > >      Notably, updated man page for the -a option
> > > 
> > > v4: Fixes based on feedback from David G:
> > >    - Handling prefix length adjustment when IPv4-to-IPv6 mapping
> > >    - Removed redundant !IN6_IS_ADDR_V4MAPPED(&addr.a6) test
> > >    - Simplified tests of acceptable address types
> > >    - Merged documentation and code commits
> > >    - Some documentation text clarifications
> > > 
> > > v5: - Moved address/prefix parsing into a refactored
> > >        inany_prefix_pton() function.
> > >      - inany_prefix_pton() now only caluclates IPv6 style
> > >        prefix lengths
> > >      - Stricter distinction between error causes.
> > >      - Some refactoring of the 'case a:' branch in conf()
> > >      - Some small fixes in passt.1
> > > ---
> > >   conf.c  | 58 +++++++++++++++++++++++++++++-------------------
> > >   inany.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   inany.h |  1 +
> > >   ip.c    | 21 ++++++++++++++++++
> > >   ip.h    |  2 ++
> > >   passt.1 | 17 ++++++++++-----
> > >   6 files changed, 139 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/conf.c b/conf.c
> > > index 2942c8c..98d5d17 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;
> > >   	}
> > > @@ -896,7 +896,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"
> > > @@ -1498,6 +1498,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >   	const char *optstring = "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> > >   	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > >   	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
> > > +	bool prefix_from_cidr = false, prefix_from_opt = false;
> > >   	bool copy_addrs_opt = false, copy_routes_opt = false;
> > >   	enum fwd_ports_mode fwd_default = FWD_NONE;
> > >   	bool v4_only = false, v6_only = false;
> > > @@ -1808,35 +1809,46 @@ 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)) {
> > > -				if (c->mode == MODE_PASTA)
> > > -					c->ip6.no_copy_addrs = true;
> > > -				break;
> > > -			}
> > > -
> > > -			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)) {
> > > +		case 'a': {
> > > +			union inany_addr addr = { 0 };
> > 
> > You're unconditionally calling inany_prefix_pton(), so you shouldn't
> > need to initialise this.
> 
> Yes, to guarantee I get an invalid address in case the parsing fails.
> However, I should do it inside the funtion instead.

Right.

> > 
> > > +			int prefix_len = 0;
> > 
> > Or this.
> Same.
> > 
> > > +			int af;
> > > +
> > > +			af = inany_prefix_pton(optarg, &addr, &prefix_len);
> > 
> > You need to check for errors from inany_prefix_pton().
> 
> I do that. Further down.

Ok, but it's hard to follow when separated from the call like that.

> 
> > 
> > > +			if (inany_is_unspecified(&addr) ||
> > > +			    inany_is_multicast(&addr) ||
> > > +			    inany_is_loopback(&addr) ||
> > > +			    IN6_IS_ADDR_V4COMPAT(&addr.a6))
> > > +				die("Invalid address: %s", optarg);
> > 
> > For sanity / extensibility, it probably also makes to fail here if af
> > is not either AF_INET or AF_INET6.
> 
> This is what I do further down.

Ah, true.

> 
> > 
> > > +
> > > +			if (prefix_len && !prefix_from_opt)
> > > +				prefix_from_cidr = true;
> > > +			else if (prefix_len)
> > > +				die("Can't mix CIDR with -n");
> > > +
> > > +			if (af == AF_INET) {
> > > +				c->ip4.addr = *inany_v4(&addr);
> > > +				c->ip4.prefix_len = prefix_len ? prefix_len - 96 :
> > > +					ip4_class_prefix_len(&c->ip4.addr);
> > >   				if (c->mode == MODE_PASTA)
> > >   					c->ip4.no_copy_addrs = true;
> > > -				break;
> > > +			} else if (af == AF_INET6) {
> > > +				c->ip6.addr = addr.a6;
> > > +				if (c->mode == MODE_PASTA)
> > > +					c->ip6.no_copy_addrs = true;
> > > +			} else {
> > > +				die("Invalid prefix length: %d", prefix_len);
> > 
> > This error message does not seem to match the conditions that trigger it.
> 
> The only way you can obtain a valid address and a return value different
> from AF_INET
> and AF_INET6 is that the prefix_len parsing failed or the value was invalid.

Right, but it's a minor layering violation to assume that in the caller.

> All the above is logically correct, although we can discuss if it is the
> cleanest
> and best algorithm. The fact that you misunderstand this is a sign it is
> not.
> 
> > 
> > >   			}
> > > -
> > > -			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);
> > > diff --git a/inany.c b/inany.c
> > > index 7680439..00d44e0 100644
> > > --- a/inany.c
> > > +++ b/inany.c
> > > @@ -11,6 +11,7 @@
> > >   #include <assert.h>
> > >   #include <netinet/in.h>
> > >   #include <arpa/inet.h>
> > > +#include <errno.h>
> > >   #include "util.h"
> > >   #include "ip.h"
> > > @@ -57,3 +58,70 @@ int inany_pton(const char *src, union inany_addr *dst)
> > >   	return 0;
> > >   }
> > > +
> > > +/** inany_prefix_pton - Parse an IPv[46] address with prefix length adjustment
> > 
> > "adjustment" doesn't make sense to me here.
> > 
> > > + * @src:	IPv[46] address string
> > > + * @dst:	output buffer, filled with parsed address
> > > + * @prefix_len:	pointer to prefix length
> > > + *
> > > + * Return: AF_INET for IPv4, AF_INET6 for IPv6, -1 on error
> > 
> > I'm not particularly convinced that returning the address family here
> > is useful, since the caller can call inany_v4() just as easily as we
> > can.  But if we do return the address family, the it probably makes
> > more sense to use AF_UNSPEC for errors, rather than -1, since it's not
> > - strictly speaking - guaranteed that one of the AF_* constants has
> > the value -1.
> 
> AF_UNSPEC is a valid code, meaning "any" or "don't care".
> That doesn't sound like a suitable error code, and we do want to know
> if the parsing failed.
> I think I'll go for just a 0/-1 return value instead.

Ok.

> > > + */
> > > +int inany_prefix_pton(char *src, union inany_addr *dst, int *prefix_len)
> > 
> > If we are returning an address family, the return type should be sa_family_t.
> Ok.
> > 
> > > +{
> > > +	bool mapped = false;
> > > +	struct in6_addr a6;
> > > +	struct in_addr a4;
> > > +	char *slash;
> > > +	char *end;
> > > +	int af;
> > > +
> > > +	*prefix_len = 0;
> > 
> > As noted below, 0 is not a suitable value for "missing prefix",
> > because 0 length prefixes are real and important.
> 
> 0 is a valid value, but the question is if /0 ia a valid suffix
> in our CIDR format. I see below that you think so.

For -a, maybe not.  But we want to be able to re-use this for things
where it will be.

> > I think it would be
> > better to make the prefix length non-optional for this functional.
> > The caller can fall back to inany_pton() if this fails.  That makes
> > this function more easily reusable for cases where we _require_ a
> > prefix length.
> 
> I can try that.
> 
> > 
> > > +
> > > +	/* Check for presence of /prefix_len suffix */
> > > +	slash = strchr(src, '/');
> > > +	if (slash)
> > > +		*slash = '\0';
> > > +
> > > +	/* Read address */
> > > +	if (inet_pton(AF_INET, src, &a4)) {
> > > +		inany_from_af(dst, AF_INET, &a4);
> > > +		af = AF_INET;
> > > +	} else if (inet_pton(AF_INET6, src, &a6)) {
> > > +		inany_from_af(dst, AF_INET6, &a6);
> > > +		af = AF_INET6;
> > > +		if (inany_v4(dst))
> > > +			mapped = true;
> > > +	} else {
> > > +		memset(dst, 0, sizeof(*dst));
> > > +		return -1;
> > > +	}
> > > +
> > > +	if (!slash)
> > > +		return mapped ? AF_INET : af;
> > 
> > You can avoid messing around with the mapped temporary by
> > unconditionally deriving the address family from inany_v4().
> 
> Ok.
> 
> > 
> > > +
> > > +	/* Read prefix_len - /0 is not allowed */
> > 
> > /0 should be allowed.  It doesn't make much sense for -a, but it's
> > valid and important if we use this in future for routes (a 0 length
> > prefix indicates a default route).
> 
> ok. That changes a few things.
> 
> > 
> > > +	errno = 0;
> > > +	*prefix_len = strtoul(slash + 1, &end, 10);
> > > +	if (errno || *end || *prefix_len == 0)
> > > +		return -1;
> > > +
> > > +	if (mapped) {
> > > +		/* IPv4-mapped: prefix already in IPv6 format, must be 96-128 */
> > > +		if (*prefix_len < 96 || *prefix_len > 128)
> > > +			return -1;
> > > +		return AF_INET;
> > > +	}
> > > +
> > > +	if (af == AF_INET) {
> > > +		/* Native IPv4: convert to IPv6 format */
> > > +		if (*prefix_len > 32)
> > > +			return -1;
> > > +		*prefix_len += 96;
> > 
> > If you make this adjustment before the previous stanza you again don't
> > need the 'mapped' local and can use inany_v4().
> 
> Good point.
> 
> > 
> > > +		return AF_INET;
> > > +	}
> > > +
> > > +	/* Native IPv6: keep as-is */
> > > +	if (*prefix_len > 128)
> > > +		return -1;
> > 
> > And if you move this before the if (mapped) stanza, you can remove the
> > duplicated check for > 128.
> 
> I'll give it a try.
> 
> ///jon
> 
> > 
> > > +	return AF_INET6;
> > > +}
> > > diff --git a/inany.h b/inany.h
> > > index 61b36fb..316ee44 100644
> > > --- a/inany.h
> > > +++ b/inany.h
> > > @@ -295,5 +295,6 @@ static inline void inany_siphash_feed(struct siphash_state *state,
> > >   const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size);
> > >   int inany_pton(const char *src, union inany_addr *dst);
> > > +int inany_prefix_pton(char *src, union inany_addr *dst, int *prefix_len);
> > >   #endif /* INANY_H */
> > > diff --git a/ip.c b/ip.c
> > > index 9a7f4c5..40dc24e 100644
> > > --- a/ip.c
> > > +++ b/ip.c
> > > @@ -13,6 +13,8 @@
> > >    */
> > >   #include <stddef.h>
> > > +#include <netinet/in.h>
> > > +
> > >   #include "util.h"
> > >   #include "ip.h"
> > > @@ -67,3 +69,22 @@ found:
> > >   	*proto = nh;
> > >   	return true;
> > >   }
> > > +
> > > +/**
> > > + * ip4_class_prefix_len() - Get class based prefix length for IPv4 address
> > > + * @addr:	IPv4 address
> > > + *
> > > + * Return: prefix length based on address class, or 32 for other
> > > + */
> > > +int ip4_class_prefix_len(const struct in_addr *addr)
> > > +{
> > > +	in_addr_t a = ntohl(addr->s_addr);
> > > +
> > > +	if (IN_CLASSA(a))
> > > +		return 32 - IN_CLASSA_NSHIFT;
> > > +	if (IN_CLASSB(a))
> > > +		return 32 - IN_CLASSB_NSHIFT;
> > > +	if (IN_CLASSC(a))
> > > +		return 32 - IN_CLASSC_NSHIFT;
> > > +	return 32;
> > > +}
> > > diff --git a/ip.h b/ip.h
> > > index 5830b92..bd28640 100644
> > > --- a/ip.h
> > > +++ b/ip.h
> > > @@ -135,4 +135,6 @@ static const struct in_addr in4addr_broadcast = { 0xffffffff };
> > >   #define IPV6_MIN_MTU		1280
> > >   #endif
> > > +int ip4_class_prefix_len(const struct in_addr *addr);
> > > +
> > >   #endif /* IP_H */
> > > diff --git a/passt.1 b/passt.1
> > > index db0d662..53537c4 100644
> > > --- a/passt.1
> > > +++ b/passt.1
> > > @@ -156,10 +156,14 @@ By default, the advertised MTU is 65520 bytes, that is, the maximum 802.3 MTU
> > >   minus the length of a 802.3 header, rounded to 32 bits (IPv4 words).
> > >   .TP
> > > -.BR \-a ", " \-\-address " " \fIaddr
> > > +.BR \-a ", " \-\-address " " \fIaddr\fR[/\fIprefix_len\fR]
> > >   Assign IPv4 \fIaddr\fR via DHCP (\fByiaddr\fR), or \fIaddr\fR via DHCPv6 (option
> > >   5) and an \fIaddr\fR-based prefix via NDP Router Advertisement (option type 3)
> > >   for an IPv6 \fIaddr\fR.
> > > +An optional /\fIprefix_len\fR (1-32 for IPv4, 1-128 for IPv6) can be
> > > +appended in CIDR notation (e.g. 192.0.2.1/24). This is an alternative to
> > > +using the \fB-n\fR, \fB--netmask\fR option. Mixing CIDR notation with
> > > +\fB-n\fR results in an error.
> > >   This option can be specified zero (for defaults) to two times (once for IPv4,
> > >   once for IPv6).
> > >   By default, assigned IPv4 and IPv6 addresses are taken from the host interfaces
> > > @@ -172,10 +176,13 @@ is assigned for IPv4, and no additional address will be assigned for IPv6.
> > >   .TP
> > >   .BR \-n ", " \-\-netmask " " \fImask
> > >   Assign IPv4 netmask \fImask\fR, expressed as dot-decimal or number of bits, via
> > > -DHCP (option 1).
> > > -By default, the netmask associated to the host address matching the assigned one
> > > -is used. If there's no matching address on the host, the netmask is determined
> > > -according to the CIDR block of the assigned address (RFC 4632).
> > > +DHCP (option 1). Alternatively, the prefix length can be specified using CIDR
> > > +notation with the \fB-a\fR, \fB--address\fR option (e.g. \fB-a\fR 192.0.2.1/24).
> > > +Mixing \fB-n\fR with CIDR notation results in an error.
> > > +If no address is indicated, the netmask associated with the adopted host address,
> > > +if any, is used. If an address is indicated, but without a prefix length, the
> > > +netmask is determined based on the corresponding network class. In all other
> > > +cases, the netmask is determined by using the indicated prefix length.
> > >   .TP
> > >   .BR \-M ", " \-\-mac-addr " " \fIaddr
> > > -- 
> > > 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 --]