From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202512 header.b=eBSMdNjO; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 7B54C5A004E for ; Fri, 06 Feb 2026 04:26:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202512; t=1770348373; bh=24Bo9RFlUEE7ZGm0lzU/HJjy83aCY1pHkLedlBZpvIg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eBSMdNjOau9gW0x8HAP60ESuWyPQA+wQ72PEEMDQM1gWHFcRAwr8eJ98wTJYemneW jPWs4LBg5uIsaI+i1p31wvoUJ/JyGfqSK3aMxlbRRB4hmL9+M6SOSkqsjzWCmjexiX HLNPVNNdqzSCHzJvSZJ05tblYb6bHu0sc/04KtyqXLH6OO6ResBGR6Z6vuYmwcnvwC B6qvj/43AR7h7Cl1wBcWlpQoY7+HlVKhi12t5S+2XXsqyaphx8bl0Hkg9oG3OB/O5k nH7xn0BbwkZweHOoLufMMlwm0LbuAv3FD6r/3ur+t4nqjGYz3xSSTf5k7Kbs7p8nRu 9dpsK6bsT67Qw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4f6fd101cXz4w9k; Fri, 06 Feb 2026 14:26:12 +1100 (AEDT) Date: Fri, 6 Feb 2026 13:26:05 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v3 01/11] conf: Support CIDR notation for -a/--address option Message-ID: References: <20260130214447.2540791-1-jmaloy@redhat.com> <20260130214447.2540791-2-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="dJ94ysSQcZV1g74u" Content-Disposition: inline In-Reply-To: Message-ID-Hash: U3R6OXLBRRJF2VSTZ3FYYBCNCBYSKBI7 X-Message-ID-Hash: U3R6OXLBRRJF2VSTZ3FYYBCNCBYSKBI7 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: sbrivio@redhat.com, dgibson@redhat.com, passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --dJ94ysSQcZV1g74u Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 04, 2026 at 07:56:35PM -0500, Jon Maloy wrote: >=20 >=20 > 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. > > >=20 > > > 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. > > >=20 > > > 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. > > >=20 > > > Also fix a bug in conf_ip4_prefix() that was incorrectly using the > > > global 'optarg' instead of its 'arg' parameter. > > >=20 > > > Signed-off-by: Jon Maloy > > >=20 > > > --- > > > v3: Fixes after feedback from Laurent, David and Stefano > > > Notably, updated man page for the -a option > > >=20 > > > 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 > > >=20 > > > 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(-) > > >=20 > > > 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 =3D 0; > > > - len =3D strtoul(optarg, NULL, 0); > > > + len =3D 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 =3D "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:4= 61t:u:T:U:"; > > > const char *logname =3D (c->mode =3D=3D MODE_PASTA) ? "pasta" : "p= asst"; > > > char userns[PATH_MAX] =3D { 0 }, netns[PATH_MAX] =3D { 0 }; > > > + bool prefix_from_cidr =3D false, prefix_from_opt =3D false; > > > bool copy_addrs_opt =3D false, copy_routes_opt =3D false; > > > enum fwd_ports_mode fwd_default =3D FWD_NONE; > > > bool v4_only =3D false, v6_only =3D false; > > > @@ -1808,35 +1809,46 @@ void conf(struct ctx *c, int argc, char **arg= v) > > > c->mtu =3D 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 =3D=3D MODE_PASTA) > > > - c->ip6.no_copy_addrs =3D 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 =3D { 0 }; > >=20 > > You're unconditionally calling inany_prefix_pton(), so you shouldn't > > need to initialise this. >=20 > Yes, to guarantee I get an invalid address in case the parsing fails. > However, I should do it inside the funtion instead. Right. > >=20 > > > + int prefix_len =3D 0; > >=20 > > Or this. > Same. > >=20 > > > + int af; > > > + > > > + af =3D inany_prefix_pton(optarg, &addr, &prefix_len); > >=20 > > You need to check for errors from inany_prefix_pton(). >=20 > I do that. Further down. Ok, but it's hard to follow when separated from the call like that. >=20 > >=20 > > > + if (inany_is_unspecified(&addr) || > > > + inany_is_multicast(&addr) || > > > + inany_is_loopback(&addr) || > > > + IN6_IS_ADDR_V4COMPAT(&addr.a6)) > > > + die("Invalid address: %s", optarg); > >=20 > > For sanity / extensibility, it probably also makes to fail here if af > > is not either AF_INET or AF_INET6. >=20 > This is what I do further down. Ah, true. >=20 > >=20 > > > + > > > + if (prefix_len && !prefix_from_opt) > > > + prefix_from_cidr =3D true; > > > + else if (prefix_len) > > > + die("Can't mix CIDR with -n"); > > > + > > > + if (af =3D=3D AF_INET) { > > > + c->ip4.addr =3D *inany_v4(&addr); > > > + c->ip4.prefix_len =3D prefix_len ? prefix_len - 96 : > > > + ip4_class_prefix_len(&c->ip4.addr); > > > if (c->mode =3D=3D MODE_PASTA) > > > c->ip4.no_copy_addrs =3D true; > > > - break; > > > + } else if (af =3D=3D AF_INET6) { > > > + c->ip6.addr =3D addr.a6; > > > + if (c->mode =3D=3D MODE_PASTA) > > > + c->ip6.no_copy_addrs =3D true; > > > + } else { > > > + die("Invalid prefix length: %d", prefix_len); > >=20 > > This error message does not seem to match the conditions that trigger i= t. >=20 > 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 inval= id. 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. >=20 > >=20 > > > } > > > - > > > - 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 =3D conf_ip4_prefix(optarg); > > > if (c->ip4.prefix_len < 0) > > > die("Invalid netmask: %s", optarg); > > > - > > > + prefix_from_opt =3D 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 > > > #include > > > #include > > > +#include > > > #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 > >=20 > > "adjustment" doesn't make sense to me here. > >=20 > > > + * @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 > >=20 > > 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. >=20 > 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) > >=20 > > If we are returning an address family, the return type should be sa_fam= ily_t. > Ok. > >=20 > > > +{ > > > + bool mapped =3D false; > > > + struct in6_addr a6; > > > + struct in_addr a4; > > > + char *slash; > > > + char *end; > > > + int af; > > > + > > > + *prefix_len =3D 0; > >=20 > > As noted below, 0 is not a suitable value for "missing prefix", > > because 0 length prefixes are real and important. >=20 > 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. >=20 > I can try that. >=20 > >=20 > > > + > > > + /* Check for presence of /prefix_len suffix */ > > > + slash =3D strchr(src, '/'); > > > + if (slash) > > > + *slash =3D '\0'; > > > + > > > + /* Read address */ > > > + if (inet_pton(AF_INET, src, &a4)) { > > > + inany_from_af(dst, AF_INET, &a4); > > > + af =3D AF_INET; > > > + } else if (inet_pton(AF_INET6, src, &a6)) { > > > + inany_from_af(dst, AF_INET6, &a6); > > > + af =3D AF_INET6; > > > + if (inany_v4(dst)) > > > + mapped =3D true; > > > + } else { > > > + memset(dst, 0, sizeof(*dst)); > > > + return -1; > > > + } > > > + > > > + if (!slash) > > > + return mapped ? AF_INET : af; > >=20 > > You can avoid messing around with the mapped temporary by > > unconditionally deriving the address family from inany_v4(). >=20 > Ok. >=20 > >=20 > > > + > > > + /* Read prefix_len - /0 is not allowed */ > >=20 > > /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). >=20 > ok. That changes a few things. >=20 > >=20 > > > + errno =3D 0; > > > + *prefix_len =3D strtoul(slash + 1, &end, 10); > > > + if (errno || *end || *prefix_len =3D=3D 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 =3D=3D AF_INET) { > > > + /* Native IPv4: convert to IPv6 format */ > > > + if (*prefix_len > 32) > > > + return -1; > > > + *prefix_len +=3D 96; > >=20 > > If you make this adjustment before the previous stanza you again don't > > need the 'mapped' local and can use inany_v4(). >=20 > Good point. >=20 > >=20 > > > + return AF_INET; > > > + } > > > + > > > + /* Native IPv6: keep as-is */ > > > + if (*prefix_len > 128) > > > + return -1; > >=20 > > And if you move this before the if (mapped) stanza, you can remove the > > duplicated check for > 128. >=20 > I'll give it a try. >=20 > ///jon >=20 > >=20 > > > + 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 siph= ash_state *state, > > > const char *inany_ntop(const union inany_addr *src, char *dst, sock= len_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 > > > +#include > > > + > > > #include "util.h" > > > #include "ip.h" > > > @@ -67,3 +69,22 @@ found: > > > *proto =3D nh; > > > return true; > > > } > > > + > > > +/** > > > + * ip4_class_prefix_len() - Get class based prefix length for IPv4 a= ddress > > > + * @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 =3D 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 =3D= { 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 D= HCPv6 (option > > > 5) and an \fIaddr\fR-based prefix via NDP Router Advertisement (opt= ion 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 alternativ= e 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 hos= t 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 u= sing CIDR > > > +notation with the \fB-a\fR, \fB--address\fR option (e.g. \fB-a\fR 19= 2.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 le= ngth, the > > > +netmask is determined based on the corresponding network class. In a= ll other > > > +cases, the netmask is determined by using the indicated prefix lengt= h. > > > .TP > > > .BR \-M ", " \-\-mac-addr " " \fIaddr > > > --=20 > > > 2.52.0 > > >=20 > >=20 >=20 --=20 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 --dJ94ysSQcZV1g74u Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmmFX0wACgkQzQJF27ox 2GcYtRAApjKi2ThEvg8GlVWSmcRkp7kN2GPQjoshK3epYoKm/WiOZfTMxsoW5fYH 5o2X1nF7obsZQq0QN5jduGhLtClJQwR7sVejG2ruCY/MmD91vYhCDVg0iFPy08A2 IlWrou/NiTHZaC4nF/GROv+QhD03FCT/93dS16qjoatItb/vAxLyQbEffL/f8BQF nwpgMBZZ+SHQDStMMHYLlwkOHA4gu5Z5MOrMk1FdCqMsHxw6U8xqdH6jQz/kRudA vRVUGRn5l3VTtqS6pkMEIeqj+CVQWrQBDm4keCQkd1WXlIAkXbI1jwk+rP2qoNfD y8o54Dhb9Xx0ScUeBIardRg/Wu7ykJ7bac+RGaXiQqMgWmvB2UQly2KZtXGpK23q B9zlFTrZGS91NPAmt0CUufBFjDsfDeHeVdqEmQZFxORMdeBEOOQI1Tk9fLA6k1Gs 7HIzuxRXLqB+L7ms1GNa9LthNw90jh5Nx2+AJqzCt38Y2/cBneVq+iC+zekmjTeP zlzc+bVHApNVFC2AJVJe4AP+w2Eu0f5EzNZbvkVwbnCQaOUrnxM3J4zeKLSOnWdy f4ghr175XhHKh4waj3a8TJ+skv+9DSl66HcjZNkiY9jWVQnEcdysqK1ev8VvyEyA zMZwbmdzO4hCO0+vI677Yv252eaADG0DSIrQlrYPdPwjjMw/bAU= =7c2+ -----END PGP SIGNATURE----- --dJ94ysSQcZV1g74u--