public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH v2] conf: Support CIDR notation for -a/--address option
@ 2025-12-17 21:31 Jon Maloy
  2025-12-17 22:51 ` Stefano Brivio
  2025-12-18  4:22 ` David Gibson
  0 siblings, 2 replies; 3+ messages in thread
From: Jon Maloy @ 2025-12-17 21:31 UTC (permalink / raw)
  To: sbrivio, dgibson, david, jmaloy, passt-dev

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() helper function that:
- Parses address strings with optional /prefix suffix
- Validates prefix length based on address family (0-32 for IPv4,
  0-128 for IPv6)
- Returns address family and fills address/prefix output parameters

For IPv4, the prefix is stored in ip4.prefix_len when provided.
Multiple CIDR addresses use last-wins semantics for the prefix,
consistent with how addresses are handled currently. However,
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>

---
v2: Fixed incorrect error printout, as noted by Laurent Vivier.
    We now keep the old semantics, i.e., allowing multiple -a
    options for each protocol. This semantics looks wrong,
    but will anyway be fixed in my upcoming series.

Signed-off-by: Jon Maloy <jmaloy@redhat.com>
---
 conf.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 84 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c
index 2942c8c..764b910 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,56 @@ static int conf_ip4_prefix(const char *arg)
 	return len;
 }
 
