From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top, Jon Maloy <jmaloy@redhat.com>
Subject: Re: [PATCH 10/12] conf: Use new parsing tools to handle -a option
Date: Wed, 1 Jul 2026 11:55:12 +1000 [thread overview]
Message-ID: <akRzgKnY8ae1AuOE@zatzit> (raw)
In-Reply-To: <20260701020750.765493ee@elisabeth>
[-- Attachment #1: Type: text/plain, Size: 10475 bytes --]
On Wed, Jul 01, 2026 at 02:07:50AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:10:01 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > The -a command line option can take either an address prefix, or a bare
> > address. Current parsing of this is pretty awkward, using the special
> > purpose helper inany_prefix_pton(). With the new incremental parsing
> > helpers this can be done more naturally. Rework it to use them.
> >
> > This does requiring extending parse_inany() to parse_inany_() which also
> > reports the format of the address as parse, as opposed to the family of
> > the resulting address. This is so that ::ffff:192.0.1.1/112 will be
> > correctly interpreted the same as 192.0.1.1/16, rather than the
> > nonsensical 192.0.0.1/112.
>
> By the way, as far as I know, ::ffff:192.0.1.1/112 is not a valid
> address, because IPv4-mapped addresses must always have /96 as prefix
> length (see RFC 6890, Table 20).
AFAICT the /96 there is just indicating the size of the v4-mapped
block, not saying you can't have longer prefixes within that block.
> RFC 6052 adds some madness (e.g. 2001:db8:122:344::192.0.2.33 from
> Table 1) on top, but as far as I understand you can't use that for
> prefixes.
>
> >
> > Cc: Jon Maloy <jmaloy@redhat.com>
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > Makefile | 1 -
> > conf.c | 63 +++++++++++++++++++++++++++++++++++---------------------
> > inany.c | 50 --------------------------------------------
> > inany.h | 2 --
> > parse.c | 17 ++++++++++++---
> > parse.h | 5 ++++-
> > 6 files changed, 58 insertions(+), 80 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index e2b22ddf..5757aeff 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -223,7 +223,6 @@ passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repa
> > pesto.cppcheck: BASE_CPPFLAGS += -DPESTO
> > pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:bitmap.c
> > pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.h
> > -pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.c
> > pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:ip.h
> > pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
> > pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c
> > diff --git a/conf.c b/conf.c
> > index 3614776c..ff7ca5c7 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1176,43 +1176,60 @@ int conf_tap_fd(const char *arg)
> > */
> > static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n)
> > {
> > + unsigned long prefix_len;
> > + const struct in_addr *a4;
> > union inany_addr addr;
> > + sa_family_t parseaf;
>
> I think parse_af would be more readable (and also more consistent with
> e.g. prefix_len).
Sure.
> > const char *p = arg;
> > - uint8_t prefix_len;
> > bool is_prefix;
> >
> > - is_prefix = inany_prefix_pton(arg, &addr, &prefix_len);
> > -
> > - if (is_prefix && opt_n)
> > - die("Redundant prefix length specification");
> > -
> > - if (!is_prefix &&
> > - !(parse_inany(&p, &addr) && parse_eoi(p)))
> > - die("Invalid address: %s", arg);
> > -
> > - if (opt_n && inany_v4(&addr))
> > - prefix_len = opt_n;
> > - else if (!is_prefix)
> > - prefix_len = inany_default_prefix_len(&addr);
> > + if (!parse_inany_(&p, &addr, &parseaf))
>
> ...just call it af? It doesn't matter so much in the following code
> where that comes from. Starting from here, we know it's the address
> family we're using.
Eh.. I don't like that, because just "af" suggests it's the address
family of the address itself. That's not the case - it's very
specifically the address family of the *string* not the resulting
address.
> > + goto bad;
> > + a4 = inany_v4(&addr);
> > +
> > + if ((is_prefix = parse_literal(&p, "/"))) {
>
> The current return convention makes more sense here, but I wouldn't
> find:
>
> is_prefix = !parse_literal(&p, "/")
>
> outrageous, either.
>
> > + /* Prefix length included in -a option */
> > + if (!parse_unsigned(&p, 10, &prefix_len))
> > + goto bad;
> > + if (opt_n)
> > + die("Redundant prefix length specification");
> > + if (parseaf == AF_INET) {
> > + if (prefix_len > 32)
> > + goto bad_prefix;
> > + prefix_len += 96;
> > + } else if (prefix_len > 128) {
> > + goto bad_prefix;
> > + }
> > + } else {
> > + /* Get prefix length from elsewhere */
> > + if (opt_n && a4)
> > + prefix_len = opt_n;
> > + else
> > + prefix_len = inany_default_prefix_len(&addr);
> > + }
> >
> > - if (inany_is_unspecified(&addr) || inany_is_multicast(&addr) ||
> > - inany_is_loopback(&addr) || IN6_IS_ADDR_V4COMPAT(&addr.a6))
> > - die("Invalid address: %s", arg);
> > + if (!parse_eoi(p) ||
>
> Note: with *p instead of !parse_eoi(), this would fit on one line.
>
> > + !inany_is_unicast(&addr) ||
> > + inany_is_loopback(&addr))
> > + goto bad;
> >
> > - if (inany_v4(&addr)) {
> > - c->ip4.addr = *inany_v4(&addr);
> > + if (a4) {
> > + c->ip4.addr = *a4;
> > c->ip4.prefix_len = prefix_len - 96;
> > c->ip4.addr_fixed = true;
> > - if (c->mode == MODE_PASTA)
>
> Why does this (and the same condition just below) go away here? I mean,
> it's not fundamental in any case, but changing it here makes it look
> like it has something to do with this change (and I guess it's not the
> case).
Good point, that's an unrelated change. I split it into its own patch.
>
> > - c->ip4.no_copy_addrs = true;
> > + c->ip4.no_copy_addrs = true;
> > } else {
> > c->ip6.addr = addr.a6;
> > c->ip6.addr_fixed = true;
> > - if (c->mode == MODE_PASTA)
> > - c->ip6.no_copy_addrs = true;
> > + c->ip6.no_copy_addrs = true;
> > }
> >
> > return is_prefix;
> > +
> > +bad_prefix:
> > + die("Invalid prefix length: %s", arg);
> > +bad:
> > + die("Invalid guest address: %s", arg);
> > }
> >
> > /**
> > diff --git a/inany.c b/inany.c
> > index 154f08b5..120c9387 100644
> > --- a/inany.c
> > +++ b/inany.c
> > @@ -70,53 +70,3 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
> >
> > return inet_ntop(AF_INET6, &src->a6, dst, size);
> > }
> > -
> > -/**
> > - * inany_prefix_pton() - Parse an IPv[46] address with prefix length
> > - * @src: IPv[46] address and prefix length string in CIDR format
> > - * @dst: Output buffer, filled with parsed address
> > - * @prefix_len: Prefix length, to be filled in IPv6 format
> > - *
> > - * Return: 1 on success, 0 if no parseable address or prefix is found
> > - */
> > -int inany_prefix_pton(const char *src, union inany_addr *dst,
> > - uint8_t *prefix_len)
> > -{
> > - char astr[INANY_ADDRSTRLEN] = { 0 };
> > - size_t alen = strcspn(src, "/");
> > - const char *pstr = &src[alen + 1];
> > - const char *p = astr;
> > - unsigned long plen;
> > - char *end;
> > -
> > - if (alen >= INANY_ADDRSTRLEN)
> > - return 0;
> > -
> > - if (src[alen] != '/')
> > - return 0;
> > -
> > - strncpy(astr, src, alen);
> > -
> > - /* Read prefix length */
> > - errno = 0;
> > - plen = strtoul(pstr, &end, 10);
> > - if (errno || *end || plen > 128)
> > - return 0;
> > -
> > - /* Read address */
> > - if (inet_pton(AF_INET6, astr, dst)) {
> > - if (inany_v4(dst) && plen < 96)
> > - return 0;
> > - *prefix_len = plen;
> > - return 1;
> > - }
> > -
> > - if (parse_inany(&p, dst) && parse_eoi(p)) {
> > - if (plen > 32)
> > - return 0;
> > - *prefix_len = plen + 96;
> > - return 1;
> > - }
> > -
> > - return 0;
> > -}
> > diff --git a/inany.h b/inany.h
> > index 93d98368..5b176ccf 100644
> > --- a/inany.h
> > +++ b/inany.h
> > @@ -303,7 +303,5 @@ static inline int inany_from_sockaddr(union inany_addr *dst, in_port_t *port,
> >
> > bool inany_matches(const union inany_addr *a, const union inany_addr *b);
> > const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size);
> > -int inany_prefix_pton(const char *src, union inany_addr *dst,
> > - uint8_t *prefix_len);
> >
> > #endif /* INANY_H */
> > diff --git a/parse.c b/parse.c
> > index 0349c5dc..3e0dbd45 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -184,18 +184,29 @@ static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> > }
> >
> > /**
> > - * parse_inany() - Parse an IPv4 or IPv6 address from a string
> > + * parse_inany_() - Parse an IPv4 or IPv6 address from a string
> > * @addr: On success, updated with parsed address
> > + * @parseaf: On success, updated with the format of the parsed address
> > + *
> > + * @parseaf is updated to reflect the string format, not the final address
> > + * family. So "::ffff:192.0.1.1", will set @parseaf to AF_INET6, despite being
> > + * a IPv4-mapped address.
> > */
> > -bool parse_inany(const char **cursor, union inany_addr *addr)
> > +bool parse_inany_(const char **cursor, union inany_addr *addr,
> > + sa_family_t *parseaf)
> > {
> > struct in_addr a4;
> >
> > - if (parse_ipv6(cursor, &addr->a6))
> > + if (parse_ipv6(cursor, &addr->a6)) {
> > + if (parseaf)
> > + *parseaf = AF_INET6;
> > return true;
> > + }
> >
> > if (parse_ipv4(cursor, &a4)) {
> > *addr = inany_from_v4(a4);
> > + if (parseaf)
> > + *parseaf = AF_INET;
> > return true;
> > }
> >
> > diff --git a/parse.h b/parse.h
> > index 2820a065..08b038cf 100644
> > --- a/parse.h
> > +++ b/parse.h
> > @@ -27,6 +27,9 @@ bool parse_eoi(const char *cursor);
> > bool parse_unsigned(const char **cursor, int base, unsigned long *valp);
> > bool parse_port_range(const char **cursor, struct port_range *range);
> > bool parse_ipv4(const char **cursor, struct in_addr *abuf);
> > -bool parse_inany(const char **cursor, union inany_addr *addr);
> > +bool parse_inany_(const char **cursor, union inany_addr *addr,
> > + sa_family_t *parseaf);
> > +
> > +#define parse_inany(cursor, addr) parse_inany_((cursor), (addr), NULL)
> >
> > #endif /* _PARSE_H */
>
> --
> Stefano
>
--
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 --]
next prev parent reply other threads:[~2026-07-01 3:04 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-26 7:09 [PATCH 00/12] Rework option parsing in preparation for destination remapping David Gibson
2026-06-26 7:09 ` [PATCH 01/12] Makefile: Add missing PESTO_HEADERS variable David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 1:23 ` David Gibson
2026-06-26 7:09 ` [PATCH 02/12] conf: Use parameter instead of global in conf_nat() David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 1:24 ` David Gibson
2026-06-26 7:09 ` [PATCH 03/12] parse: Start splitting out parsing helpers David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 1:36 ` David Gibson
2026-06-26 7:09 ` [PATCH 04/12] conf: Remove duplicate parsing of -F option David Gibson
2026-06-26 7:09 ` [PATCH 05/12] conf: Clean up conf_ip4_prefix() David Gibson
2026-06-26 7:09 ` [PATCH 06/12] parse: Add helper to parse unsigned integer values David Gibson
2026-06-26 7:09 ` [PATCH 07/12] parse: Move parse_port_range() to new parsing framework David Gibson
2026-06-26 7:09 ` [PATCH 08/12] parse: Add helpers for parsing IP addresses David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 0:09 ` [RESEND] " Stefano Brivio
2026-07-01 0:11 ` [RESEND #2] " Stefano Brivio
2026-07-01 1:44 ` David Gibson
2026-07-01 1:43 ` David Gibson
2026-06-26 7:10 ` [PATCH 09/12] conf: Move address configuration into helper function David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 1:44 ` David Gibson
2026-06-26 7:10 ` [PATCH 10/12] conf: Use new parsing tools to handle -a option David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 1:55 ` David Gibson [this message]
2026-06-26 7:10 ` [PATCH 11/12] fwd_rule: Allow "all" port specs to be combined with other options David Gibson
2026-07-01 0:07 ` Stefano Brivio
2026-07-01 1:55 ` David Gibson
2026-06-26 7:10 ` [PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers David Gibson
2026-07-01 0:08 ` Stefano Brivio
2026-07-01 3:03 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=akRzgKnY8ae1AuOE@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=jmaloy@redhat.com \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).