+/**
+ * conf_addr_prefix() - Parse address with optional /prefix notation
+ * @arg:	Address string, optionally with /prefix
+ * @addr4:	Output for IPv4 address
+ * @addr6:	Output for IPv6 address
+ * @prefix:	Output for prefix length (0 if not specified)
+ *
+ * Return: AF_INET for IPv4, AF_INET6 for IPv6, -1 on error
+ */
+static int conf_addr_prefix(const char *arg, struct in_addr *addr4,
+			    struct in6_addr *addr6, int *prefix)
+{
+	char buf[INET6_ADDRSTRLEN + sizeof("/128")];
+	char *slash;
+
+	*prefix = 0;
+
+	if (snprintf(buf, sizeof(buf), "%s", arg) >= (int)sizeof(buf))
+		return -1;
+
+	/* Check for /prefix suffix */
+	slash = strchr(buf, '/');
+	if (slash) {
+		unsigned long len;
+		char *end;
+
+		*slash = '\0';
+		errno = 0;
+		len = strtoul(slash + 1, &end, 10);
+		if (errno || *end)
+			return -1;
+
+		*prefix = len;
+	}
+
+	if (inet_pton(AF_INET6, buf, addr6) == 1) {
+		if (*prefix > 128)
+			return -1;
+		return AF_INET6;
+	}
+
+	if (inet_pton(AF_INET, buf, addr4) == 1) {
+		if (*prefix > 32)
+			return -1;
+		return AF_INET;
+	}
+
+	return -1;
+}
+
 /**
  * conf_ip4() - Verify or detect IPv4 support, get relevant addresses
  * @ifi:	Host interface to attempt (0 to determine one)
@@ -896,7 +946,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[/PREFIX]\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 +1549,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 +1859,38 @@ 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': {
+			struct in6_addr addr6;
+			struct in_addr addr4;
+			int prefix = 0;
+			int af;
+
+			af = conf_addr_prefix(optarg, &addr4, &addr6, &prefix);
+
+			if (af == AF_INET6 &&
+			    !IN6_IS_ADDR_UNSPECIFIED(&addr6)	&&
+			    !IN6_IS_ADDR_LOOPBACK(&addr6)	&&
+			    !IN6_IS_ADDR_V4MAPPED(&addr6)	&&
+			    !IN6_IS_ADDR_V4COMPAT(&addr6)	&&
+			    !IN6_IS_ADDR_MULTICAST(&addr6)) {
+				c->ip6.addr = addr6;
 				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)) {
+			if (af == AF_INET &&
+			    !IN4_IS_ADDR_UNSPECIFIED(&addr4)	&&
+			    !IN4_IS_ADDR_BROADCAST(&addr4)	&&
+			    !IN4_IS_ADDR_LOOPBACK(&addr4)	&&
+			    !IN4_IS_ADDR_MULTICAST(&addr4)) {
+				c->ip4.addr = addr4;
+				if (prefix) {
+					if (prefix_from_opt)
+						die("Can't use both -n and CIDR prefix");
+					c->ip4.prefix_len = prefix;
+					prefix_from_cidr = true;
+				}
 				if (c->mode == MODE_PASTA)
 					c->ip4.no_copy_addrs = true;
 				break;
@@ -1832,11 +1898,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");
 			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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] conf: Support CIDR notation for -a/--address option
  2025-12-17 21:31 [PATCH v2] conf: Support CIDR notation for -a/--address option Jon Maloy
@ 2025-12-17 22:51 ` Stefano Brivio
  2025-12-18  4:22 ` David Gibson
  1 sibling, 0 replies; 3+ messages in thread
From: Stefano Brivio @ 2025-12-17 22:51 UTC (permalink / raw)
  To: Jon Maloy; +Cc: dgibson, david, passt-dev

On Wed, 17 Dec 2025 16:31:42 -0500
Jon Maloy <jmaloy@redhat.com> 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() helper function that:
> - Parses address strings with optional /prefix suffix

That's not the prefix, that's the prefix length.

> - Validates prefix length based on address family (0-32 for IPv4,
>   0-128 for IPv6)
> - Returns address family and fills address/prefix output parameters
> 
> For IPv4, the prefix is stored in ip4.prefix_len when provided.
> Multiple CIDR addresses use last-wins semantics for the prefix,
> consistent with how addresses are handled currently. However,
> 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>
> 
> ---
> v2: Fixed incorrect error printout, as noted by Laurent Vivier.
>     We now keep the old semantics, i.e., allowing multiple -a
>     options for each protocol. This semantics looks wrong,
>     but will anyway be fixed in my upcoming series.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 84 insertions(+), 15 deletions(-)

man page update is missing altogether.

Not a full review, but it will be easier to review once the new
behaviour is documented.

-- 
Stefano


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] conf: Support CIDR notation for -a/--address option
  2025-12-17 21:31 [PATCH v2] conf: Support CIDR notation for -a/--address option Jon Maloy
  2025-12-17 22:51 ` Stefano Brivio
@ 2025-12-18  4:22 ` David Gibson
  1 sibling, 0 replies; 3+ messages in thread
From: David Gibson @ 2025-12-18  4:22 UTC (permalink / raw)
  To: Jon Maloy; +Cc: sbrivio, dgibson, passt-dev

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

On Wed, Dec 17, 2025 at 04:31:42PM -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() helper function that:
> - Parses address strings with optional /prefix suffix
> - Validates prefix length based on address family (0-32 for IPv4,
>   0-128 for IPv6)
> - Returns address family and fills address/prefix output parameters
> 
> For IPv4, the prefix is stored in ip4.prefix_len when provided.
> Multiple CIDR addresses use last-wins semantics for the prefix,
> consistent with how addresses are handled currently. However,
> 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.

Ouch.

> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

Missing man page update as Stefano mentioned, and a handful of nits below.

> 
> ---
> v2: Fixed incorrect error printout, as noted by Laurent Vivier.
>     We now keep the old semantics, i.e., allowing multiple -a
>     options for each protocol. This semantics looks wrong,
>     but will anyway be fixed in my upcoming series.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  conf.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 84 insertions(+), 15 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 2942c8c..764b910 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,56 @@ static int conf_ip4_prefix(const char *arg)
>  	return len;
>  }
>  
> +/**
> + * conf_addr_prefix() - Parse address with optional /prefix notation
> + * @arg:	Address string, optionally with /prefix
> + * @addr4:	Output for IPv4 address
> + * @addr6:	Output for IPv6 address
> + * @prefix:	Output for prefix length (0 if not specified)
> + *
> + * Return: AF_INET for IPv4, AF_INET6 for IPv6, -1 on error
> + */
> +static int conf_addr_prefix(const char *arg, struct in_addr *addr4,
> +			    struct in6_addr *addr6, int *prefix)

Would it make sense to use union inany_addr for the output?  That
would also let you reuse inany_pton().

> +{
> +	char buf[INET6_ADDRSTRLEN + sizeof("/128")];
> +	char *slash;
> +
> +	*prefix = 0;
> +
> +	if (snprintf(buf, sizeof(buf), "%s", arg) >= (int)sizeof(buf))
> +		return -1;

Complicated way of doing a strncpy.  But more to the point, I don't
think we need to copy the argument - we already add \0s in place in
conf_ports(), so I think it's fine to do it here too.

> +	/* Check for /prefix suffix */

prefix length, as Stefano mentioned.

> +	slash = strchr(buf, '/');
> +	if (slash) {
> +		unsigned long len;
> +		char *end;
> +
> +		*slash = '\0';
> +		errno = 0;
> +		len = strtoul(slash + 1, &end, 10);
> +		if (errno || *end)
> +			return -1;
> +
> +		*prefix = len;
> +	}
> +
> +	if (inet_pton(AF_INET6, buf, addr6) == 1) {
> +		if (*prefix > 128)
> +			return -1;
> +		return AF_INET6;
> +	}
> +
> +	if (inet_pton(AF_INET, buf, addr4) == 1) {
> +		if (*prefix > 32)
> +			return -1;
> +		return AF_INET;
> +	}
> +
> +	return -1;
> +}
> +
>  /**
>   * conf_ip4() - Verify or detect IPv4 support, get relevant addresses
>   * @ifi:	Host interface to attempt (0 to determine one)
> @@ -896,7 +946,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[/PREFIX]\n"

prefix length

>  		"    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 +1549,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 +1859,38 @@ 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': {
> +			struct in6_addr addr6;
> +			struct in_addr addr4;
> +			int prefix = 0;
> +			int af;
> +
> +			af = conf_addr_prefix(optarg, &addr4, &addr6, &prefix);
> +
> +			if (af == AF_INET6 &&
> +			    !IN6_IS_ADDR_UNSPECIFIED(&addr6)	&&
> +			    !IN6_IS_ADDR_LOOPBACK(&addr6)	&&
> +			    !IN6_IS_ADDR_V4MAPPED(&addr6)	&&
> +			    !IN6_IS_ADDR_V4COMPAT(&addr6)	&&
> +			    !IN6_IS_ADDR_MULTICAST(&addr6)) {
> +				c->ip6.addr = addr6;
>  				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)) {
> +			if (af == AF_INET &&
> +			    !IN4_IS_ADDR_UNSPECIFIED(&addr4)	&&
> +			    !IN4_IS_ADDR_BROADCAST(&addr4)	&&
> +			    !IN4_IS_ADDR_LOOPBACK(&addr4)	&&
> +			    !IN4_IS_ADDR_MULTICAST(&addr4)) {
> +				c->ip4.addr = addr4;
> +				if (prefix) {
> +					if (prefix_from_opt)
> +						die("Can't use both -n and CIDR prefix");
> +					c->ip4.prefix_len = prefix;
> +					prefix_from_cidr = true;
> +				}
>  				if (c->mode == MODE_PASTA)
>  					c->ip4.no_copy_addrs = true;
>  				break;
> @@ -1832,11 +1898,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");
>  			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 --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-12-18  4:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-17 21:31 [PATCH v2] conf: Support CIDR notation for -a/--address option Jon Maloy
2025-12-17 22:51 ` Stefano Brivio
2025-12-18  4:22 ` David Gibson

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).