[PATCH 00/12] Rework option parsing in preparation for destination remapping

[PATCH 00/12] Rework option parsing in preparation for destination remapping

[David Gibson]

This was... a bit of a nightmare.  It took way too long and went down
a bunch of blind alleys.  I'm not sure how much I like the final
result: it's pleasingly terse in some cases, but in others it feels
dangerously subtle, requiring a pretty careful understanding of the
sequencing rules of C's &&, || and , operators.

That said, while I was at many points ready to pack it in and hack my
around the problems limiting the parsing for destination remapping, I
couldn't really think of feasible way to do that either.  So, here we
are.

David Gibson (12):
  Makefile: Add missing PESTO_HEADERS variable
  conf: Use parameter instead of global in conf_nat()
  parse: Start splitting out parsing helpers
  conf: Remove duplicate parsing of -F option
  conf: Clean up conf_ip4_prefix()
  parse: Add helper to parse unsigned integer values
  parse: Move parse_port_range() to new parsing framework
  parse: Add helpers for parsing IP addresses
  conf: Move address configuration into helper function
  conf: Use new parsing tools to handle -a option
  fwd_rule: Allow "all" port specs to be combined with other options
  fwd_rule: Rewrite forward rule parsing using parse.c helpers

 Makefile   |  22 +--
 common.h   |   3 +
 conf.c     | 209 ++++++++++++++++------------
 conf.h     |   1 +
 fwd_rule.c | 392 +++++++++++++++++++++++------------------------------
 fwd_rule.h |   2 -
 inany.c    |  70 +---------
 inany.h    |   3 -
 parse.c    | 243 +++++++++++++++++++++++++++++++++
 parse.h    |  37 +++++
 util.c     |  12 +-
 11 files changed, 591 insertions(+), 403 deletions(-)
 create mode 100644 parse.c
 create mode 100644 parse.h

-- 
2.54.0

[PATCH 01/12] Makefile: Add missing PESTO_HEADERS variable

[David Gibson]

In several places we use a PESTO_HEADERS variable, with all the headers
that we need to build the pesto binary.  However, we never define it.
This looks like an error introduced by a bad rebase of the series
introducing pesto before it was merged.

It turns out the fact we didn't list the headers was the only reason we
weren't getting unusedStructMember cppcheck warnings for pesto as we
already do for passt and passt-repair.  So, reinstate that suppression for
pesto as well.

Fixes: 02236db32625 ("pesto: Introduce stub configuration tool")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index e8170e90..5ed0f702 100644
--- a/Makefile
+++ b/Makefile
@@ -55,6 +55,7 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
 	virtio.h vu_common.h
 QRAP_HEADERS = arp.h ip.h passt.h util.h
 PASST_REPAIR_HEADERS = linux_dep.h
+PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h
 
 C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
 ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
@@ -203,7 +204,8 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	else								\
 		echo "";						\
 	fi)								\
-	--suppress=missingIncludeSystem
+	--suppress=missingIncludeSystem					\
+	--suppress=unusedStructMember
 
 cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
 
@@ -212,10 +214,8 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
 	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
 
 passt.cppcheck: BASE_CPPFLAGS += -UPESTO
-passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember
 passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 
-passt-repair.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember
 passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
 
 pesto.cppcheck: BASE_CPPFLAGS += -DPESTO
-- 
2.54.0

[PATCH 02/12] conf: Use parameter instead of global in conf_nat()

[David Gibson]

Conf nat takes a parameter @arg for the argument it's parsing.  However on
error we print instead optarg, the getopt() global.  This happens to be the
same thing at the time of the call, but it's not the right way to get to
it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c
index 4755a9f4..6ab8efec 100644
--- a/conf.c
+++ b/conf.c
@@ -1049,7 +1049,7 @@ static void conf_nat(const char *arg, struct in_addr *addr4,
 	    !IN4_IS_ADDR_MULTICAST(addr4))
 		return;
 
-	die("Invalid address to remap to host: %s", optarg);
+	die("Invalid address to remap to host: %s", arg);
 }
 
 /**
-- 
2.54.0

[PATCH 03/12] parse: Start splitting out parsing helpers

[David Gibson]

As we add more complexity to what forwarding rules are allowed, our
existing ad-hoc C parsing is starting to become quite awkward.  We already
have some parts that resemble a very simple recursive[0] descent parser,
with composable subfunctions that consume as much input as they need,
rather than pre-splitting based on known delimiters.

Start extending this approach, by creating parse.[ch] with parsing helpers
with a uniform interface.  Initially we start with very simple cases: the
parse_keyword() function from fwd_rule.c and another helper to check that
we've reached the end of input.

Rename parse_keyword() to parse_literal(), because it's not just useful
for "keywords" as such.  We can use it in a bunch of additional places for
parsing delimiters and other symbols.  This doesn't gain us a lot now but
will make things clearer as we use more such parser helpers.

[0] Except that the grammars we have aren't actually recursive, so neither
    is the code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile   | 17 ++++++++------
 conf.c     |  5 +++-
 fwd_rule.c | 33 +++++----------------------
 parse.c    | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse.h    | 14 ++++++++++++
 5 files changed, 101 insertions(+), 35 deletions(-)
 create mode 100644 parse.c
 create mode 100644 parse.h

diff --git a/Makefile b/Makefile
index 5ed0f702..bd729c75 100644
--- a/Makefile
+++ b/Makefile
@@ -36,12 +36,13 @@ BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security
 PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
 	epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \
 	isolation.c lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c \
-	passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \
+	parse.c passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \
 	tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \
 	vhost_user.c virtio.c vu_common.c
 QRAP_SRCS = qrap.c
 PASST_REPAIR_SRCS = passt-repair.c
-PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c serialise.c
+PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c parse.c \
+	serialise.c
 SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
 
 MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
@@ -49,13 +50,14 @@ MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
 PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
 	epoll_ctl.h flow.h fwd.h fwd_rule.h flow_table.h icmp.h icmp_flow.h \
 	inany.h iov.h ip.h isolation.h lineread.h log.h migrate.h ndp.h \
-	netlink.h packet.h passt.h pasta.h pcap.h pif.h repair.h serialise.h \
-	siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \
-	tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \
-	virtio.h vu_common.h
+	netlink.h packet.h parse.h passt.h pasta.h pcap.h pif.h repair.h \
+	serialise.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \
+	tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \
+	vhost_user.h virtio.h vu_common.h
 QRAP_HEADERS = arp.h ip.h passt.h util.h
 PASST_REPAIR_HEADERS = linux_dep.h
-PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h
+PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h parse.h \
+	pesto.h serialise.h
 
 C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
 ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
@@ -223,6 +225,7 @@ 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:parse.c
 pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
 pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c
 pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
diff --git a/conf.c b/conf.c
index 6ab8efec..ca6c859c 100644
--- a/conf.c
+++ b/conf.c
@@ -52,6 +52,7 @@
 #include "conf.h"
 #include "pesto.h"
 #include "serialise.h"
+#include "parse.h"
 
 #define NETNS_RUN_DIR	"/run/netns"
 
@@ -1028,7 +1029,9 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 static void conf_nat(const char *arg, struct in_addr *addr4,
 		     struct in6_addr *addr6, int *no_map_gw)
 {
-	if (strcmp(arg, "none") == 0) {
+	const char *p = arg;
+
+	if (parse_literal(&p, "none") && parse_eoi(p)) {
 		*addr4 = in4addr_any;
 		*addr6 = in6addr_any;
 		if (no_map_gw)
diff --git a/fwd_rule.c b/fwd_rule.c
index 04a0101d..d0e90402 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -24,6 +24,7 @@
 #include "fwd_rule.h"
 #include "lineread.h"
 #include "log.h"
+#include "parse.h"
 #include "serialise.h"
 
 /* Ephemeral port range: values from RFC 6335 */
@@ -427,28 +428,6 @@ static int parse_port_range(const char *s, const char **endptr,
 	return 0;
 }
 
-/**
- * parse_keyword() - Parse a literal keyword
- * @s:		String to parse
- * @endptr:	Update to the character after the keyword
- * @kw:		Keyword to accept
- *
- * Return: 0, if @s starts with @kw, -EINVAL if it does not
- */
-static int parse_keyword(const char *s, const char **endptr, const char *kw)
-{
-	size_t len = strlen(kw);
-
-	if (strlen(s) < len)
-		return -EINVAL;
-
-	if (memcmp(s, kw, len))
-		return -EINVAL;
-
-	*endptr = s + len;
-	return 0;
-}
-
 /**
  * fwd_rule_range_except() - Set up forwarding for a range of ports minus a
  *                           bitmap of exclusions
@@ -568,7 +547,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 			continue;
 		}
 
-		if (parse_keyword(p, &p, "auto") == 0) {
+		if (parse_literal(&p, "auto")) {
 			if (p != ep) /* Garbage after the keyword */
 				goto bad;
 
@@ -582,9 +561,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 		}
 
 		/* Should be an exclude range */
-		if (*p != '~')
+		if (!parse_literal(&p, "~"))
 			goto bad;
-		p++;
 
 		if (parse_port_range(p, &p, &xrange))
 			goto bad;
@@ -616,8 +594,9 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 		if (parse_port_range(p, &p, &orig_range))
 			goto bad;
 
-		if (*p == ':') { /* There's a range to map to as well */
-			if (parse_port_range(p + 1, &p, &mapped_range))
+		if (parse_literal(&p, ":")) {
+			/* There's a range to map to as well */
+			if (parse_port_range(p, &p, &mapped_range))
 				goto bad;
 			if ((mapped_range.last - mapped_range.first) !=
 			    (orig_range.last - orig_range.first))
diff --git a/parse.c b/parse.c
new file mode 100644
index 00000000..c2a92422
--- /dev/null
+++ b/parse.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* PASST - Plug A Simple Socket Transport
+ *  for qemu/UNIX domain socket mode
+ *
+ * PASTA - Pack A Subtle Tap Abstraction
+ *  for network namespace/tap device mode
+ *
+ * parse.c - Composable parsing helpers
+ *
+ * Copyright Red Hat
+ * Author: David Gibson <david@gibson.dropbear.id.au>
+ */
+
+#include <assert.h>
+#include <stddef.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stdbool.h>
+#include <unistd.h>
+
+#include "common.h"
+
+/**
+ * DOC: Theory of Operation
+ *
+ * These are a number of primitives which can be combined into parsers for
+ * moderately complex input.  For simpler composability, they have common
+ * conventions.
+ *
+ * - Functions return a bool indicating whether they successfully parsed.
+ * - Any specific output from the parse is as output parameters
+ * - First argument is always @cursor, a const char **.
+ * - On entry, *@cursor has the point to start parsing.
+ * - On successful exit, *@cursor is updated to the next character after the
+     parsed portion of the input
+ * - On failure, *@cursor and any output arguments are not modified
+ *
+ * For brevity the common parameters and return information are omitted from the
+ * individual function documentation comments.
+ */
+
+/**
+ * parse_literal() - Parse a specified literal string
+ * @lit:	Keyword to accept
+ */
+bool parse_literal(const char **cursor, const char *lit)
+{
+	size_t len = strlen(lit);
+
+	if (strlen(*cursor) < len || memcmp(*cursor, lit, len))
+		return false;
+
+	*cursor += len;
+	return true;
+}
+
+/**
+ * parse_eoi() - Parse end of input
+ * @cursor:	Current parse pointer
+ *
+ * Return: true if @p is at End of Input (\0), false otherwise
+ */
+bool parse_eoi(const char *cursor)
+{
+	return !(*cursor);
+}
diff --git a/parse.h b/parse.h
new file mode 100644
index 00000000..eb51a469
--- /dev/null
+++ b/parse.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright Red Hat
+ * Author: David Gibson <david@gibson.dropbear.id.au>
+ */
+
+#ifndef PARSE_H
+#define PARSE_H
+
+#include <stdbool.h>
+
+bool parse_literal(const char **cursor, const char *lit);
+bool parse_eoi(const char *cursor);
+
+#endif /* _PARSE_H */
-- 
2.54.0

[PATCH 04/12] conf: Remove duplicate parsing of -F option

[David Gibson]

The -F option is parsed in conf() along with everything else.  However,
because it informs what fds we can close at startup, we also have a special
case parse of it in close_open_files().

At present we duplicate the parsing and validation code, which is a bit
risky.  Avoid that with a conf_tap_fd() helper.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 29 +++++++++++++++++++----------
 conf.h |  1 +
 util.c | 12 +++---------
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/conf.c b/conf.c
index ca6c859c..32163f56 100644
--- a/conf.c
+++ b/conf.c
@@ -1153,6 +1153,24 @@ static void conf_sock_listen(const struct ctx *c)
 		die_perror("Couldn't add configuration socket to epoll");
 }
 
+/**
+ * conf_tap_fd() - Read tap fd as supplied by -F command line option
+ * @arg:	Argument to -F command line option
+ */
+int conf_tap_fd(const char *arg)
+{
+	long val;
+
+	errno = 0;
+	val = strtol(arg, NULL, 0);
+
+	if (errno || (val != STDIN_FILENO && val <= STDERR_FILENO) ||
+	    val > INT_MAX)
+		die("Invalid --fd: %s", arg);
+
+	return val;
+}
+
 /**
  * conf() - Process command-line arguments and set configuration
  * @c:		Execution context
@@ -1253,7 +1271,6 @@ void conf(struct ctx *c, int argc, char **argv)
 	const char *logfile = NULL;
 	char *runas = NULL;
 	size_t logsize = 0;
-	long fd_tap_opt;
 	int name, ret;
 	uid_t uid;
 	gid_t gid;
@@ -1506,15 +1523,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->fd_control_listen = c->fd_control = -1;
 			break;
 		case 'F':
-			errno = 0;
-			fd_tap_opt = strtol(optarg, NULL, 0);
-
-			if (errno ||
-			    (fd_tap_opt != STDIN_FILENO && fd_tap_opt <= STDERR_FILENO) ||
-			    fd_tap_opt > INT_MAX)
-				die("Invalid --fd: %s", optarg);
-
-			c->fd_tap = fd_tap_opt;
+			c->fd_tap = conf_tap_fd(optarg);
 			c->one_off = true;
 			*c->sock_path = 0;
 			break;
diff --git a/conf.h b/conf.h
index 16f97189..1fa1280e 100644
--- a/conf.h
+++ b/conf.h
@@ -7,6 +7,7 @@
 #define CONF_H
 
 enum passt_modes conf_mode(int argc, char *argv[]);
+int conf_tap_fd(const char *arg);
 void conf(struct ctx *c, int argc, char **argv);
 void conf_listen_handler(struct ctx *c, uint32_t events);
 void conf_handler(struct ctx *c, uint32_t events);
diff --git a/util.c b/util.c
index 0aa4d42d..4bc5d6f8 100644
--- a/util.c
+++ b/util.c
@@ -38,6 +38,7 @@
 #include "epoll_ctl.h"
 #include "pasta.h"
 #include "serialise.h"
+#include "conf.h"
 #ifdef HAS_GETRANDOM
 #include <sys/random.h>
 #endif
@@ -939,15 +940,8 @@ void close_open_files(int argc, char **argv)
 	do {
 		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
 
-		if (name == 'F') {
-			errno = 0;
-			fd = strtol(optarg, NULL, 0);
-
-			if (errno ||
-			    (fd != STDIN_FILENO && fd <= STDERR_FILENO) ||
-			    fd > INT_MAX)
-				die("Invalid --fd: %s", optarg);
-		}
+		if (name == 'F')
+			fd = conf_tap_fd(optarg);
 	} while (name != -1);
 
 	if (fd == -1) {
-- 
2.54.0

[PATCH 05/12] conf: Clean up conf_ip4_prefix()

[David Gibson]

Restructure conf_ip4_prefix() so that success is the early exit path and
failure is the "default" path.  Also move the error handling (die()) into
it from the caller.

This saves a handful of lines for now, and will make integration with
upcoming parsing changes nicer.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/conf.c b/conf.c
index 32163f56..ea299e18 100644
--- a/conf.c
+++ b/conf.c
@@ -350,9 +350,9 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
 /** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask
  * @arg:	Netmask in dotted decimal or prefix length
  *
- * Return: validated prefix length on success, -1 on failure
+ * Return: validated prefix length; dies on bad argument
  */
-static int conf_ip4_prefix(const char *arg)
+static uint8_t conf_ip4_prefix(const char *arg)
 {
 	struct in_addr mask;
 	unsigned long len;
@@ -360,16 +360,16 @@ static int conf_ip4_prefix(const char *arg)
 	if (inet_pton(AF_INET, arg, &mask)) {
 		in_addr_t hmask = ntohl(mask.s_addr);
 		len = __builtin_popcount(hmask);
-		if ((hmask << len) != 0)
-			return -1;
+		if ((hmask << len) == 0)
+			return len;
 	} else {
 		errno = 0;
 		len = strtoul(arg, NULL, 0);
-		if (len > 32 || errno)
-			return -1;
+		if (!errno && len <= 32)
+			return len;
 	}
 
-	return len;
+	die("Invalid prefix length: %s", arg);
 }
 
 /**
@@ -1606,20 +1606,13 @@ void conf(struct ctx *c, int argc, char **argv)
 			}
 			break;
 		}
-		case 'n': {
-			int plen;
-
+		case 'n':
 			if (addr_has_prefix_len)
 				die("Redundant prefix length specification");
 
-			plen = conf_ip4_prefix(optarg);
-			if (plen < 0)
-				die("Invalid prefix length: %s", optarg);
-
-			prefix_len_from_opt = plen + 96;
-			c->ip4.prefix_len = plen;
+			c->ip4.prefix_len = conf_ip4_prefix(optarg);
+			prefix_len_from_opt = c->ip4.prefix_len + 96;
 			break;
-		}
 		case 'M':
 			parse_mac(c->our_tap_mac, optarg);
 			break;
-- 
2.54.0

[PATCH 06/12] parse: Add helper to parse unsigned integer values

[David Gibson]

Most places we need to parse an integer encoded as a string, we use
strtol() or strtoul().  These already work a bit like descent parser
helpers, in that they consume as much as they can, but don't require the
number parsed to be the whole of the einput string.  Make a wrapper to
parse unsigned integers as part of our parsing helper.  This wrapper
handles the mildly fiddly error checking requirements for strtoul().

We replace a number, though not all, of our existing strtoul() uses with
the new parse_unsigned().  We also replace a number of strtol() use cases,
because, despite using that instead of strtoul() they are only used for
non-negative values.  In some cases this makes the logic a little more
straightforward.  In some other cases it will catch some error cases we
previously could have missed.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile   |  1 -
 conf.c     | 51 +++++++++++++++++++++++++--------------------------
 fwd_rule.c | 42 ++++++++++++++----------------------------
 parse.c    | 27 ++++++++++++++++++++++-----
 parse.h    |  1 +
 5 files changed, 62 insertions(+), 60 deletions(-)

diff --git a/Makefile b/Makefile
index bd729c75..85d279c0 100644
--- a/Makefile
+++ b/Makefile
@@ -225,7 +225,6 @@ 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:parse.c
 pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
 pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c
 pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
diff --git a/conf.c b/conf.c
index ea299e18..bd357117 100644
--- a/conf.c
+++ b/conf.c
@@ -321,17 +321,16 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
 		die("Both --netns and PID or command given");
 
 	if (optind + 1 == argc) {
-		char *endptr;
-		long pidval;
+		const char *p = argv[optind];
+		unsigned long pidval;
 
-		pidval = strtol(argv[optind], &endptr, 10);
-		if (!*endptr) {
+		if (parse_unsigned(&p, 10, &pidval) && parse_eoi(p)) {
 			/* Looks like a pid */
-			if (pidval < 0 || pidval > INT_MAX)
+			if (pidval > INT_MAX)
 				die("Invalid PID %s", argv[optind]);
 
 			if (snprintf_check(netns, PATH_MAX,
-					   "/proc/%ld/ns/net", pidval))
+					   "/proc/%lu/ns/net", pidval))
 				die_perror("Can't build netns path");
 
 			if (!*userns) {
@@ -354,6 +353,7 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
  */
 static uint8_t conf_ip4_prefix(const char *arg)
 {
+	const char *p = arg;
 	struct in_addr mask;
 	unsigned long len;
 
@@ -362,11 +362,9 @@ static uint8_t conf_ip4_prefix(const char *arg)
 		len = __builtin_popcount(hmask);
 		if ((hmask << len) == 0)
 			return len;
-	} else {
-		errno = 0;
-		len = strtoul(arg, NULL, 0);
-		if (!errno && len <= 32)
-			return len;
+	} else if (parse_unsigned(&p, 0, &len) && parse_eoi(p) &&
+		   len <= 32) {
+		return len;
 	}
 
 	die("Invalid prefix length: %s", arg);
@@ -1159,13 +1157,12 @@ static void conf_sock_listen(const struct ctx *c)
  */
 int conf_tap_fd(const char *arg)
 {
-	long val;
-
-	errno = 0;
-	val = strtol(arg, NULL, 0);
+	const char *p = arg;
+	unsigned long val;
 
-	if (errno || (val != STDIN_FILENO && val <= STDERR_FILENO) ||
-	    val > INT_MAX)
+	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
+	    val > INT_MAX					||
+	    (val != STDIN_FILENO && val <= STDERR_FILENO))
 		die("Invalid --fd: %s", arg);
 
 	return val;
@@ -1288,6 +1285,8 @@ void conf(struct ctx *c, int argc, char **argv)
 
 	optind = 0;
 	do {
+		const char *p;
+
 		name = getopt_long(argc, argv, optstring, options, NULL);
 
 		switch (name) {
@@ -1368,14 +1367,17 @@ void conf(struct ctx *c, int argc, char **argv)
 		case 12:
 			runas = optarg;
 			break;
-		case 13:
-			errno = 0;
-			logsize = strtol(optarg, NULL, 0);
+		case 13: {
+			unsigned long val;
 
-			if (logsize < LOGFILE_SIZE_MIN || errno)
+			p = optarg;
+			if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) ||
+			    val < LOGFILE_SIZE_MIN)
 				die("Invalid --log-size: %s", optarg);
 
+			logsize = val;
 			break;
+		}
 		case 14:
 			FPRINTF(stdout,
 				c->mode == MODE_PASTA ? "pasta " : "passt ");
@@ -1552,12 +1554,9 @@ void conf(struct ctx *c, int argc, char **argv)
 			break;
 		case 'm': {
 			unsigned long mtu;
-			char *e;
-
-			errno = 0;
-			mtu = strtoul(optarg, &e, 0);
 
-			if (errno || *e)
+			p = optarg;
+			if (!parse_unsigned(&p, 0, &mtu) || !parse_eoi(p))
 				die("Invalid MTU: %s", optarg);
 
 			if (mtu > max_mtu) {
diff --git a/fwd_rule.c b/fwd_rule.c
index d0e90402..d4b422d7 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -41,10 +41,11 @@ static in_port_t fwd_ephemeral_max = NUM_PORTS - 1;
  */
 void fwd_probe_ephemeral(void)
 {
-	char *line, *tab, *end;
+	unsigned long min, max;
 	struct lineread lr;
-	long min, max;
+	const char *p;
 	ssize_t len;
+	char *line;
 	int fd;
 
 	fd = open(PORT_RANGE_SYSCTL, O_RDONLY | O_CLOEXEC);
@@ -57,35 +58,20 @@ void fwd_probe_ephemeral(void)
 	len = lineread_get(&lr, &line);
 	close(fd);
 
-	if (len < 0)
-		goto parse_err;
-
-	tab = strchr(line, '\t');
-	if (!tab)
-		goto parse_err;
-	*tab = '\0';
-
-	errno = 0;
-	min = strtol(line, &end, 10);
-	if (*end || errno)
-		goto parse_err;
-
-	errno = 0;
-	max = strtol(tab + 1, &end, 10);
-	if (*end || errno)
-		goto parse_err;
-
-	if (min < 0 || min >= (long)NUM_PORTS ||
-	    max < 0 || max >= (long)NUM_PORTS)
-		goto parse_err;
+	p = line;
+	if (len < 0				||
+	    !parse_unsigned(&p, 10, &min)	||
+	    !parse_literal(&p, "\t")		||
+	    !parse_unsigned(&p, 10, &max)	||
+	    !parse_eoi(p)			||
+	    min >= NUM_PORTS			||
+	    max >= NUM_PORTS) {
+		warn("Unable to parse %s", PORT_RANGE_SYSCTL);
+		return;
+	}
 
 	fwd_ephemeral_min = min;
 	fwd_ephemeral_max = max;
-
-	return;
-
-parse_err:
-	warn("Unable to parse %s", PORT_RANGE_SYSCTL);
 }
 
 /**
diff --git a/parse.c b/parse.c
index c2a92422..8abfd4dd 100644
--- a/parse.c
+++ b/parse.c
@@ -12,14 +12,12 @@
  * Author: David Gibson <david@gibson.dropbear.id.au>
  */
 
-#include <assert.h>
+#include <errno.h>
 #include <stddef.h>
-#include <fcntl.h>
+#include <stdlib.h>
 #include <string.h>
-#include <stdbool.h>
-#include <unistd.h>
 
-#include "common.h"
+#include "parse.h"
 
 /**
  * DOC: Theory of Operation
@@ -65,3 +63,22 @@ bool parse_eoi(const char *cursor)
 {
 	return !(*cursor);
 }
+
+/*
+ * parse_unsigned() - Parse an unsigned integer
+ * @base:	Numeric base for string as strtoul(3)
+ * @valp:	On success, updated with parsed value
+ */
+bool parse_unsigned(const char **cursor, int base, unsigned long *valp)
+{
+	const char *p = *cursor;
+	unsigned long val;
+
+	errno = 0;
+	val = strtoul(p, (char **)&p, base);
+	if (errno || p == *cursor)
+		return false;
+	*valp = val;
+	*cursor = p;
+	return true;
+}
diff --git a/parse.h b/parse.h
index eb51a469..4f38c28b 100644
--- a/parse.h
+++ b/parse.h
@@ -10,5 +10,6 @@
 
 bool parse_literal(const char **cursor, const char *lit);
 bool parse_eoi(const char *cursor);
+bool parse_unsigned(const char **cursor, int base, unsigned long *valp);
 
 #endif /* _PARSE_H */
-- 
2.54.0

[PATCH 07/12] parse: Move parse_port_range() to new parsing framework

[David Gibson]

parse_port_range() in fwd_rule.c takes an approach similar to that used
by the new parsing helpers in parse.c (and is partially inspiration for
it), but isn't quite the same.  Move it into parse.c, and bring it into
line with that file's conventions.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 common.h   |  3 +++
 fwd_rule.c | 53 +++--------------------------------------------------
 fwd_rule.h |  2 --
 parse.c    | 28 ++++++++++++++++++++++++++++
 parse.h    | 13 +++++++++++++
 5 files changed, 47 insertions(+), 52 deletions(-)

diff --git a/common.h b/common.h
index f2518f66..16b9dc8c 100644
--- a/common.h
+++ b/common.h
@@ -113,4 +113,7 @@ static inline const char *strerror_(int errnum)
 #define ntohll(x)		(be64toh((x)))
 #define htonll(x)		(htobe64((x)))
 
+/* Number of ports for both TCP and UDP */
+#define	NUM_PORTS	(1U << 16)
+
 #endif /* COMMON_H */
diff --git a/fwd_rule.c b/fwd_rule.c
index d4b422d7..0b85b17e 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -367,53 +367,6 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
 	return 0;
 }
 
-/**
- * port_range() - Represents a non-empty range of ports
- * @first:	First port number in the range
- * @last:	Last port number in the range (inclusive)
- *
- * Invariant:	@last >= @first
- */
-struct port_range {
-	in_port_t first, last;
-};
-
-/**
- * parse_port_range() - Parse a range of port numbers '<first>[-<last>]'
- * @s:		String to parse
- * @endptr:	Update to the character after the parsed range (similar to
- *		strtol() etc.)
- * @range:	Update with the parsed values on success
- *
- * Return: -EINVAL on parsing error, -ERANGE on out of range port
- *	   numbers, 0 on success
- */
-static int parse_port_range(const char *s, const char **endptr,
-			    struct port_range *range)
-{
-	unsigned long first, last;
-	char *ep;
-
-	last = first = strtoul(s, &ep, 10);
-	if (ep == s) /* Parsed nothing */
-		return -EINVAL;
-	if (*ep == '-') { /* we have a last value too */
-		const char *lasts = ep + 1;
-		last = strtoul(lasts, &ep, 10);
-		if (ep == lasts) /* Parsed nothing */
-			return -EINVAL;
-	}
-
-	if ((last < first) || (last >= NUM_PORTS))
-		return -ERANGE;
-
-	range->first = first;
-	range->last = last;
-	*endptr = ep;
-
-	return 0;
-}
-
 /**
  * fwd_rule_range_except() - Set up forwarding for a range of ports minus a
  *                           bitmap of exclusions
@@ -550,7 +503,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 		if (!parse_literal(&p, "~"))
 			goto bad;
 
-		if (parse_port_range(p, &p, &xrange))
+		if (!parse_port_range(&p, &xrange))
 			goto bad;
 		if (p != ep) /* Garbage after the range */
 			goto bad;
@@ -577,12 +530,12 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 			/* Already parsed */
 			continue;
 
-		if (parse_port_range(p, &p, &orig_range))
+		if (!parse_port_range(&p, &orig_range))
 			goto bad;
 
 		if (parse_literal(&p, ":")) {
 			/* There's a range to map to as well */
-			if (parse_port_range(p, &p, &mapped_range))
+			if (!parse_port_range(&p, &mapped_range))
 				goto bad;
 			if ((mapped_range.last - mapped_range.first) !=
 			    (orig_range.last - orig_range.first))
diff --git a/fwd_rule.h b/fwd_rule.h
index ae9a3cb0..435be5bd 100644
--- a/fwd_rule.h
+++ b/fwd_rule.h
@@ -18,8 +18,6 @@
 #include "inany.h"
 #include "bitmap.h"
 
-/* Number of ports for both TCP and UDP */
-#define	NUM_PORTS	(1U << 16)
 #define PORT_BITMAP_SIZE	DIV_ROUND_UP(NUM_PORTS, 8)
 
 /* Forwarding capability bits */
diff --git a/parse.c b/parse.c
index 8abfd4dd..bd091fde 100644
--- a/parse.c
+++ b/parse.c
@@ -17,6 +17,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "common.h"
 #include "parse.h"
 
 /**
@@ -82,3 +83,30 @@ bool parse_unsigned(const char **cursor, int base, unsigned long *valp)
 	*cursor = p;
 	return true;
 }
+
+/**
+ * parse_port_range() - Parse a range of port numbers '<first>[-<last>]'
+ * @range:	Update with the parsed values on success
+ */
+bool parse_port_range(const char **cursor, struct port_range *range)
+{
+	unsigned long first, last;
+	const char *p = *cursor;
+
+	if (!parse_unsigned(&p, 10, &first))
+		return false;
+
+	last = first;
+
+	if (parse_literal(&p, "-"))
+		if (!parse_unsigned(&p, 10, &last))
+			return false;
+
+	if ((last < first) || (last >= NUM_PORTS))
+		return false;
+
+	range->first = first;
+	range->last = last;
+	*cursor = p;
+	return true;
+}
diff --git a/parse.h b/parse.h
index 4f38c28b..155b3995 100644
--- a/parse.h
+++ b/parse.h
@@ -7,9 +7,22 @@
 #define PARSE_H
 
 #include <stdbool.h>
+#include <netinet/in.h>
+
+/**
+ * port_range() - Represents a non-empty range of ports
+ * @first:	First port number in the range
+ * @last:	Last port number in the range (inclusive)
+ *
+ * Invariant:	@last >= @first
+ */
+struct port_range {
+	in_port_t first, last;
+};
 
 bool parse_literal(const char **cursor, const char *lit);
 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);
 
 #endif /* _PARSE_H */
-- 
2.54.0

[PATCH 08/12] parse: Add helpers for parsing IP addresses

[David Gibson]

parse_ipv[46]() are wrappers around inet_pton() that are more
convenient for use when the IP is part of a longer string, rather than
the entire string.  parse_inany() replaces inany_pton() which again
will become more convenient for strings including IPs that aren't just
an IP.

For now we only have some simple and sometimes awkward use cases,
we'll replace these with more natural uses in future.

Cc: Jon Maloy <jamloy@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile   |  1 +
 conf.c     |  6 ++--
 fwd_rule.c | 21 ++++---------
 inany.c    | 24 ++------------
 inany.h    |  1 -
 parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse.h    |  4 +++
 7 files changed, 109 insertions(+), 39 deletions(-)

diff --git a/Makefile b/Makefile
index 85d279c0..e2b22ddf 100644
--- a/Makefile
+++ b/Makefile
@@ -227,4 +227,5 @@ 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
+pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
 pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
diff --git a/conf.c b/conf.c
index bd357117..2d1895d4 100644
--- a/conf.c
+++ b/conf.c
@@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
 	struct in_addr mask;
 	unsigned long len;
 
-	if (inet_pton(AF_INET, arg, &mask)) {
+	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
 		in_addr_t hmask = ntohl(mask.s_addr);
 		len = __builtin_popcount(hmask);
 		if ((hmask << len) == 0)
@@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (addr_has_prefix_len && prefix_len_from_opt)
 				die("Redundant prefix length specification");
 
-			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
+			p = optarg;
+			if (!addr_has_prefix_len &&
+			    !(parse_inany(&p, &addr) && parse_eoi(p)))
 				die("Invalid address: %s", optarg);
 
 			if (prefix_len_from_opt && inany_v4(&addr))
diff --git a/fwd_rule.c b/fwd_rule.c
index 0b85b17e..6d7ec2c5 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 	strncpy(buf, optarg, sizeof(buf) - 1);
 
 	if ((spec = strchr(buf, '/'))) {
+		const char *p = buf;
+
 		*spec = 0;
 		spec++;
 
@@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 			}
 		}
 
-		if (ifname == buf + 1) {	/* Interface without address */
+		if (ifname == buf + 1 || /* Interface without address */
+		    !strcmp(buf, "*")) /* Explicit wildcard address */
 			addr = NULL;
-		} else {
-			char *p = buf;
-
-			/* Allow square brackets for IPv4 too for convenience */
-			if (*p == '[' && p[strlen(p) - 1] == ']') {
-				p[strlen(p) - 1] = '\0';
-				p++;
-			}
-
-			if (strcmp(p, "*") == 0)
-				addr = NULL;
-			else if (!inany_pton(p, &addr_buf))
-				die("Bad forwarding address '%s'", p);
-		}
+		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
+			die("Bad forwarding address '%s'", buf);
 	} else {
 		spec = buf;
 
diff --git a/inany.c b/inany.c
index 23faf3ff..154f08b5 100644
--- a/inany.c
+++ b/inany.c
@@ -27,6 +27,7 @@
 #include "ip.h"
 #include "inany.h"
 #include "fwd.h"
+#include "parse.h"
 
 const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
 const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
@@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
 	return inet_ntop(AF_INET6, &src->a6, dst, size);
 }
 
-/** inany_pton - Parse an IPv[46] address from text format
- * @src:	IPv[46] address
- * @dst:	output buffer, filled with parsed address
- *
- * Return: on success, 1, if no parseable address is found, 0
- */
-int inany_pton(const char *src, union inany_addr *dst)
-{
-	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
-		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
-		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
-		return 1;
-	}
-
-	if (inet_pton(AF_INET6, src, &dst->a6))
-		return 1;
-
-	return 0;
-}
-
 /**
  * inany_prefix_pton() - Parse an IPv[46] address with prefix length
  * @src:	IPv[46] address and prefix length string in CIDR format
@@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
 	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;
 
@@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
 		return 1;
 	}
 
-	if (inany_pton(astr, dst)) {
+	if (parse_inany(&p, dst) && parse_eoi(p)) {
 		if (plen > 32)
 			return 0;
 		*prefix_len = plen + 96;
diff --git a/inany.h b/inany.h
index 6bf3ecaa..93d98368 100644
--- a/inany.h
+++ b/inany.h
@@ -303,7 +303,6 @@ 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_pton(const char *src, union inany_addr *dst);
 int inany_prefix_pton(const char *src, union inany_addr *dst,
 		      uint8_t *prefix_len);
 
diff --git a/parse.c b/parse.c
index bd091fde..0349c5dc 100644
--- a/parse.c
+++ b/parse.c
@@ -16,9 +16,12 @@
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
+#include <limits.h>
+#include <arpa/inet.h>
 
 #include "common.h"
 #include "parse.h"
+#include "inany.h"
 
 /**
  * DOC: Theory of Operation
@@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
 	*cursor = p;
 	return true;
 }
+
+/**
+ * parse_ipv4() - Parse an IPv4 address from a string
+ * @abuf:	On success, updated with parsed address
+ */
+bool parse_ipv4(const char **cursor, struct in_addr *abuf)
+{
+	/* Brackets are not typical on IPv4, but allow for consistency */
+	const char *p = *cursor;
+	bool bracket = parse_literal(&p, "[");
+	char buf[INET_ADDRSTRLEN];
+	struct in_addr addr;
+	size_t len;
+
+	if (bracket)
+		len = strcspn(p, "]");
+	else
+		len = strspn(p, "0123456789.");
+
+	if (len >= sizeof(buf))
+		return false;
+	memcpy(buf, p, len);
+	buf[len] = '\0';
+	p += len;
+
+	if (!inet_pton(AF_INET, buf, &addr))
+		return false;
+
+	if (bracket && !parse_literal(&p, "]"))
+		return false;
+
+	*cursor = p;
+	*abuf = addr;
+	return true;
+}
+
+/**
+ * parse_ipv6() - Parse an IPv6 address from a string
+ * @abuf:	On success, updated with parsed address
+ */
+static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
+{
+	const char *p = *cursor;
+	bool bracket = parse_literal(&p, "[");
+	char buf[INET6_ADDRSTRLEN];
+	struct in6_addr addr;
+	size_t len;
+
+	if (bracket)
+		len = strcspn(p, "]");
+	else
+		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
+
+	if (len >= sizeof(buf))
+		return false;
+	memcpy(buf, p, len);
+	buf[len] = '\0';
+	p += len;
+
+	if (!inet_pton(AF_INET6, buf, &addr))
+		return false;
+
+	if (bracket && !parse_literal(&p, "]"))
+		return false;
+
+	*cursor = p;
+	*abuf = addr;
+	return true;
+}
+
+/**
+ * parse_inany() - Parse an IPv4 or IPv6 address from a string
+ * @addr:	On success, updated with parsed address
+ */
+bool parse_inany(const char **cursor, union inany_addr *addr)
+{
+	struct in_addr a4;
+
+	if (parse_ipv6(cursor, &addr->a6))
+		return true;
+
+	if (parse_ipv4(cursor, &a4)) {
+		*addr = inany_from_v4(a4);
+		return true;
+	}
+
+	return false;
+}
diff --git a/parse.h b/parse.h
index 155b3995..2820a065 100644
--- a/parse.h
+++ b/parse.h
@@ -9,6 +9,8 @@
 #include <stdbool.h>
 #include <netinet/in.h>
 
+union inany_addr;
+
 /**
  * port_range() - Represents a non-empty range of ports
  * @first:	First port number in the range
@@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
 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);
 
 #endif /* _PARSE_H */
-- 
2.54.0

[PATCH 09/12] conf: Move address configuration into helper function

[David Gibson]

The handling of the -a option is getting complex enough that it's pretty
bulky inside it's switch label.  Move it into a new conf_addr() function.
We also rename the bulky addr_has_prefix_len and prefix_len_from_opt
variables to the terser 'opt_a_is_prefix' and 'opt_n', since they are
specifically about those command line options.

Cc: Jon Maloy <jmaloy@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 96 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 43 deletions(-)

diff --git a/conf.c b/conf.c
index 2d1895d4..3614776c 100644
--- a/conf.c
+++ b/conf.c
@@ -1168,6 +1168,53 @@ int conf_tap_fd(const char *arg)
 	return val;
 }
 
+/**
+ * conf_addr() - Configure guest address with -a option
+ * @c:		Execution context
+ * @arg:	-a command line argument
+ * @opt_n:	Value from -n option, if any
+ */
+static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n)
+{
+	union inany_addr addr;
+	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 (inany_is_unspecified(&addr) || inany_is_multicast(&addr) ||
+	    inany_is_loopback(&addr) || IN6_IS_ADDR_V4COMPAT(&addr.a6))
+		die("Invalid address: %s", arg);
+
+	if (inany_v4(&addr)) {
+		c->ip4.addr = *inany_v4(&addr);
+		c->ip4.prefix_len = prefix_len - 96;
+		c->ip4.addr_fixed = true;
+		if (c->mode == MODE_PASTA)
+			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;
+	}
+
+	return is_prefix;
+}
+
 /**
  * conf() - Process command-line arguments and set configuration
  * @c:		Execution context
@@ -1262,12 +1309,12 @@ void conf(struct ctx *c, int argc, char **argv)
 	unsigned dns4_idx = 0, dns6_idx = 0;
 	unsigned long max_mtu = IP_MAX_MTU;
 	struct fqdn *dnss = c->dns_search;
-	bool addr_has_prefix_len = false;
-	uint8_t prefix_len_from_opt = 0;
 	unsigned int ifi4 = 0, ifi6 = 0;
+	bool opt_a_is_prefix = false;
 	const char *logfile = NULL;
 	char *runas = NULL;
 	size_t logsize = 0;
+	uint8_t opt_n = 0;
 	int name, ret;
 	uid_t uid;
 	gid_t gid;
@@ -1567,52 +1614,15 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->mtu = mtu;
 			break;
 		}
-		case 'a': {
-			union inany_addr addr;
-			uint8_t prefix_len;
-
-			addr_has_prefix_len = inany_prefix_pton(optarg, &addr,
-								&prefix_len);
-
-			if (addr_has_prefix_len && prefix_len_from_opt)
-				die("Redundant prefix length specification");
-
-			p = optarg;
-			if (!addr_has_prefix_len &&
-			    !(parse_inany(&p, &addr) && parse_eoi(p)))
-				die("Invalid address: %s", optarg);
-
-			if (prefix_len_from_opt && inany_v4(&addr))
-				prefix_len = prefix_len_from_opt;
-			else if (!addr_has_prefix_len)
-				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", optarg);
-
-			if (inany_v4(&addr)) {
-				c->ip4.addr = *inany_v4(&addr);
-				c->ip4.prefix_len = prefix_len - 96;
-				c->ip4.addr_fixed = true;
-				if (c->mode == MODE_PASTA)
-					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;
-			}
+		case 'a':
+			opt_a_is_prefix = conf_addr(c, optarg, opt_n);
 			break;
-		}
 		case 'n':
-			if (addr_has_prefix_len)
+			if (opt_a_is_prefix)
 				die("Redundant prefix length specification");
 
 			c->ip4.prefix_len = conf_ip4_prefix(optarg);
-			prefix_len_from_opt = c->ip4.prefix_len + 96;
+			opt_n = c->ip4.prefix_len + 96;
 			break;
 		case 'M':
 			parse_mac(c->our_tap_mac, optarg);
-- 
2.54.0

[PATCH 10/12] conf: Use new parsing tools to handle -a option

[David Gibson]

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.

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;
 	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))
+		goto bad;
+	a4 = inany_v4(&addr);
+
+	if ((is_prefix = parse_literal(&p, "/"))) {
+		/* 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)		||
+	    !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)
-			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 */
-- 
2.54.0

[PATCH 11/12] fwd_rule: Allow "all" port specs to be combined with other options

[David Gibson]

Currently we handle -t all and the like as a special case, it can't be
combined with other port specifier options.  Remove that restriction,
allowing combined options like:
     -t all,~9999          # Forward everything non-ephemeral except 9999
     -t all,auto           # Equivalent to -t auto
     -t all,33000          # Forward non-ephemeral plus port 33,000

This isn't particularly useful immediately, but will become important for
destination address specification - it provides a place to attach the
target address for "all" or exclude only mappings.  It will also work
better with some parsing reworks we want to make.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd_rule.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/fwd_rule.c b/fwd_rule.c
index 6d7ec2c5..b14df340 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -471,20 +471,13 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 	uint8_t flags = 0;
 	unsigned i;
 
-	if (!strcmp(spec, "all")) {
-		/* Treat "all" as equivalent to "": all non-ephemeral ports */
-		spec = "";
-	}
-
 	/* Parse excluded ranges and "auto" in the first pass */
 	for_each_chunk(p, ep, spec, ",") {
 		struct port_range xrange;
 
-		if (isdigit(*p)) {
-			/* Include range, parse later */
-			exclude_only = false;
+		/* Include range, parse later */
+		if (parse_literal(&p, "all") || isdigit(*p))
 			continue;
-		}
 
 		if (parse_literal(&p, "auto")) {
 			if (p != ep) /* Garbage after the keyword */
@@ -512,20 +505,18 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 			bitmap_set(exclude, i);
 	}
 
-	if (exclude_only) {
-		/* Exclude ephemeral ports */
-		fwd_port_map_ephemeral(exclude);
-
-		fwd_rule_range_except(fwd, del, proto, addr, ifname,
-				      1, NUM_PORTS - 1, exclude,
-				      1, flags | FWD_WEAK);
-		return;
-	}
-
 	/* Now process base ranges, skipping exclusions */
 	for_each_chunk(p, ep, spec, ",") {
 		struct port_range orig_range, mapped_range;
 
+		/* Handle "all" like exclude only */
+		if (parse_literal(&p, "all")) {
+			if (p != ep) /* Garbage after the keyword */
+				goto bad;
+
+			continue;
+		}
+
 		if (!isdigit(*p))
 			/* Already parsed */
 			continue;
@@ -533,6 +524,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 		if (!parse_port_range(&p, &orig_range))
 			goto bad;
 
+		exclude_only = false;
+
 		if (parse_literal(&p, ":")) {
 			/* There's a range to map to as well */
 			if (!parse_port_range(&p, &mapped_range))
@@ -553,6 +546,14 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 				      mapped_range.first, flags);
 	}
 
+	/* Finally handle "all" and exclude only specs */
+	if (exclude_only) {
+		fwd_port_map_ephemeral(exclude);
+
+		fwd_rule_range_except(fwd, del, proto, addr, ifname,
+				      1, NUM_PORTS - 1, exclude,
+				      1, flags | FWD_WEAK);
+	}
 	return;
 bad:
 	die("Invalid port specifier '%s'", spec);
-- 
2.54.0

[PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers

[David Gibson]

Forwarding rules for the -[tTuU] options are parsed in fwd_rule_parse()
and fwd_rule_parse_ports().  Currently those use a mix of loosely
recursive descent parsing helpers, consuming input from left to right,
and ad-hoc C parsing - searching for delimeters with strchr() or the like
and breaking down on that basis.

As well as being a slightly awkward mix, the current approach will not work
for adding target addresses to the rules: we can't easily split the
listening from target parts first, because the ':' delimiter could appear
multiple times for multiple port ranges, or in IPv6 addresses.  However,
without splitting the target part first, we can't first split off the
listening address and interface because the '/' delimiter could appear in
the target portion, but not the listening portion, e.g.:
    -t 12345:192.0.1.1/12345

To address this, rewrite the entirety of the parsing so we consume the
input left to right in a more-or-less recursive descent manner.  This means
we no longer rely on certain delimiters having a single meaning over the
whole input, just the next part of it.

Because of the semantics of port specs, we need to make several passes
over the list of comma separated chunks.  Previously, some of the logic was
duplicated between the two passes, making it fragile.  Now, we introduce
a parse_port_chunk() helper which we re-use in each pass to make it clearer
we parse exactly the same way on each pass.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd_rule.c | 262 ++++++++++++++++++++++++++++++-----------------------
 parse.c    |  29 ++++++
 parse.h    |   2 +
 3 files changed, 181 insertions(+), 112 deletions(-)

diff --git a/fwd_rule.c b/fwd_rule.c
index b14df340..8ae21b99 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -439,17 +439,96 @@ fail:
 	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
 }
 
-/*
- * for_each_chunk - Step through delimited chunks of a string
- * @p_:		Pointer to start of each chunk (updated)
- * @ep_:	Pointer to end of each chunk (updated)
- * @s_:		String to step through
- * @sep_:	String of all allowed delimiters
+/**
+ * enum fwd_port_chunk_kind - Kind of port specifier piece
+ */
+enum fwd_port_chunk_kind {
+	CHUNK_ALL,		/* "all" */
+	CHUNK_AUTO,		/* "auto" */
+	CHUNK_EXCLUDE,		/* "~1111-2222" */
+	CHUNK_INCLUDE,		/* "1111-2222" */
+};
+
+/**
+ * parse_port_chunk() - Parse one chunk of a port specifier
+ * @cursor:	Parsing point (see parse.c)
+ * @kindp:	Updated with kind of chunk we parsed
+ * @lrange:	Updated with listening port range (for INCLUDE & EXCLUDE)
+ * @trange:	Updated with target port range (for EXCLUDE)
+ */
+static bool parse_port_chunk(const char **cursor, enum fwd_port_chunk_kind *kindp,
+			     struct port_range *lrange, struct port_range *trange)
+{
+	struct port_range lr = { 0 }, tr = { 0 };
+	enum fwd_port_chunk_kind kind;
+	const char *p = *cursor;
+
+	if (parse_literal(&p, "all")) {
+		kind = CHUNK_ALL;
+	} else if (parse_literal(&p, "auto")) {
+		kind = CHUNK_AUTO;
+	} else if (parse_literal(&p, "~")) {
+		kind = CHUNK_EXCLUDE;
+		if (!parse_port_range(&p, &lr))
+			return false;
+	} else if (parse_port_range(&p, &lr)) {
+		kind = CHUNK_INCLUDE;
+
+		if (parse_literal(&p, ":")) {
+			if (!parse_port_range(&p, &tr))
+				return false;
+		} else {
+			tr = lr;
+		}
+	} else {
+		return false;
+	}
+
+	*kindp = kind;
+	*lrange = lr;
+	if (trange)
+		*trange = tr;
+	*cursor = p;
+	return true;
+}
+
+/**
+ * parse_laddrif() - Parse listening address+ifname
+ * @cursor:	Parsing cursor (see parse.c)
+ * @addr:	Updated with parsed inany address (NULL for *)
+ * @abuf:	Buffer to store address
+ * @ifname:	Updated with parsed interface name ("" if none)
  */
-#define for_each_chunk(p_, ep_, s_, sep_)			\
-	for ((p_) = (s_);					\
-	     (ep_) = (p_) + strcspn((p_), (sep_)), *(p_);	\
-	     (p_) = *(ep_) ? (ep_) + 1 : (ep_))
+static bool parse_laddrif(const char **cursor,
+			  const union inany_addr **addr,
+			  union inany_addr *abuf,
+			  char *ifname)
+{
+	union inany_addr atmp = inany_any6;
+	char iftmp[IFNAMSIZ] = {0};
+	const char *p;
+
+	if (p = *cursor,
+	    parse_inany(&p, &atmp)		&&
+	    parse_ifspec(&p, iftmp)		&&
+	    parse_literal(&p, "/")) {
+		/* Specific listening address */
+		*addr = abuf;
+	} else if (p = *cursor,
+		   parse_literal(&p, "*"),
+		   parse_ifspec(&p, iftmp)	&&
+		   parse_literal(&p, "/")) {
+		/* Missing or "*" address */
+		*addr = NULL;
+	} else {
+		return false;
+	}
+
+	*abuf = atmp;
+	memcpy(ifname, iftmp, IFNAMSIZ);
+	*cursor = p;
+	return true;
+}
 
 /**
  * fwd_rule_parse_ports() - Parse port range(s) specifier
@@ -466,87 +545,70 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 				 const char *spec)
 {
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
+	enum fwd_port_chunk_kind kind;
+	struct port_range lrange;
 	bool exclude_only = true;
-	const char *p, *ep;
 	uint8_t flags = 0;
+	const char *p;
 	unsigned i;
 
-	/* Parse excluded ranges and "auto" in the first pass */
-	for_each_chunk(p, ep, spec, ",") {
-		struct port_range xrange;
-
-		/* Include range, parse later */
-		if (parse_literal(&p, "all") || isdigit(*p))
-			continue;
-
-		if (parse_literal(&p, "auto")) {
-			if (p != ep) /* Garbage after the keyword */
-				goto bad;
+	/* Consider excluded ranges and "auto" in the first pass */
+	p = spec;
+	do {
+		if (!parse_port_chunk(&p, &kind, &lrange, NULL))
+			goto bad;
 
+		switch (kind) {
+		case CHUNK_AUTO:
 			if (!(fwd->caps & FWD_CAP_SCAN)) {
 				die(
 "'auto' port forwarding is only allowed for pasta");
 			}
-
 			flags |= FWD_SCAN;
-			continue;
-		}
-
-		/* Should be an exclude range */
-		if (!parse_literal(&p, "~"))
-			goto bad;
-
-		if (!parse_port_range(&p, &xrange))
-			goto bad;
-		if (p != ep) /* Garbage after the range */
-			goto bad;
-
-		for (i = xrange.first; i <= xrange.last; i++)
-			bitmap_set(exclude, i);
-	}
-
-	/* Now process base ranges, skipping exclusions */
-	for_each_chunk(p, ep, spec, ",") {
-		struct port_range orig_range, mapped_range;
-
-		/* Handle "all" like exclude only */
-		if (parse_literal(&p, "all")) {
-			if (p != ep) /* Garbage after the keyword */
-				goto bad;
+			break;
 
-			continue;
+		case CHUNK_EXCLUDE:
+			for (i = lrange.first; i <= lrange.last; i++)
+				bitmap_set(exclude, i);
+			break;
+		default:
+			; /* Handled later */
 		}
+	} while (parse_literal(&p, ","));
 
-		if (!isdigit(*p))
-			/* Already parsed */
-			continue;
+	/* Consider included ranges in next pass */
+	p = spec;
+	do {
+		struct port_range trange;
 
-		if (!parse_port_range(&p, &orig_range))
+		if (!parse_port_chunk(&p, &kind, &lrange, &trange))
 			goto bad;
 
-		exclude_only = false;
+		switch (kind) {
+		case CHUNK_AUTO:	/* already handled */
+		case CHUNK_EXCLUDE:	/* already handled */
+		case CHUNK_ALL:		/* handled later */
+			continue;
 
-		if (parse_literal(&p, ":")) {
-			/* There's a range to map to as well */
-			if (!parse_port_range(&p, &mapped_range))
-				goto bad;
-			if ((mapped_range.last - mapped_range.first) !=
-			    (orig_range.last - orig_range.first))
+		case CHUNK_INCLUDE:
+			exclude_only = false;
+			if (trange.last - trange.first !=
+			    lrange.last - lrange.first)
 				goto bad;
-		} else {
-			mapped_range = orig_range;
-		}
 
-		if (p != ep) /* Garbage after the ranges */
+			fwd_rule_range_except(fwd, del, proto, addr, ifname,
+					      lrange.first, lrange.last,
+					      exclude, trange.first, flags);
+			break;
+		default:
 			goto bad;
+		}
+	} while (parse_literal(&p, ","));
 
-		fwd_rule_range_except(fwd, del, proto, addr, ifname,
-				      orig_range.first, orig_range.last,
-				      exclude,
-				      mapped_range.first, flags);
-	}
+	if (!parse_eoi(p))
+		goto bad; /* trailing garbage */
 
-	/* Finally handle "all" and exclude only specs */
+	/* Finally handle "all" and exclude only cases */
 	if (exclude_only) {
 		fwd_port_map_ephemeral(exclude);
 
@@ -569,10 +631,11 @@ bad:
 void fwd_rule_parse(char optname, bool del, const char *optarg,
 		    struct fwd_table *fwd)
 {
-	char buf[BUFSIZ], *spec, *ifname = NULL;
-	union inany_addr addr_buf = inany_any6;
-	const union inany_addr *addr = &addr_buf;
+	const union inany_addr *addr;
+	union inany_addr addr_buf;
+	char ifname[IFNAMSIZ];
 	uint8_t proto;
+	const char *p;
 
 	if (optname == 't' || optname == 'T')
 		proto = IPPROTO_TCP;
@@ -581,7 +644,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 	else
 		assert(0);
 
-	if (!strcmp(optarg, "none")) {
+	if (p = optarg,
+	    parse_literal(&p, "none") && parse_eoi(p)) {
 		unsigned i;
 
 		for (i = 0; i < fwd->count; i++) {
@@ -593,45 +657,19 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 		return;
 	}
 
-	strncpy(buf, optarg, sizeof(buf) - 1);
-
-	if ((spec = strchr(buf, '/'))) {
-		const char *p = buf;
-
-		*spec = 0;
-		spec++;
-
-		if (optname != 't' && optname != 'u')
+	if (p = optarg,
+	    parse_laddrif(&p, &addr, &addr_buf, ifname)) {
+		if (optname == 'T' || optname == 'U')
 			die("Listening address not allowed for -%c %s",
 			    optname, optarg);
-
-		if ((ifname = strchr(buf, '%'))) {
-			*ifname = 0;
-			ifname++;
-
-			/* spec is already advanced one past the '/',
-			 * so the length of the given ifname is:
-			 * (spec - ifname - 1)
-			 */
-			if (spec - ifname - 1 >= IFNAMSIZ) {
-				die("Interface name '%s' is too long (max %u)",
-				    ifname, IFNAMSIZ - 1);
-			}
-		}
-
-		if (ifname == buf + 1 || /* Interface without address */
-		    !strcmp(buf, "*")) /* Explicit wildcard address */
-			addr = NULL;
-		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
-			die("Bad forwarding address '%s'", buf);
 	} else {
-		spec = buf;
-
+		/* No address or ifname */
 		addr = NULL;
+		ifname[0] = '\0';
 	}
 
 	if (optname == 'T' || optname == 'U') {
-		assert(!addr && !ifname);
+		assert(!addr && !*ifname);
 
 		if (!(fwd->caps & FWD_CAP_IFNAME)) {
 			warn(
@@ -640,18 +678,18 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 
 			if (fwd->caps & FWD_CAP_IPV4) {
 				fwd_rule_parse_ports(fwd, del, proto,
-						     &inany_loopback4, NULL,
-						     spec);
+						     &inany_loopback4, NULL, p);
 			}
 			if (fwd->caps & FWD_CAP_IPV6) {
 				fwd_rule_parse_ports(fwd, del, proto,
-						     &inany_loopback6, NULL,
-						     spec);
+						     &inany_loopback6, NULL, p);
 			}
 			return;
 		}
 
-		ifname = "lo";
+		static_assert(sizeof("lo") <= sizeof(ifname),
+			      "ifname buffer too small");
+		strcpy(ifname, "lo");
 	}
 
 	/* No need for dual stack if we only have one IP version */
@@ -660,13 +698,13 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 	else if (!addr && !(fwd->caps & FWD_CAP_IPV6))
 		addr = &inany_any4;
 
-	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
+	if (*ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
 		die(
 "Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
 		    optname, optarg);
 	}
 
-	fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec);
+	fwd_rule_parse_ports(fwd, del, proto, addr, *ifname ? ifname : NULL, p);
 }
 
 /**
diff --git a/parse.c b/parse.c
index 3e0dbd45..d83ea9f9 100644
--- a/parse.c
+++ b/parse.c
@@ -18,6 +18,7 @@
 #include <string.h>
 #include <limits.h>
 #include <arpa/inet.h>
+#include <net/if.h>
 
 #include "common.h"
 #include "parse.h"
@@ -212,3 +213,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
 
 	return false;
 }
+
+/**
+ * parse_ifspec() - Parse a interface name specifier (starting with %)
+ * @ifname:	On success updated with parsed name (must have IFNAMSIZ space)
+ *
+ * This will accept a missing specifier (empty string), setting ifname to ""
+ */
+bool parse_ifspec(const char **cursor, char *ifname)
+{
+	const char *p = *cursor;
+	size_t len;
+
+	if (!parse_literal(&p, "%")) {
+		/* No interface specifier */
+		ifname[0] = '\0';
+		return true;
+	}
+
+	/* ifnames can have anything that's not '/' or whitespace */
+	len = strcspn(p, "/ \f\n\r\t\v");
+	if (!len || len >= IFNAMSIZ)
+		return false;
+
+	memcpy(ifname, p, len);
+	ifname[len] = '\0';
+	*cursor = p + len;
+	return true;
+}
diff --git a/parse.h b/parse.h
index 08b038cf..1079f5bf 100644
--- a/parse.h
+++ b/parse.h
@@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
 
 #define parse_inany(cursor, addr)	parse_inany_((cursor), (addr), NULL)
 
+bool parse_ifspec(const char **cursor, char *ifname);
+
 #endif /* _PARSE_H */
-- 
2.54.0

Re: [PATCH 01/12] Makefile: Add missing PESTO_HEADERS variable

[Stefano Brivio]

On Fri, 26 Jun 2026 17:09:52 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> In several places we use a PESTO_HEADERS variable, with all the headers
> that we need to build the pesto binary.  However, we never define it.
> This looks like an error introduced by a bad rebase of the series
> introducing pesto before it was merged.

Oops, yes, I just found that in a patch reject file. And I just
discovered that a Makefile variable that's not defined happily expands
to nothing, I wasn't aware of that.

> It turns out the fact we didn't list the headers was the only reason we
> weren't getting unusedStructMember cppcheck warnings for pesto as we
> already do for passt and passt-repair.  So, reinstate that suppression for
> pesto as well.
> 
> Fixes: 02236db32625 ("pesto: Introduce stub configuration tool")
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  Makefile | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e8170e90..5ed0f702 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -55,6 +55,7 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
>  	virtio.h vu_common.h
>  QRAP_HEADERS = arp.h ip.h passt.h util.h
>  PASST_REPAIR_HEADERS = linux_dep.h
> +PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h
>  
>  C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
>  ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> @@ -203,7 +204,8 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
>  	else								\
>  		echo "";						\
>  	fi)								\
> -	--suppress=missingIncludeSystem
> +	--suppress=missingIncludeSystem					\
> +	--suppress=unusedStructMember
>  
>  cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
>  
> @@ -212,10 +214,8 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
>  	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
>  
>  passt.cppcheck: BASE_CPPFLAGS += -UPESTO
> -passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember
>  passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
>  
> -passt-repair.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember
>  passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
>  
>  pesto.cppcheck: BASE_CPPFLAGS += -DPESTO

-- 
Stefano

Re: [PATCH 02/12] conf: Use parameter instead of global in conf_nat()

[Stefano Brivio]

On Fri, 26 Jun 2026 17:09:53 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Conf nat takes a parameter @arg for the argument it's parsing.  However on

Nit: conf_nat().

> error we print instead optarg, the getopt() global.  This happens to be the
> same thing at the time of the call, but it's not the right way to get to
> it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/conf.c b/conf.c
> index 4755a9f4..6ab8efec 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1049,7 +1049,7 @@ static void conf_nat(const char *arg, struct in_addr *addr4,
>  	    !IN4_IS_ADDR_MULTICAST(addr4))
>  		return;
>  
> -	die("Invalid address to remap to host: %s", optarg);
> +	die("Invalid address to remap to host: %s", arg);
>  }
>  
>  /**

-- 
Stefano

Re: [PATCH 03/12] parse: Start splitting out parsing helpers

[Stefano Brivio]

On Fri, 26 Jun 2026 17:09:54 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> As we add more complexity to what forwarding rules are allowed, our
> existing ad-hoc C parsing is starting to become quite awkward.  We already
> have some parts that resemble a very simple recursive[0] descent parser,
> with composable subfunctions that consume as much input as they need,
> rather than pre-splitting based on known delimiters.
> 
> Start extending this approach, by creating parse.[ch] with parsing helpers
> with a uniform interface.  Initially we start with very simple cases: the
> parse_keyword() function from fwd_rule.c and another helper to check that
> we've reached the end of input.
> 
> Rename parse_keyword() to parse_literal(), because it's not just useful
> for "keywords" as such.  We can use it in a bunch of additional places for
> parsing delimiters and other symbols.  This doesn't gain us a lot now but
> will make things clearer as we use more such parser helpers.
> 
> [0] Except that the grammars we have aren't actually recursive, so neither
>     is the code.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  Makefile   | 17 ++++++++------
>  conf.c     |  5 +++-
>  fwd_rule.c | 33 +++++----------------------
>  parse.c    | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  parse.h    | 14 ++++++++++++
>  5 files changed, 101 insertions(+), 35 deletions(-)
>  create mode 100644 parse.c
>  create mode 100644 parse.h
> 
> diff --git a/Makefile b/Makefile
> index 5ed0f702..bd729c75 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -36,12 +36,13 @@ BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security
>  PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
>  	epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \
>  	isolation.c lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c \
> -	passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \
> +	parse.c passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \
>  	tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \
>  	vhost_user.c virtio.c vu_common.c
>  QRAP_SRCS = qrap.c
>  PASST_REPAIR_SRCS = passt-repair.c
> -PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c serialise.c
> +PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c parse.c \
> +	serialise.c
>  SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
>  
>  MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
> @@ -49,13 +50,14 @@ MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
>  PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
>  	epoll_ctl.h flow.h fwd.h fwd_rule.h flow_table.h icmp.h icmp_flow.h \
>  	inany.h iov.h ip.h isolation.h lineread.h log.h migrate.h ndp.h \
> -	netlink.h packet.h passt.h pasta.h pcap.h pif.h repair.h serialise.h \
> -	siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \
> -	tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \
> -	virtio.h vu_common.h
> +	netlink.h packet.h parse.h passt.h pasta.h pcap.h pif.h repair.h \
> +	serialise.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \
> +	tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \
> +	vhost_user.h virtio.h vu_common.h
>  QRAP_HEADERS = arp.h ip.h passt.h util.h
>  PASST_REPAIR_HEADERS = linux_dep.h
> -PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h
> +PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h parse.h \
> +	pesto.h serialise.h
>  
>  C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
>  ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> @@ -223,6 +225,7 @@ 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:parse.c
>  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
>  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c
>  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> diff --git a/conf.c b/conf.c
> index 6ab8efec..ca6c859c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -52,6 +52,7 @@
>  #include "conf.h"
>  #include "pesto.h"
>  #include "serialise.h"
> +#include "parse.h"
>  
>  #define NETNS_RUN_DIR	"/run/netns"
>  
> @@ -1028,7 +1029,9 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  static void conf_nat(const char *arg, struct in_addr *addr4,
>  		     struct in6_addr *addr6, int *no_map_gw)
>  {
> -	if (strcmp(arg, "none") == 0) {
> +	const char *p = arg;
> +
> +	if (parse_literal(&p, "none") && parse_eoi(p)) {

I think that !*p would be so much clearer here compared to:

- parse_eoi(p)... must be "input"?

- [takes a look at parse_eoi()] yes, it's input, great [goes back to
  here]. Ah, wait, does it return false if *p is NULL? Or true?

- [takes a second look at parse_eoi()] good, it returns true, so this
  is *!p...

I understand it's somewhat consistent with parse_literal() this way, so
I'm not strongly against it, just a slight preference overall.

>  		*addr4 = in4addr_any;
>  		*addr6 = in6addr_any;
>  		if (no_map_gw)
> diff --git a/fwd_rule.c b/fwd_rule.c
> index 04a0101d..d0e90402 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -24,6 +24,7 @@
>  #include "fwd_rule.h"
>  #include "lineread.h"
>  #include "log.h"
> +#include "parse.h"
>  #include "serialise.h"
>  
>  /* Ephemeral port range: values from RFC 6335 */
> @@ -427,28 +428,6 @@ static int parse_port_range(const char *s, const char **endptr,
>  	return 0;
>  }
>  
> -/**
> - * parse_keyword() - Parse a literal keyword
> - * @s:		String to parse
> - * @endptr:	Update to the character after the keyword
> - * @kw:		Keyword to accept
> - *
> - * Return: 0, if @s starts with @kw, -EINVAL if it does not
> - */
> -static int parse_keyword(const char *s, const char **endptr, const char *kw)
> -{
> -	size_t len = strlen(kw);
> -
> -	if (strlen(s) < len)
> -		return -EINVAL;
> -
> -	if (memcmp(s, kw, len))
> -		return -EINVAL;
> -
> -	*endptr = s + len;
> -	return 0;
> -}
> -
>  /**
>   * fwd_rule_range_except() - Set up forwarding for a range of ports minus a
>   *                           bitmap of exclusions
> @@ -568,7 +547,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
>  			continue;
>  		}
>  
> -		if (parse_keyword(p, &p, "auto") == 0) {
> +		if (parse_literal(&p, "auto")) {
>  			if (p != ep) /* Garbage after the keyword */
>  				goto bad;
>  
> @@ -582,9 +561,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
>  		}
>  
>  		/* Should be an exclude range */
> -		if (*p != '~')
> +		if (!parse_literal(&p, "~"))
>  			goto bad;
> -		p++;
>  
>  		if (parse_port_range(p, &p, &xrange))
>  			goto bad;
> @@ -616,8 +594,9 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
>  		if (parse_port_range(p, &p, &orig_range))
>  			goto bad;
>  
> -		if (*p == ':') { /* There's a range to map to as well */
> -			if (parse_port_range(p + 1, &p, &mapped_range))
> +		if (parse_literal(&p, ":")) {
> +			/* There's a range to map to as well */
> +			if (parse_port_range(p, &p, &mapped_range))
>  				goto bad;
>  			if ((mapped_range.last - mapped_range.first) !=
>  			    (orig_range.last - orig_range.first))
> diff --git a/parse.c b/parse.c
> new file mode 100644
> index 00000000..c2a92422
> --- /dev/null
> +++ b/parse.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/* PASST - Plug A Simple Socket Transport
> + *  for qemu/UNIX domain socket mode
> + *
> + * PASTA - Pack A Subtle Tap Abstraction
> + *  for network namespace/tap device mode
> + *
> + * parse.c - Composable parsing helpers
> + *
> + * Copyright Red Hat
> + * Author: David Gibson <david@gibson.dropbear.id.au>
> + */
> +
> +#include <assert.h>
> +#include <stddef.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +
> +#include "common.h"
> +
> +/**
> + * DOC: Theory of Operation
> + *
> + * These are a number of primitives which can be combined into parsers for
> + * moderately complex input.  For simpler composability, they have common
> + * conventions.
> + *
> + * - Functions return a bool indicating whether they successfully parsed.

As I reached 7/12 (converting parse_port_range()), this became rather
confusing.

It makes sense for parse_eoi() (because in some sense it's a
parse_is_eoi() function), but for the rest I'd expect almost anything
that doesn't have "is" or similar in its name to return 0 on success,
and a negative error code on failure (or -1).

I'm not sure if I'm ignoring some particular advantage for the
composability you mention above, if it's clearly better for whatever
reason so be it, but otherwise I'd suggest re-considering this.

I mean:

-		if (parse_port_range(p, &p, &xrange))
+		if (!parse_port_range(&p, &xrange))

...is quite telling.

> + * - Any specific output from the parse is as output parameters
> + * - First argument is always @cursor, a const char **.
> + * - On entry, *@cursor has the point to start parsing.
> + * - On successful exit, *@cursor is updated to the next character after the
> +     parsed portion of the input
> + * - On failure, *@cursor and any output arguments are not modified
> + *
> + * For brevity the common parameters and return information are omitted from the
> + * individual function documentation comments.

The reason why we have "@c:	Execution context" all over the
place isn't so much human readability, it's rather automated usage, and
the day we manage to run Sphinx on the whole codebase:

  https://bugs.passt.top/show_bug.cgi?id=35

it would be nice to have a clean output instead of having to go through
a pile of warnings.

So, actually, I'd keep @cursor "described" everywhere. We can also skip
it for the moment but I'm afraid we'll start following this example and
it will be more work later.

> + */
> +
> +/**
> + * parse_literal() - Parse a specified literal string
> + * @lit:	Keyword to accept
> + */
> +bool parse_literal(const char **cursor, const char *lit)
> +{
> +	size_t len = strlen(lit);
> +
> +	if (strlen(*cursor) < len || memcmp(*cursor, lit, len))
> +		return false;
> +
> +	*cursor += len;
> +	return true;
> +}
> +
> +/**
> + * parse_eoi() - Parse end of input
> + * @cursor:	Current parse pointer
> + *
> + * Return: true if @p is at End of Input (\0), false otherwise
> + */
> +bool parse_eoi(const char *cursor)
> +{
> +	return !(*cursor);
> +}
> diff --git a/parse.h b/parse.h
> new file mode 100644
> index 00000000..eb51a469
> --- /dev/null
> +++ b/parse.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright Red Hat
> + * Author: David Gibson <david@gibson.dropbear.id.au>
> + */
> +
> +#ifndef PARSE_H
> +#define PARSE_H
> +
> +#include <stdbool.h>
> +
> +bool parse_literal(const char **cursor, const char *lit);
> +bool parse_eoi(const char *cursor);
> +
> +#endif /* _PARSE_H */

-- 
Stefano

Re: [PATCH 08/12] parse: Add helpers for parsing IP addresses

[Stefano Brivio]

On Fri, 26 Jun 2026 17:09:59 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> parse_ipv[46]() are wrappers around inet_pton() that are more
> convenient for use when the IP is part of a longer string, rather than
> the entire string.  parse_inany() replaces inany_pton() which again
> will become more convenient for strings including IPs that aren't just
> an IP.
> 
> For now we only have some simple and sometimes awkward use cases,
> we'll replace these with more natural uses in future.
> 
> Cc: Jon Maloy <jamloy@redhat.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  Makefile   |  1 +
>  conf.c     |  6 ++--
>  fwd_rule.c | 21 ++++---------
>  inany.c    | 24 ++------------
>  inany.h    |  1 -
>  parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  parse.h    |  4 +++
>  7 files changed, 109 insertions(+), 39 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 85d279c0..e2b22ddf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -227,4 +227,5 @@ 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
> +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
>  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> diff --git a/conf.c b/conf.c
> index bd357117..2d1895d4 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
>  	struct in_addr mask;
>  	unsigned long len;
>  
> -	if (inet_pton(AF_INET, arg, &mask)) {
> +	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
>  		in_addr_t hmask = ntohl(mask.s_addr);
>  		len = __builtin_popcount(hmask);
>  		if ((hmask << len) == 0)
> @@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  			if (addr_has_prefix_len && prefix_len_from_opt)
>  				die("Redundant prefix length specification");
>  
> -			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
> +			p = optarg;
> +			if (!addr_has_prefix_len &&
> +			    !(parse_inany(&p, &addr) && parse_eoi(p)))

parse_port_range() reports error if the input isn't NULL-terminated,
but these new functions don't, and in all the usages at least
introduced in this patch, you always need to couple that with a &&
parse_eoi(p) (or && !*p).

Is there a reason for this difference?

>  				die("Invalid address: %s", optarg);
>  
>  			if (prefix_len_from_opt && inany_v4(&addr))
> diff --git a/fwd_rule.c b/fwd_rule.c
> index 0b85b17e..6d7ec2c5 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
>  	strncpy(buf, optarg, sizeof(buf) - 1);
>  
>  	if ((spec = strchr(buf, '/'))) {
> +		const char *p = buf;
> +
>  		*spec = 0;
>  		spec++;
>  
> @@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
>  			}
>  		}
>  
> -		if (ifname == buf + 1) {	/* Interface without address */
> +		if (ifname == buf + 1 || /* Interface without address */
> +		    !strcmp(buf, "*")) /* Explicit wildcard address */
>  			addr = NULL;
> -		} else {
> -			char *p = buf;
> -
> -			/* Allow square brackets for IPv4 too for convenience */
> -			if (*p == '[' && p[strlen(p) - 1] == ']') {
> -				p[strlen(p) - 1] = '\0';
> -				p++;
> -			}
> -
> -			if (strcmp(p, "*") == 0)
> -				addr = NULL;
> -			else if (!inany_pton(p, &addr_buf))
> -				die("Bad forwarding address '%s'", p);
> -		}
> +		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> +			die("Bad forwarding address '%s'", buf);
>  	} else {
>  		spec = buf;
>  
> diff --git a/inany.c b/inany.c
> index 23faf3ff..154f08b5 100644
> --- a/inany.c
> +++ b/inany.c
> @@ -27,6 +27,7 @@
>  #include "ip.h"
>  #include "inany.h"
>  #include "fwd.h"
> +#include "parse.h"
>  
>  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
>  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> @@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
>  	return inet_ntop(AF_INET6, &src->a6, dst, size);
>  }
>  
> -/** inany_pton - Parse an IPv[46] address from text format
> - * @src:	IPv[46] address
> - * @dst:	output buffer, filled with parsed address
> - *
> - * Return: on success, 1, if no parseable address is found, 0
> - */
> -int inany_pton(const char *src, union inany_addr *dst)
> -{
> -	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
> -		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
> -		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
> -		return 1;
> -	}
> -
> -	if (inet_pton(AF_INET6, src, &dst->a6))
> -		return 1;
> -
> -	return 0;
> -}
> -
>  /**
>   * inany_prefix_pton() - Parse an IPv[46] address with prefix length
>   * @src:	IPv[46] address and prefix length string in CIDR format
> @@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
>  	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;
>  
> @@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
>  		return 1;
>  	}
>  
> -	if (inany_pton(astr, dst)) {
> +	if (parse_inany(&p, dst) && parse_eoi(p)) {
>  		if (plen > 32)
>  			return 0;
>  		*prefix_len = plen + 96;
> diff --git a/inany.h b/inany.h
> index 6bf3ecaa..93d98368 100644
> --- a/inany.h
> +++ b/inany.h
> @@ -303,7 +303,6 @@ 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_pton(const char *src, union inany_addr *dst);
>  int inany_prefix_pton(const char *src, union inany_addr *dst,
>  		      uint8_t *prefix_len);
>  
> diff --git a/parse.c b/parse.c
> index bd091fde..0349c5dc 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -16,9 +16,12 @@
>  #include <stddef.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <limits.h>
> +#include <arpa/inet.h>
>  
>  #include "common.h"
>  #include "parse.h"
> +#include "inany.h"
>  
>  /**
>   * DOC: Theory of Operation
> @@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
>  	*cursor = p;
>  	return true;
>  }
> +
> +/**
> + * parse_ipv4() - Parse an IPv4 address from a string
> + * @abuf:	On success, updated with parsed address
> + */
> +bool parse_ipv4(const char **cursor, struct in_addr *abuf)
> +{
> +	/* Brackets are not typical on IPv4, but allow for consistency */
> +	const char *p = *cursor;
> +	bool bracket = parse_literal(&p, "[");
> +	char buf[INET_ADDRSTRLEN];
> +	struct in_addr addr;
> +	size_t len;
> +
> +	if (bracket)
> +		len = strcspn(p, "]");
> +	else
> +		len = strspn(p, "0123456789.");
> +
> +	if (len >= sizeof(buf))
> +		return false;
> +	memcpy(buf, p, len);
> +	buf[len] = '\0';
> +	p += len;
> +
> +	if (!inet_pton(AF_INET, buf, &addr))
> +		return false;
> +
> +	if (bracket && !parse_literal(&p, "]"))
> +		return false;
> +
> +	*cursor = p;
> +	*abuf = addr;
> +	return true;
> +}
> +
> +/**
> + * parse_ipv6() - Parse an IPv6 address from a string
> + * @abuf:	On success, updated with parsed address
> + */
> +static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> +{
> +	const char *p = *cursor;
> +	bool bracket = parse_literal(&p, "[");
> +	char buf[INET6_ADDRSTRLEN];
> +	struct in6_addr addr;
> +	size_t len;
> +
> +	if (bracket)
> +		len = strcspn(p, "]");
> +	else
> +		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
> +
> +	if (len >= sizeof(buf))
> +		return false;
> +	memcpy(buf, p, len);
> +	buf[len] = '\0';
> +	p += len;
> +
> +	if (!inet_pton(AF_INET6, buf, &addr))
> +		return false;
> +
> +	if (bracket && !parse_literal(&p, "]"))
> +		return false;
> +
> +	*cursor = p;
> +	*abuf = addr;
> +	return true;
> +}
> +
> +/**
> + * parse_inany() - Parse an IPv4 or IPv6 address from a string
> + * @addr:	On success, updated with parsed address
> + */
> +bool parse_inany(const char **cursor, union inany_addr *addr)
> +{
> +	struct in_addr a4;
> +
> +	if (parse_ipv6(cursor, &addr->a6))
> +		return true;
> +
> +	if (parse_ipv4(cursor, &a4)) {
> +		*addr = inany_from_v4(a4);
> +		return true;
> +	}
> +
> +	return false;
> +}
> diff --git a/parse.h b/parse.h
> index 155b3995..2820a065 100644
> --- a/parse.h
> +++ b/parse.h
> @@ -9,6 +9,8 @@
>  #include <stdbool.h>
>  #include <netinet/in.h>
>  
> +union inany_addr;
> +
>  /**
>   * port_range() - Represents a non-empty range of ports
>   * @first:	First port number in the range
> @@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
>  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);
>  
>  #endif /* _PARSE_H */

-- 
Stefano

Re: [PATCH 09/12] conf: Move address configuration into helper function

[Stefano Brivio]

On Fri, 26 Jun 2026 17:10:00 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> The handling of the -a option is getting complex enough that it's pretty
> bulky inside it's switch label.  Move it into a new conf_addr() function.

Nit if you respin: 'its'.

> We also rename the bulky addr_has_prefix_len and prefix_len_from_opt
> variables to the terser 'opt_a_is_prefix' and 'opt_n', since they are
> specifically about those command line options.
> 
> Cc: Jon Maloy <jmaloy@redhat.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c | 96 ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 53 insertions(+), 43 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 2d1895d4..3614776c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1168,6 +1168,53 @@ int conf_tap_fd(const char *arg)
>  	return val;
>  }
>  
> +/**
> + * conf_addr() - Configure guest address with -a option
> + * @c:		Execution context
> + * @arg:	-a command line argument
> + * @opt_n:	Value from -n option, if any
> + */
> +static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n)
> +{
> +	union inany_addr addr;
> +	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 (inany_is_unspecified(&addr) || inany_is_multicast(&addr) ||
> +	    inany_is_loopback(&addr) || IN6_IS_ADDR_V4COMPAT(&addr.a6))
> +		die("Invalid address: %s", arg);
> +
> +	if (inany_v4(&addr)) {
> +		c->ip4.addr = *inany_v4(&addr);
> +		c->ip4.prefix_len = prefix_len - 96;
> +		c->ip4.addr_fixed = true;
> +		if (c->mode == MODE_PASTA)
> +			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;
> +	}
> +
> +	return is_prefix;
> +}
> +
>  /**
>   * conf() - Process command-line arguments and set configuration
>   * @c:		Execution context
> @@ -1262,12 +1309,12 @@ void conf(struct ctx *c, int argc, char **argv)
>  	unsigned dns4_idx = 0, dns6_idx = 0;
>  	unsigned long max_mtu = IP_MAX_MTU;
>  	struct fqdn *dnss = c->dns_search;
> -	bool addr_has_prefix_len = false;
> -	uint8_t prefix_len_from_opt = 0;
>  	unsigned int ifi4 = 0, ifi6 = 0;
> +	bool opt_a_is_prefix = false;
>  	const char *logfile = NULL;
>  	char *runas = NULL;
>  	size_t logsize = 0;
> +	uint8_t opt_n = 0;
>  	int name, ret;
>  	uid_t uid;
>  	gid_t gid;
> @@ -1567,52 +1614,15 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->mtu = mtu;
>  			break;
>  		}
> -		case 'a': {
> -			union inany_addr addr;
> -			uint8_t prefix_len;
> -
> -			addr_has_prefix_len = inany_prefix_pton(optarg, &addr,
> -								&prefix_len);
> -
> -			if (addr_has_prefix_len && prefix_len_from_opt)
> -				die("Redundant prefix length specification");
> -
> -			p = optarg;
> -			if (!addr_has_prefix_len &&
> -			    !(parse_inany(&p, &addr) && parse_eoi(p)))
> -				die("Invalid address: %s", optarg);
> -
> -			if (prefix_len_from_opt && inany_v4(&addr))
> -				prefix_len = prefix_len_from_opt;
> -			else if (!addr_has_prefix_len)
> -				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", optarg);
> -
> -			if (inany_v4(&addr)) {
> -				c->ip4.addr = *inany_v4(&addr);
> -				c->ip4.prefix_len = prefix_len - 96;
> -				c->ip4.addr_fixed = true;
> -				if (c->mode == MODE_PASTA)
> -					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;
> -			}
> +		case 'a':
> +			opt_a_is_prefix = conf_addr(c, optarg, opt_n);
>  			break;
> -		}
>  		case 'n':
> -			if (addr_has_prefix_len)
> +			if (opt_a_is_prefix)
>  				die("Redundant prefix length specification");
>  
>  			c->ip4.prefix_len = conf_ip4_prefix(optarg);
> -			prefix_len_from_opt = c->ip4.prefix_len + 96;
> +			opt_n = c->ip4.prefix_len + 96;
>  			break;
>  		case 'M':
>  			parse_mac(c->our_tap_mac, optarg);

-- 
Stefano

Re: [PATCH 10/12] conf: Use new parsing tools to handle -a option

[Stefano Brivio]

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).

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).

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

> +		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).

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

Re: [PATCH 11/12] fwd_rule: Allow "all" port specs to be combined with other options

[Stefano Brivio]

On Fri, 26 Jun 2026 17:10:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we handle -t all and the like as a special case, it can't be
> combined with other port specifier options.  Remove that restriction,
> allowing combined options like:
>      -t all,~9999          # Forward everything non-ephemeral except 9999
>      -t all,auto           # Equivalent to -t auto
>      -t all,33000          # Forward non-ephemeral plus port 33,000
> 
> This isn't particularly useful immediately,

Actually, -t all,33000 could be useful right away. I didn't realise we
weren't allowing that.

-- 
Stefano

Re: [PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers

[Stefano Brivio]

On Fri, 26 Jun 2026 17:10:03 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Forwarding rules for the -[tTuU] options are parsed in fwd_rule_parse()
> and fwd_rule_parse_ports().  Currently those use a mix of loosely
> recursive descent parsing helpers, consuming input from left to right,
> and ad-hoc C parsing - searching for delimeters with strchr() or the like

Nit if you respin: delimiters.

> and breaking down on that basis.
> 
> As well as being a slightly awkward mix, the current approach will not work
> for adding target addresses to the rules: we can't easily split the
> listening from target parts first, because the ':' delimiter could appear
> multiple times for multiple port ranges, or in IPv6 addresses.  However,
> without splitting the target part first, we can't first split off the
> listening address and interface because the '/' delimiter could appear in
> the target portion, but not the listening portion, e.g.:
>     -t 12345:192.0.1.1/12345
> 
> To address this, rewrite the entirety of the parsing so we consume the
> input left to right in a more-or-less recursive descent manner.  This means
> we no longer rely on certain delimiters having a single meaning over the
> whole input, just the next part of it.
> 
> Because of the semantics of port specs, we need to make several passes
> over the list of comma separated chunks.  Previously, some of the logic was
> duplicated between the two passes, making it fragile.  Now, we introduce
> a parse_port_chunk() helper which we re-use in each pass to make it clearer
> we parse exactly the same way on each pass.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  fwd_rule.c | 262 ++++++++++++++++++++++++++++++-----------------------
>  parse.c    |  29 ++++++
>  parse.h    |   2 +
>  3 files changed, 181 insertions(+), 112 deletions(-)
> 
> diff --git a/fwd_rule.c b/fwd_rule.c
> index b14df340..8ae21b99 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -439,17 +439,96 @@ fail:
>  	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
>  }
>  
> -/*
> - * for_each_chunk - Step through delimited chunks of a string
> - * @p_:		Pointer to start of each chunk (updated)
> - * @ep_:	Pointer to end of each chunk (updated)
> - * @s_:		String to step through
> - * @sep_:	String of all allowed delimiters
> +/**
> + * enum fwd_port_chunk_kind - Kind of port specifier piece

Nit: I think we should use kerneldoc-style comments also for enum
(see enum udp_iov_idx, udp.c for an example).

> + */
> +enum fwd_port_chunk_kind {
> +	CHUNK_ALL,		/* "all" */
> +	CHUNK_AUTO,		/* "auto" */
> +	CHUNK_EXCLUDE,		/* "~1111-2222" */
> +	CHUNK_INCLUDE,		/* "1111-2222" */
> +};
> +
> +/**
> + * parse_port_chunk() - Parse one chunk of a port specifier
> + * @cursor:	Parsing point (see parse.c)
> + * @kindp:	Updated with kind of chunk we parsed
> + * @lrange:	Updated with listening port range (for INCLUDE & EXCLUDE)
> + * @trange:	Updated with target port range (for EXCLUDE)
> + */
> +static bool parse_port_chunk(const char **cursor, enum fwd_port_chunk_kind *kindp,
> +			     struct port_range *lrange, struct port_range *trange)

Nit: this exceeds 80 columns.

I'm wondering whether this doesn't rather belong in parse.c: having it
here makes it hard to spot what it returns (the "Theory of Operation"
comment from parse.c), and anyway it's pure parsing, nothing else.

> +{
> +	struct port_range lr = { 0 }, tr = { 0 };
> +	enum fwd_port_chunk_kind kind;
> +	const char *p = *cursor;
> +
> +	if (parse_literal(&p, "all")) {
> +		kind = CHUNK_ALL;
> +	} else if (parse_literal(&p, "auto")) {
> +		kind = CHUNK_AUTO;
> +	} else if (parse_literal(&p, "~")) {
> +		kind = CHUNK_EXCLUDE;
> +		if (!parse_port_range(&p, &lr))
> +			return false;
> +	} else if (parse_port_range(&p, &lr)) {
> +		kind = CHUNK_INCLUDE;
> +
> +		if (parse_literal(&p, ":")) {
> +			if (!parse_port_range(&p, &tr))
> +				return false;
> +		} else {
> +			tr = lr;
> +		}
> +	} else {
> +		return false;
> +	}
> +
> +	*kindp = kind;
> +	*lrange = lr;
> +	if (trange)
> +		*trange = tr;
> +	*cursor = p;

I wonder if it's worth it being rigid about avoiding side effects on
failure, in these functions. In general I agree it's desirable, but in
practice we don't need it here and it's quite a few extra lines with
slightly confusing variable names.

> +	return true;
> +}
> +
> +/**
> + * parse_laddrif() - Parse listening address+ifname

This makes it look like + is a separator, maybe "address%ifname"?

The name isn't really descriptive I think. Maybe we could take for
granted that it's about the listening part, and call it
parse_addrifname()? Or parse_addr_ifname()?

> + * @cursor:	Parsing cursor (see parse.c)
> + * @addr:	Updated with parsed inany address (NULL for *)
> + * @abuf:	Buffer to store address
> + * @ifname:	Updated with parsed interface name ("" if none)
>   */
> -#define for_each_chunk(p_, ep_, s_, sep_)			\
> -	for ((p_) = (s_);					\
> -	     (ep_) = (p_) + strcspn((p_), (sep_)), *(p_);	\
> -	     (p_) = *(ep_) ? (ep_) + 1 : (ep_))
> +static bool parse_laddrif(const char **cursor,
> +			  const union inany_addr **addr,
> +			  union inany_addr *abuf,
> +			  char *ifname)
> +{
> +	union inany_addr atmp = inany_any6;
> +	char iftmp[IFNAMSIZ] = {0};
> +	const char *p;
> +
> +	if (p = *cursor,

The advantage of using the comma operator isn't really clear to me
here. Can't we just do this assignment unconditionally as
initialisation?

By the way, while we don't adhere to it, I think MISRA C contains many
guidelines that are actually useful (maybe more than half of them?), and
MISRA C:2012 forbids this kind of usage because of readability issues:

  https://www.mathworks.com/help/bugfinder/ref/misrac2012rule12.3.html

> +	    parse_inany(&p, &atmp)		&&
> +	    parse_ifspec(&p, iftmp)		&&
> +	    parse_literal(&p, "/")) {
> +		/* Specific listening address */
> +		*addr = abuf;
> +	} else if (p = *cursor,
> +		   parse_literal(&p, "*"),
> +		   parse_ifspec(&p, iftmp)	&&
> +		   parse_literal(&p, "/")) {
> +		/* Missing or "*" address */
> +		*addr = NULL;
> +	} else {
> +		return false;
> +	}
> +
> +	*abuf = atmp;
> +	memcpy(ifname, iftmp, IFNAMSIZ);
> +	*cursor = p;
> +	return true;
> +}
>  
>  /**
>   * fwd_rule_parse_ports() - Parse port range(s) specifier
> @@ -466,87 +545,70 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
>  				 const char *spec)
>  {
>  	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
> +	enum fwd_port_chunk_kind kind;
> +	struct port_range lrange;
>  	bool exclude_only = true;
> -	const char *p, *ep;
>  	uint8_t flags = 0;
> +	const char *p;
>  	unsigned i;
>  
> -	/* Parse excluded ranges and "auto" in the first pass */
> -	for_each_chunk(p, ep, spec, ",") {
> -		struct port_range xrange;
> -
> -		/* Include range, parse later */
> -		if (parse_literal(&p, "all") || isdigit(*p))
> -			continue;
> -
> -		if (parse_literal(&p, "auto")) {
> -			if (p != ep) /* Garbage after the keyword */
> -				goto bad;
> +	/* Consider excluded ranges and "auto" in the first pass */
> +	p = spec;
> +	do {
> +		if (!parse_port_chunk(&p, &kind, &lrange, NULL))
> +			goto bad;
>  
> +		switch (kind) {
> +		case CHUNK_AUTO:
>  			if (!(fwd->caps & FWD_CAP_SCAN)) {
>  				die(
>  "'auto' port forwarding is only allowed for pasta");
>  			}
> -
>  			flags |= FWD_SCAN;
> -			continue;
> -		}
> -
> -		/* Should be an exclude range */
> -		if (!parse_literal(&p, "~"))
> -			goto bad;
> -
> -		if (!parse_port_range(&p, &xrange))
> -			goto bad;
> -		if (p != ep) /* Garbage after the range */
> -			goto bad;
> -
> -		for (i = xrange.first; i <= xrange.last; i++)
> -			bitmap_set(exclude, i);
> -	}
> -
> -	/* Now process base ranges, skipping exclusions */
> -	for_each_chunk(p, ep, spec, ",") {
> -		struct port_range orig_range, mapped_range;
> -
> -		/* Handle "all" like exclude only */
> -		if (parse_literal(&p, "all")) {
> -			if (p != ep) /* Garbage after the keyword */
> -				goto bad;
> +			break;
>  
> -			continue;
> +		case CHUNK_EXCLUDE:
> +			for (i = lrange.first; i <= lrange.last; i++)
> +				bitmap_set(exclude, i);
> +			break;
> +		default:
> +			; /* Handled later */
>  		}
> +	} while (parse_literal(&p, ","));
>  
> -		if (!isdigit(*p))
> -			/* Already parsed */
> -			continue;
> +	/* Consider included ranges in next pass */
> +	p = spec;
> +	do {
> +		struct port_range trange;
>  
> -		if (!parse_port_range(&p, &orig_range))
> +		if (!parse_port_chunk(&p, &kind, &lrange, &trange))
>  			goto bad;
>  
> -		exclude_only = false;
> +		switch (kind) {
> +		case CHUNK_AUTO:	/* already handled */
> +		case CHUNK_EXCLUDE:	/* already handled */
> +		case CHUNK_ALL:		/* handled later */
> +			continue;
>  
> -		if (parse_literal(&p, ":")) {
> -			/* There's a range to map to as well */
> -			if (!parse_port_range(&p, &mapped_range))
> -				goto bad;
> -			if ((mapped_range.last - mapped_range.first) !=
> -			    (orig_range.last - orig_range.first))
> +		case CHUNK_INCLUDE:
> +			exclude_only = false;
> +			if (trange.last - trange.first !=
> +			    lrange.last - lrange.first)
>  				goto bad;
> -		} else {
> -			mapped_range = orig_range;
> -		}
>  
> -		if (p != ep) /* Garbage after the ranges */
> +			fwd_rule_range_except(fwd, del, proto, addr, ifname,
> +					      lrange.first, lrange.last,
> +					      exclude, trange.first, flags);
> +			break;
> +		default:
>  			goto bad;
> +		}
> +	} while (parse_literal(&p, ","));
>  
> -		fwd_rule_range_except(fwd, del, proto, addr, ifname,
> -				      orig_range.first, orig_range.last,
> -				      exclude,
> -				      mapped_range.first, flags);
> -	}
> +	if (!parse_eoi(p))
> +		goto bad; /* trailing garbage */
>  
> -	/* Finally handle "all" and exclude only specs */
> +	/* Finally handle "all" and exclude only cases */
>  	if (exclude_only) {
>  		fwd_port_map_ephemeral(exclude);
>  
> @@ -569,10 +631,11 @@ bad:
>  void fwd_rule_parse(char optname, bool del, const char *optarg,
>  		    struct fwd_table *fwd)
>  {
> -	char buf[BUFSIZ], *spec, *ifname = NULL;
> -	union inany_addr addr_buf = inany_any6;
> -	const union inany_addr *addr = &addr_buf;
> +	const union inany_addr *addr;
> +	union inany_addr addr_buf;
> +	char ifname[IFNAMSIZ];
>  	uint8_t proto;
> +	const char *p;
>  
>  	if (optname == 't' || optname == 'T')
>  		proto = IPPROTO_TCP;
> @@ -581,7 +644,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
>  	else
>  		assert(0);
>  
> -	if (!strcmp(optarg, "none")) {
> +	if (p = optarg,

Same as above: do we really need this? It's local anyway.

> +	    parse_literal(&p, "none") && parse_eoi(p)) {
>  		unsigned i;
>  
>  		for (i = 0; i < fwd->count; i++) {
> @@ -593,45 +657,19 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
>  		return;
>  	}
>  
> -	strncpy(buf, optarg, sizeof(buf) - 1);
> -
> -	if ((spec = strchr(buf, '/'))) {
> -		const char *p = buf;
> -
> -		*spec = 0;
> -		spec++;
> -
> -		if (optname != 't' && optname != 'u')
> +	if (p = optarg,
> +	    parse_laddrif(&p, &addr, &addr_buf, ifname)) {
> +		if (optname == 'T' || optname == 'U')
>  			die("Listening address not allowed for -%c %s",
>  			    optname, optarg);
> -
> -		if ((ifname = strchr(buf, '%'))) {
> -			*ifname = 0;
> -			ifname++;
> -
> -			/* spec is already advanced one past the '/',
> -			 * so the length of the given ifname is:
> -			 * (spec - ifname - 1)
> -			 */
> -			if (spec - ifname - 1 >= IFNAMSIZ) {
> -				die("Interface name '%s' is too long (max %u)",
> -				    ifname, IFNAMSIZ - 1);
> -			}
> -		}
> -
> -		if (ifname == buf + 1 || /* Interface without address */
> -		    !strcmp(buf, "*")) /* Explicit wildcard address */
> -			addr = NULL;
> -		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> -			die("Bad forwarding address '%s'", buf);
>  	} else {
> -		spec = buf;
> -
> +		/* No address or ifname */
>  		addr = NULL;
> +		ifname[0] = '\0';

Actually, here, having side effects from parse_laddrif() failing would
be rather convenient and intuitive.

>  	}
>  
>  	if (optname == 'T' || optname == 'U') {
> -		assert(!addr && !ifname);
> +		assert(!addr && !*ifname);
>  
>  		if (!(fwd->caps & FWD_CAP_IFNAME)) {
>  			warn(
> @@ -640,18 +678,18 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
>  
>  			if (fwd->caps & FWD_CAP_IPV4) {
>  				fwd_rule_parse_ports(fwd, del, proto,
> -						     &inany_loopback4, NULL,
> -						     spec);
> +						     &inany_loopback4, NULL, p);
>  			}
>  			if (fwd->caps & FWD_CAP_IPV6) {
>  				fwd_rule_parse_ports(fwd, del, proto,
> -						     &inany_loopback6, NULL,
> -						     spec);
> +						     &inany_loopback6, NULL, p);
>  			}
>  			return;
>  		}
>  
> -		ifname = "lo";
> +		static_assert(sizeof("lo") <= sizeof(ifname),
> +			      "ifname buffer too small");
> +		strcpy(ifname, "lo");
>  	}
>  
>  	/* No need for dual stack if we only have one IP version */
> @@ -660,13 +698,13 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
>  	else if (!addr && !(fwd->caps & FWD_CAP_IPV6))
>  		addr = &inany_any4;
>  
> -	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
> +	if (*ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
>  		die(
>  "Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
>  		    optname, optarg);
>  	}
>  
> -	fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec);
> +	fwd_rule_parse_ports(fwd, del, proto, addr, *ifname ? ifname : NULL, p);
>  }
>  
>  /**
> diff --git a/parse.c b/parse.c
> index 3e0dbd45..d83ea9f9 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -18,6 +18,7 @@
>  #include <string.h>
>  #include <limits.h>
>  #include <arpa/inet.h>
> +#include <net/if.h>
>  
>  #include "common.h"
>  #include "parse.h"
> @@ -212,3 +213,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
>  
>  	return false;
>  }
> +
> +/**
> + * parse_ifspec() - Parse a interface name specifier (starting with %)
> + * @ifname:	On success updated with parsed name (must have IFNAMSIZ space)
> + *
> + * This will accept a missing specifier (empty string), setting ifname to ""
> + */
> +bool parse_ifspec(const char **cursor, char *ifname)
> +{
> +	const char *p = *cursor;
> +	size_t len;
> +
> +	if (!parse_literal(&p, "%")) {
> +		/* No interface specifier */
> +		ifname[0] = '\0';
> +		return true;
> +	}
> +
> +	/* ifnames can have anything that's not '/' or whitespace */

dev_valid_name(), net/core/dev.c in the Linux kernel, also forbids '.'
and '..', to avoid breaking sysfs. We don't really _need_ to check for
that as we don't create interfaces, but strictly speaking the comment
isn't accurate, and we might want to filter for consistency anyway.

> +	len = strcspn(p, "/ \f\n\r\t\v");
> +	if (!len || len >= IFNAMSIZ)
> +		return false;
> +
> +	memcpy(ifname, p, len);
> +	ifname[len] = '\0';
> +	*cursor = p + len;
> +	return true;
> +}
> diff --git a/parse.h b/parse.h
> index 08b038cf..1079f5bf 100644
> --- a/parse.h
> +++ b/parse.h
> @@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
>  
>  #define parse_inany(cursor, addr)	parse_inany_((cursor), (addr), NULL)
>  
> +bool parse_ifspec(const char **cursor, char *ifname);
> +
>  #endif /* _PARSE_H */

Other than those comments, it all looks good to me, and also like an
obvious, much needed improvement. I tested it quite a bit and I didn't
manage to break things.

-- 
Stefano

[RESEND] Re: [PATCH 08/12] parse: Add helpers for parsing IP addresses

[Stefano Brivio]

[Resending to jmaloy@redhat.com, the original email had:

On Wed, 1 Jul 2026 02:07:25 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 26 Jun 2026 17:09:59 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > parse_ipv[46]() are wrappers around inet_pton() that are more
> > convenient for use when the IP is part of a longer string, rather than
> > the entire string.  parse_inany() replaces inany_pton() which again
> > will become more convenient for strings including IPs that aren't just
> > an IP.
> > 
> > For now we only have some simple and sometimes awkward use cases,
> > we'll replace these with more natural uses in future.
> > 
> > Cc: Jon Maloy <jamloy@redhat.com>

...jamloy@redhat.com instead.

> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile   |  1 +
> >  conf.c     |  6 ++--
> >  fwd_rule.c | 21 ++++---------
> >  inany.c    | 24 ++------------
> >  inany.h    |  1 -
> >  parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  parse.h    |  4 +++
> >  7 files changed, 109 insertions(+), 39 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 85d279c0..e2b22ddf 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -227,4 +227,5 @@ 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
> > +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
> >  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > diff --git a/conf.c b/conf.c
> > index bd357117..2d1895d4 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
> >  	struct in_addr mask;
> >  	unsigned long len;
> >  
> > -	if (inet_pton(AF_INET, arg, &mask)) {
> > +	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
> >  		in_addr_t hmask = ntohl(mask.s_addr);
> >  		len = __builtin_popcount(hmask);
> >  		if ((hmask << len) == 0)
> > @@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			if (addr_has_prefix_len && prefix_len_from_opt)
> >  				die("Redundant prefix length specification");
> >  
> > -			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
> > +			p = optarg;
> > +			if (!addr_has_prefix_len &&
> > +			    !(parse_inany(&p, &addr) && parse_eoi(p)))  
> 
> parse_port_range() reports error if the input isn't NULL-terminated,
> but these new functions don't, and in all the usages at least
> introduced in this patch, you always need to couple that with a &&
> parse_eoi(p) (or && !*p).
> 
> Is there a reason for this difference?
> 
> >  				die("Invalid address: %s", optarg);
> >  
> >  			if (prefix_len_from_opt && inany_v4(&addr))
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index 0b85b17e..6d7ec2c5 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  	strncpy(buf, optarg, sizeof(buf) - 1);
> >  
> >  	if ((spec = strchr(buf, '/'))) {
> > +		const char *p = buf;
> > +
> >  		*spec = 0;
> >  		spec++;
> >  
> > @@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  			}
> >  		}
> >  
> > -		if (ifname == buf + 1) {	/* Interface without address */
> > +		if (ifname == buf + 1 || /* Interface without address */
> > +		    !strcmp(buf, "*")) /* Explicit wildcard address */
> >  			addr = NULL;
> > -		} else {
> > -			char *p = buf;
> > -
> > -			/* Allow square brackets for IPv4 too for convenience */
> > -			if (*p == '[' && p[strlen(p) - 1] == ']') {
> > -				p[strlen(p) - 1] = '\0';
> > -				p++;
> > -			}
> > -
> > -			if (strcmp(p, "*") == 0)
> > -				addr = NULL;
> > -			else if (!inany_pton(p, &addr_buf))
> > -				die("Bad forwarding address '%s'", p);
> > -		}
> > +		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> > +			die("Bad forwarding address '%s'", buf);
> >  	} else {
> >  		spec = buf;
> >  
> > diff --git a/inany.c b/inany.c
> > index 23faf3ff..154f08b5 100644
> > --- a/inany.c
> > +++ b/inany.c
> > @@ -27,6 +27,7 @@
> >  #include "ip.h"
> >  #include "inany.h"
> >  #include "fwd.h"
> > +#include "parse.h"
> >  
> >  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
> >  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> > @@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
> >  	return inet_ntop(AF_INET6, &src->a6, dst, size);
> >  }
> >  
> > -/** inany_pton - Parse an IPv[46] address from text format
> > - * @src:	IPv[46] address
> > - * @dst:	output buffer, filled with parsed address
> > - *
> > - * Return: on success, 1, if no parseable address is found, 0
> > - */
> > -int inany_pton(const char *src, union inany_addr *dst)
> > -{
> > -	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
> > -		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
> > -		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
> > -		return 1;
> > -	}
> > -
> > -	if (inet_pton(AF_INET6, src, &dst->a6))
> > -		return 1;
> > -
> > -	return 0;
> > -}
> > -
> >  /**
> >   * inany_prefix_pton() - Parse an IPv[46] address with prefix length
> >   * @src:	IPv[46] address and prefix length string in CIDR format
> > @@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  	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;
> >  
> > @@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  		return 1;
> >  	}
> >  
> > -	if (inany_pton(astr, dst)) {
> > +	if (parse_inany(&p, dst) && parse_eoi(p)) {
> >  		if (plen > 32)
> >  			return 0;
> >  		*prefix_len = plen + 96;
> > diff --git a/inany.h b/inany.h
> > index 6bf3ecaa..93d98368 100644
> > --- a/inany.h
> > +++ b/inany.h
> > @@ -303,7 +303,6 @@ 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_pton(const char *src, union inany_addr *dst);
> >  int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  		      uint8_t *prefix_len);
> >  
> > diff --git a/parse.c b/parse.c
> > index bd091fde..0349c5dc 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -16,9 +16,12 @@
> >  #include <stddef.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <limits.h>
> > +#include <arpa/inet.h>
> >  
> >  #include "common.h"
> >  #include "parse.h"
> > +#include "inany.h"
> >  
> >  /**
> >   * DOC: Theory of Operation
> > @@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
> >  	*cursor = p;
> >  	return true;
> >  }
> > +
> > +/**
> > + * parse_ipv4() - Parse an IPv4 address from a string
> > + * @abuf:	On success, updated with parsed address
> > + */
> > +bool parse_ipv4(const char **cursor, struct in_addr *abuf)
> > +{
> > +	/* Brackets are not typical on IPv4, but allow for consistency */
> > +	const char *p = *cursor;
> > +	bool bracket = parse_literal(&p, "[");
> > +	char buf[INET_ADDRSTRLEN];
> > +	struct in_addr addr;
> > +	size_t len;
> > +
> > +	if (bracket)
> > +		len = strcspn(p, "]");
> > +	else
> > +		len = strspn(p, "0123456789.");
> > +
> > +	if (len >= sizeof(buf))
> > +		return false;
> > +	memcpy(buf, p, len);
> > +	buf[len] = '\0';
> > +	p += len;
> > +
> > +	if (!inet_pton(AF_INET, buf, &addr))
> > +		return false;
> > +
> > +	if (bracket && !parse_literal(&p, "]"))
> > +		return false;
> > +
> > +	*cursor = p;
> > +	*abuf = addr;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_ipv6() - Parse an IPv6 address from a string
> > + * @abuf:	On success, updated with parsed address
> > + */
> > +static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> > +{
> > +	const char *p = *cursor;
> > +	bool bracket = parse_literal(&p, "[");
> > +	char buf[INET6_ADDRSTRLEN];
> > +	struct in6_addr addr;
> > +	size_t len;
> > +
> > +	if (bracket)
> > +		len = strcspn(p, "]");
> > +	else
> > +		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
> > +
> > +	if (len >= sizeof(buf))
> > +		return false;
> > +	memcpy(buf, p, len);
> > +	buf[len] = '\0';
> > +	p += len;
> > +
> > +	if (!inet_pton(AF_INET6, buf, &addr))
> > +		return false;
> > +
> > +	if (bracket && !parse_literal(&p, "]"))
> > +		return false;
> > +
> > +	*cursor = p;
> > +	*abuf = addr;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_inany() - Parse an IPv4 or IPv6 address from a string
> > + * @addr:	On success, updated with parsed address
> > + */
> > +bool parse_inany(const char **cursor, union inany_addr *addr)
> > +{
> > +	struct in_addr a4;
> > +
> > +	if (parse_ipv6(cursor, &addr->a6))
> > +		return true;
> > +
> > +	if (parse_ipv4(cursor, &a4)) {
> > +		*addr = inany_from_v4(a4);
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > diff --git a/parse.h b/parse.h
> > index 155b3995..2820a065 100644
> > --- a/parse.h
> > +++ b/parse.h
> > @@ -9,6 +9,8 @@
> >  #include <stdbool.h>
> >  #include <netinet/in.h>
> >  
> > +union inany_addr;
> > +
> >  /**
> >   * port_range() - Represents a non-empty range of ports
> >   * @first:	First port number in the range
> > @@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
> >  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);
> >  
> >  #endif /* _PARSE_H */  

-- 
Stefano

[RESEND #2] Re: [PATCH 08/12] parse: Add helpers for parsing IP addresses

[Stefano Brivio]

*Actually* resending to jmaloy@redhat.com, the original email had:

On Wed, 1 Jul 2026 02:07:25 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 26 Jun 2026 17:09:59 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > parse_ipv[46]() are wrappers around inet_pton() that are more
> > convenient for use when the IP is part of a longer string, rather than
> > the entire string.  parse_inany() replaces inany_pton() which again
> > will become more convenient for strings including IPs that aren't just
> > an IP.
> > 
> > For now we only have some simple and sometimes awkward use cases,
> > we'll replace these with more natural uses in future.
> > 
> > Cc: Jon Maloy <jamloy@redhat.com>

...jamloy@redhat.com instead.

> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile   |  1 +
> >  conf.c     |  6 ++--
> >  fwd_rule.c | 21 ++++---------
> >  inany.c    | 24 ++------------
> >  inany.h    |  1 -
> >  parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  parse.h    |  4 +++
> >  7 files changed, 109 insertions(+), 39 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 85d279c0..e2b22ddf 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -227,4 +227,5 @@ 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
> > +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
> >  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > diff --git a/conf.c b/conf.c
> > index bd357117..2d1895d4 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
> >  	struct in_addr mask;
> >  	unsigned long len;
> >  
> > -	if (inet_pton(AF_INET, arg, &mask)) {
> > +	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
> >  		in_addr_t hmask = ntohl(mask.s_addr);
> >  		len = __builtin_popcount(hmask);
> >  		if ((hmask << len) == 0)
> > @@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			if (addr_has_prefix_len && prefix_len_from_opt)
> >  				die("Redundant prefix length specification");
> >  
> > -			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
> > +			p = optarg;
> > +			if (!addr_has_prefix_len &&
> > +			    !(parse_inany(&p, &addr) && parse_eoi(p)))  
> 
> parse_port_range() reports error if the input isn't NULL-terminated,
> but these new functions don't, and in all the usages at least
> introduced in this patch, you always need to couple that with a &&
> parse_eoi(p) (or && !*p).
> 
> Is there a reason for this difference?
> 
> >  				die("Invalid address: %s", optarg);
> >  
> >  			if (prefix_len_from_opt && inany_v4(&addr))
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index 0b85b17e..6d7ec2c5 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  	strncpy(buf, optarg, sizeof(buf) - 1);
> >  
> >  	if ((spec = strchr(buf, '/'))) {
> > +		const char *p = buf;
> > +
> >  		*spec = 0;
> >  		spec++;
> >  
> > @@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  			}
> >  		}
> >  
> > -		if (ifname == buf + 1) {	/* Interface without address */
> > +		if (ifname == buf + 1 || /* Interface without address */
> > +		    !strcmp(buf, "*")) /* Explicit wildcard address */
> >  			addr = NULL;
> > -		} else {
> > -			char *p = buf;
> > -
> > -			/* Allow square brackets for IPv4 too for convenience */
> > -			if (*p == '[' && p[strlen(p) - 1] == ']') {
> > -				p[strlen(p) - 1] = '\0';
> > -				p++;
> > -			}
> > -
> > -			if (strcmp(p, "*") == 0)
> > -				addr = NULL;
> > -			else if (!inany_pton(p, &addr_buf))
> > -				die("Bad forwarding address '%s'", p);
> > -		}
> > +		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> > +			die("Bad forwarding address '%s'", buf);
> >  	} else {
> >  		spec = buf;
> >  
> > diff --git a/inany.c b/inany.c
> > index 23faf3ff..154f08b5 100644
> > --- a/inany.c
> > +++ b/inany.c
> > @@ -27,6 +27,7 @@
> >  #include "ip.h"
> >  #include "inany.h"
> >  #include "fwd.h"
> > +#include "parse.h"
> >  
> >  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
> >  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> > @@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
> >  	return inet_ntop(AF_INET6, &src->a6, dst, size);
> >  }
> >  
> > -/** inany_pton - Parse an IPv[46] address from text format
> > - * @src:	IPv[46] address
> > - * @dst:	output buffer, filled with parsed address
> > - *
> > - * Return: on success, 1, if no parseable address is found, 0
> > - */
> > -int inany_pton(const char *src, union inany_addr *dst)
> > -{
> > -	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
> > -		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
> > -		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
> > -		return 1;
> > -	}
> > -
> > -	if (inet_pton(AF_INET6, src, &dst->a6))
> > -		return 1;
> > -
> > -	return 0;
> > -}
> > -
> >  /**
> >   * inany_prefix_pton() - Parse an IPv[46] address with prefix length
> >   * @src:	IPv[46] address and prefix length string in CIDR format
> > @@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  	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;
> >  
> > @@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  		return 1;
> >  	}
> >  
> > -	if (inany_pton(astr, dst)) {
> > +	if (parse_inany(&p, dst) && parse_eoi(p)) {
> >  		if (plen > 32)
> >  			return 0;
> >  		*prefix_len = plen + 96;
> > diff --git a/inany.h b/inany.h
> > index 6bf3ecaa..93d98368 100644
> > --- a/inany.h
> > +++ b/inany.h
> > @@ -303,7 +303,6 @@ 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_pton(const char *src, union inany_addr *dst);
> >  int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  		      uint8_t *prefix_len);
> >  
> > diff --git a/parse.c b/parse.c
> > index bd091fde..0349c5dc 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -16,9 +16,12 @@
> >  #include <stddef.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <limits.h>
> > +#include <arpa/inet.h>
> >  
> >  #include "common.h"
> >  #include "parse.h"
> > +#include "inany.h"
> >  
> >  /**
> >   * DOC: Theory of Operation
> > @@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
> >  	*cursor = p;
> >  	return true;
> >  }
> > +
> > +/**
> > + * parse_ipv4() - Parse an IPv4 address from a string
> > + * @abuf:	On success, updated with parsed address
> > + */
> > +bool parse_ipv4(const char **cursor, struct in_addr *abuf)
> > +{
> > +	/* Brackets are not typical on IPv4, but allow for consistency */
> > +	const char *p = *cursor;
> > +	bool bracket = parse_literal(&p, "[");
> > +	char buf[INET_ADDRSTRLEN];
> > +	struct in_addr addr;
> > +	size_t len;
> > +
> > +	if (bracket)
> > +		len = strcspn(p, "]");
> > +	else
> > +		len = strspn(p, "0123456789.");
> > +
> > +	if (len >= sizeof(buf))
> > +		return false;
> > +	memcpy(buf, p, len);
> > +	buf[len] = '\0';
> > +	p += len;
> > +
> > +	if (!inet_pton(AF_INET, buf, &addr))
> > +		return false;
> > +
> > +	if (bracket && !parse_literal(&p, "]"))
> > +		return false;
> > +
> > +	*cursor = p;
> > +	*abuf = addr;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_ipv6() - Parse an IPv6 address from a string
> > + * @abuf:	On success, updated with parsed address
> > + */
> > +static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> > +{
> > +	const char *p = *cursor;
> > +	bool bracket = parse_literal(&p, "[");
> > +	char buf[INET6_ADDRSTRLEN];
> > +	struct in6_addr addr;
> > +	size_t len;
> > +
> > +	if (bracket)
> > +		len = strcspn(p, "]");
> > +	else
> > +		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
> > +
> > +	if (len >= sizeof(buf))
> > +		return false;
> > +	memcpy(buf, p, len);
> > +	buf[len] = '\0';
> > +	p += len;
> > +
> > +	if (!inet_pton(AF_INET6, buf, &addr))
> > +		return false;
> > +
> > +	if (bracket && !parse_literal(&p, "]"))
> > +		return false;
> > +
> > +	*cursor = p;
> > +	*abuf = addr;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_inany() - Parse an IPv4 or IPv6 address from a string
> > + * @addr:	On success, updated with parsed address
> > + */
> > +bool parse_inany(const char **cursor, union inany_addr *addr)
> > +{
> > +	struct in_addr a4;
> > +
> > +	if (parse_ipv6(cursor, &addr->a6))
> > +		return true;
> > +
> > +	if (parse_ipv4(cursor, &a4)) {
> > +		*addr = inany_from_v4(a4);
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > diff --git a/parse.h b/parse.h
> > index 155b3995..2820a065 100644
> > --- a/parse.h
> > +++ b/parse.h
> > @@ -9,6 +9,8 @@
> >  #include <stdbool.h>
> >  #include <netinet/in.h>
> >  
> > +union inany_addr;
> > +
> >  /**
> >   * port_range() - Represents a non-empty range of ports
> >   * @first:	First port number in the range
> > @@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
> >  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);
> >  
> >  #endif /* _PARSE_H */  

-- 
Stefano

Re: [PATCH 01/12] Makefile: Add missing PESTO_HEADERS variable

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:07:01AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:09:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In several places we use a PESTO_HEADERS variable, with all the headers
> > that we need to build the pesto binary.  However, we never define it.
> > This looks like an error introduced by a bad rebase of the series
> > introducing pesto before it was merged.
> 
> Oops, yes, I just found that in a patch reject file. And I just
> discovered that a Makefile variable that's not defined happily expands
> to nothing, I wasn't aware of that.

Yeah :/.  Classic gotcha.

> > It turns out the fact we didn't list the headers was the only reason we
> > weren't getting unusedStructMember cppcheck warnings for pesto as we
> > already do for passt and passt-repair.  So, reinstate that suppression for
> > pesto as well.
> > 
> > Fixes: 02236db32625 ("pesto: Introduce stub configuration tool")
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index e8170e90..5ed0f702 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -55,6 +55,7 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
> >  	virtio.h vu_common.h
> >  QRAP_HEADERS = arp.h ip.h passt.h util.h
> >  PASST_REPAIR_HEADERS = linux_dep.h
> > +PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h
> >  
> >  C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
> >  ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> > @@ -203,7 +204,8 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
> >  	else								\
> >  		echo "";						\
> >  	fi)								\
> > -	--suppress=missingIncludeSystem
> > +	--suppress=missingIncludeSystem					\
> > +	--suppress=unusedStructMember
> >  
> >  cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
> >  
> > @@ -212,10 +214,8 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck
> >  	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
> >  
> >  passt.cppcheck: BASE_CPPFLAGS += -UPESTO
> > -passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember
> >  passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
> >  
> > -passt-repair.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember
> >  passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
> >  
> >  pesto.cppcheck: BASE_CPPFLAGS += -DPESTO
> 
> -- 
> 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 --]

Re: [PATCH 02/12] conf: Use parameter instead of global in conf_nat()

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:07:06AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:09:53 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Conf nat takes a parameter @arg for the argument it's parsing.  However on
> 
> Nit: conf_nat().

Huh.  What a weird mistake to make.  Oh well, fixed.

> 
> > error we print instead optarg, the getopt() global.  This happens to be the
> > same thing at the time of the call, but it's not the right way to get to
> > it.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 4755a9f4..6ab8efec 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1049,7 +1049,7 @@ static void conf_nat(const char *arg, struct in_addr *addr4,
> >  	    !IN4_IS_ADDR_MULTICAST(addr4))
> >  		return;
> >  
> > -	die("Invalid address to remap to host: %s", optarg);
> > +	die("Invalid address to remap to host: %s", arg);
> >  }
> >  
> >  /**
> 
> -- 
> 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 --]

Re: [PATCH 03/12] parse: Start splitting out parsing helpers

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:07:12AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:09:54 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > As we add more complexity to what forwarding rules are allowed, our
> > existing ad-hoc C parsing is starting to become quite awkward.  We already
> > have some parts that resemble a very simple recursive[0] descent parser,
> > with composable subfunctions that consume as much input as they need,
> > rather than pre-splitting based on known delimiters.
> > 
> > Start extending this approach, by creating parse.[ch] with parsing helpers
> > with a uniform interface.  Initially we start with very simple cases: the
> > parse_keyword() function from fwd_rule.c and another helper to check that
> > we've reached the end of input.
> > 
> > Rename parse_keyword() to parse_literal(), because it's not just useful
> > for "keywords" as such.  We can use it in a bunch of additional places for
> > parsing delimiters and other symbols.  This doesn't gain us a lot now but
> > will make things clearer as we use more such parser helpers.
> > 
> > [0] Except that the grammars we have aren't actually recursive, so neither
> >     is the code.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile   | 17 ++++++++------
> >  conf.c     |  5 +++-
> >  fwd_rule.c | 33 +++++----------------------
> >  parse.c    | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  parse.h    | 14 ++++++++++++
> >  5 files changed, 101 insertions(+), 35 deletions(-)
> >  create mode 100644 parse.c
> >  create mode 100644 parse.h
> > 
> > diff --git a/Makefile b/Makefile
> > index 5ed0f702..bd729c75 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -36,12 +36,13 @@ BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security
> >  PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
> >  	epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \
> >  	isolation.c lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c \
> > -	passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \
> > +	parse.c passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \
> >  	tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \
> >  	vhost_user.c virtio.c vu_common.c
> >  QRAP_SRCS = qrap.c
> >  PASST_REPAIR_SRCS = passt-repair.c
> > -PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c serialise.c
> > +PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c parse.c \
> > +	serialise.c
> >  SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
> >  
> >  MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
> > @@ -49,13 +50,14 @@ MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
> >  PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
> >  	epoll_ctl.h flow.h fwd.h fwd_rule.h flow_table.h icmp.h icmp_flow.h \
> >  	inany.h iov.h ip.h isolation.h lineread.h log.h migrate.h ndp.h \
> > -	netlink.h packet.h passt.h pasta.h pcap.h pif.h repair.h serialise.h \
> > -	siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \
> > -	tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \
> > -	virtio.h vu_common.h
> > +	netlink.h packet.h parse.h passt.h pasta.h pcap.h pif.h repair.h \
> > +	serialise.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \
> > +	tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \
> > +	vhost_user.h virtio.h vu_common.h
> >  QRAP_HEADERS = arp.h ip.h passt.h util.h
> >  PASST_REPAIR_HEADERS = linux_dep.h
> > -PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h
> > +PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h parse.h \
> > +	pesto.h serialise.h
> >  
> >  C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
> >  ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> > @@ -223,6 +225,7 @@ 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:parse.c
> >  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
> >  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c
> >  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > diff --git a/conf.c b/conf.c
> > index 6ab8efec..ca6c859c 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -52,6 +52,7 @@
> >  #include "conf.h"
> >  #include "pesto.h"
> >  #include "serialise.h"
> > +#include "parse.h"
> >  
> >  #define NETNS_RUN_DIR	"/run/netns"
> >  
> > @@ -1028,7 +1029,9 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> >  static void conf_nat(const char *arg, struct in_addr *addr4,
> >  		     struct in6_addr *addr6, int *no_map_gw)
> >  {
> > -	if (strcmp(arg, "none") == 0) {
> > +	const char *p = arg;
> > +
> > +	if (parse_literal(&p, "none") && parse_eoi(p)) {
> 
> I think that !*p would be so much clearer here compared to:
> 
> - parse_eoi(p)... must be "input"?
> 
> - [takes a look at parse_eoi()] yes, it's input, great [goes back to
>   here]. Ah, wait, does it return false if *p is NULL? Or true?

Like all the parse_*() functions, it returns true if it *did* see the
thing it was looking for, in this case end of string.

> - [takes a second look at parse_eoi()] good, it returns true, so this
>   is *!p...
> 
> I understand it's somewhat consistent with parse_literal() this way, so
> I'm not strongly against it, just a slight preference overall.

Right, that's the reason for it.  Because later on composing these
parse helpers has a bit of its own style, it seemed helpful to
continue that style, rather than poking directly into the string.

> 
> >  		*addr4 = in4addr_any;
> >  		*addr6 = in6addr_any;
> >  		if (no_map_gw)
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index 04a0101d..d0e90402 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -24,6 +24,7 @@
> >  #include "fwd_rule.h"
> >  #include "lineread.h"
> >  #include "log.h"
> > +#include "parse.h"
> >  #include "serialise.h"
> >  
> >  /* Ephemeral port range: values from RFC 6335 */
> > @@ -427,28 +428,6 @@ static int parse_port_range(const char *s, const char **endptr,
> >  	return 0;
> >  }
> >  
> > -/**
> > - * parse_keyword() - Parse a literal keyword
> > - * @s:		String to parse
> > - * @endptr:	Update to the character after the keyword
> > - * @kw:		Keyword to accept
> > - *
> > - * Return: 0, if @s starts with @kw, -EINVAL if it does not
> > - */
> > -static int parse_keyword(const char *s, const char **endptr, const char *kw)
> > -{
> > -	size_t len = strlen(kw);
> > -
> > -	if (strlen(s) < len)
> > -		return -EINVAL;
> > -
> > -	if (memcmp(s, kw, len))
> > -		return -EINVAL;
> > -
> > -	*endptr = s + len;
> > -	return 0;
> > -}
> > -
> >  /**
> >   * fwd_rule_range_except() - Set up forwarding for a range of ports minus a
> >   *                           bitmap of exclusions
> > @@ -568,7 +547,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  			continue;
> >  		}
> >  
> > -		if (parse_keyword(p, &p, "auto") == 0) {
> > +		if (parse_literal(&p, "auto")) {
> >  			if (p != ep) /* Garbage after the keyword */
> >  				goto bad;
> >  
> > @@ -582,9 +561,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  		}
> >  
> >  		/* Should be an exclude range */
> > -		if (*p != '~')
> > +		if (!parse_literal(&p, "~"))
> >  			goto bad;
> > -		p++;
> >  
> >  		if (parse_port_range(p, &p, &xrange))
> >  			goto bad;
> > @@ -616,8 +594,9 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  		if (parse_port_range(p, &p, &orig_range))
> >  			goto bad;
> >  
> > -		if (*p == ':') { /* There's a range to map to as well */
> > -			if (parse_port_range(p + 1, &p, &mapped_range))
> > +		if (parse_literal(&p, ":")) {
> > +			/* There's a range to map to as well */
> > +			if (parse_port_range(p, &p, &mapped_range))
> >  				goto bad;
> >  			if ((mapped_range.last - mapped_range.first) !=
> >  			    (orig_range.last - orig_range.first))
> > diff --git a/parse.c b/parse.c
> > new file mode 100644
> > index 00000000..c2a92422
> > --- /dev/null
> > +++ b/parse.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +/* PASST - Plug A Simple Socket Transport
> > + *  for qemu/UNIX domain socket mode
> > + *
> > + * PASTA - Pack A Subtle Tap Abstraction
> > + *  for network namespace/tap device mode
> > + *
> > + * parse.c - Composable parsing helpers
> > + *
> > + * Copyright Red Hat
> > + * Author: David Gibson <david@gibson.dropbear.id.au>
> > + */
> > +
> > +#include <assert.h>
> > +#include <stddef.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <stdbool.h>
> > +#include <unistd.h>
> > +
> > +#include "common.h"
> > +
> > +/**
> > + * DOC: Theory of Operation
> > + *
> > + * These are a number of primitives which can be combined into parsers for
> > + * moderately complex input.  For simpler composability, they have common
> > + * conventions.
> > + *
> > + * - Functions return a bool indicating whether they successfully parsed.
> 
> As I reached 7/12 (converting parse_port_range()), this became rather
> confusing.
> 
> It makes sense for parse_eoi() (because in some sense it's a
> parse_is_eoi() function), but for the rest I'd expect almost anything
> that doesn't have "is" or similar in its name to return 0 on success,
> and a negative error code on failure (or -1).

Huh.  So an earlier draft did that, making them more similar to
strtoul() in interface.  I changed it because I was finding it
confusing.  Even knowing the convention, I found it hard to remember to read:
	if (!parse_foo() && !parse_bar())
	
as meaning we *succeeded* in parsing a foo and a bar.  That would not
be true of parse_foo() == 0, but that's bulkier.

> I'm not sure if I'm ignoring some particular advantage for the
> composability you mention above, if it's clearly better for whatever
> reason so be it, but otherwise I'd suggest re-considering this.

No technical advantage, I just found it more readable.

> I mean:
> 
> -		if (parse_port_range(p, &p, &xrange))
> +		if (!parse_port_range(&p, &xrange))
> 
> ...is quite telling.

It is?  I'm not sure what it's telling me.

> > + * - Any specific output from the parse is as output parameters
> > + * - First argument is always @cursor, a const char **.
> > + * - On entry, *@cursor has the point to start parsing.
> > + * - On successful exit, *@cursor is updated to the next character after the
> > +     parsed portion of the input
> > + * - On failure, *@cursor and any output arguments are not modified
> > + *
> > + * For brevity the common parameters and return information are omitted from the
> > + * individual function documentation comments.
> 
> The reason why we have "@c:	Execution context" all over the
> place isn't so much human readability, it's rather automated usage, and
> the day we manage to run Sphinx on the whole codebase:
> 
>   https://bugs.passt.top/show_bug.cgi?id=35
> 
> it would be nice to have a clean output instead of having to go through
> a pile of warnings.
> 
> So, actually, I'd keep @cursor "described" everywhere. We can also skip
> it for the moment but I'm afraid we'll start following this example and
> it will be more work later.

Yeah, that's reasonable.  Now that I'm not churning so much on the
interfaces it's not too hard to duplicate the descriptions.

> 
> > + */
> > +
> > +/**
> > + * parse_literal() - Parse a specified literal string
> > + * @lit:	Keyword to accept
> > + */
> > +bool parse_literal(const char **cursor, const char *lit)
> > +{
> > +	size_t len = strlen(lit);
> > +
> > +	if (strlen(*cursor) < len || memcmp(*cursor, lit, len))
> > +		return false;
> > +
> > +	*cursor += len;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_eoi() - Parse end of input
> > + * @cursor:	Current parse pointer
> > + *
> > + * Return: true if @p is at End of Input (\0), false otherwise
> > + */
> > +bool parse_eoi(const char *cursor)
> > +{
> > +	return !(*cursor);
> > +}
> > diff --git a/parse.h b/parse.h
> > new file mode 100644
> > index 00000000..eb51a469
> > --- /dev/null
> > +++ b/parse.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright Red Hat
> > + * Author: David Gibson <david@gibson.dropbear.id.au>
> > + */
> > +
> > +#ifndef PARSE_H
> > +#define PARSE_H
> > +
> > +#include <stdbool.h>
> > +
> > +bool parse_literal(const char **cursor, const char *lit);
> > +bool parse_eoi(const char *cursor);
> > +
> > +#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 --]

Re: [PATCH 08/12] parse: Add helpers for parsing IP addresses

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:07:26AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:09:59 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > parse_ipv[46]() are wrappers around inet_pton() that are more
> > convenient for use when the IP is part of a longer string, rather than
> > the entire string.  parse_inany() replaces inany_pton() which again
> > will become more convenient for strings including IPs that aren't just
> > an IP.
> > 
> > For now we only have some simple and sometimes awkward use cases,
> > we'll replace these with more natural uses in future.
> > 
> > Cc: Jon Maloy <jamloy@redhat.com>
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile   |  1 +
> >  conf.c     |  6 ++--
> >  fwd_rule.c | 21 ++++---------
> >  inany.c    | 24 ++------------
> >  inany.h    |  1 -
> >  parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  parse.h    |  4 +++
> >  7 files changed, 109 insertions(+), 39 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 85d279c0..e2b22ddf 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -227,4 +227,5 @@ 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
> > +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
> >  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > diff --git a/conf.c b/conf.c
> > index bd357117..2d1895d4 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
> >  	struct in_addr mask;
> >  	unsigned long len;
> >  
> > -	if (inet_pton(AF_INET, arg, &mask)) {
> > +	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
> >  		in_addr_t hmask = ntohl(mask.s_addr);
> >  		len = __builtin_popcount(hmask);
> >  		if ((hmask << len) == 0)
> > @@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			if (addr_has_prefix_len && prefix_len_from_opt)
> >  				die("Redundant prefix length specification");
> >  
> > -			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
> > +			p = optarg;
> > +			if (!addr_has_prefix_len &&
> > +			    !(parse_inany(&p, &addr) && parse_eoi(p)))
> 
> parse_port_range() reports error if the input isn't NULL-terminated,

Not after 7/12 it doesn't.

> but these new functions don't, and in all the usages at least
> introduced in this patch, you always need to couple that with a &&
> parse_eoi(p) (or && !*p).
> 
> Is there a reason for this difference?

Checking for \0 within the function means it can't be used to parse an
IP as part of something longer, they can only work at the end of a
string.  The simple users in this patch only need an IP at the end of
a string, but the more complex uses for -[tu] don't and mustn't follow
with parse_eoi().

Basically, checking for \0 within the parse function breaks
composability, which is the whole point of this interface style.

> >  				die("Invalid address: %s", optarg);
> >  
> >  			if (prefix_len_from_opt && inany_v4(&addr))
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index 0b85b17e..6d7ec2c5 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  	strncpy(buf, optarg, sizeof(buf) - 1);
> >  
> >  	if ((spec = strchr(buf, '/'))) {
> > +		const char *p = buf;
> > +
> >  		*spec = 0;
> >  		spec++;
> >  
> > @@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  			}
> >  		}
> >  
> > -		if (ifname == buf + 1) {	/* Interface without address */
> > +		if (ifname == buf + 1 || /* Interface without address */
> > +		    !strcmp(buf, "*")) /* Explicit wildcard address */
> >  			addr = NULL;
> > -		} else {
> > -			char *p = buf;
> > -
> > -			/* Allow square brackets for IPv4 too for convenience */
> > -			if (*p == '[' && p[strlen(p) - 1] == ']') {
> > -				p[strlen(p) - 1] = '\0';
> > -				p++;
> > -			}
> > -
> > -			if (strcmp(p, "*") == 0)
> > -				addr = NULL;
> > -			else if (!inany_pton(p, &addr_buf))
> > -				die("Bad forwarding address '%s'", p);
> > -		}
> > +		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> > +			die("Bad forwarding address '%s'", buf);
> >  	} else {
> >  		spec = buf;
> >  
> > diff --git a/inany.c b/inany.c
> > index 23faf3ff..154f08b5 100644
> > --- a/inany.c
> > +++ b/inany.c
> > @@ -27,6 +27,7 @@
> >  #include "ip.h"
> >  #include "inany.h"
> >  #include "fwd.h"
> > +#include "parse.h"
> >  
> >  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
> >  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> > @@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
> >  	return inet_ntop(AF_INET6, &src->a6, dst, size);
> >  }
> >  
> > -/** inany_pton - Parse an IPv[46] address from text format
> > - * @src:	IPv[46] address
> > - * @dst:	output buffer, filled with parsed address
> > - *
> > - * Return: on success, 1, if no parseable address is found, 0
> > - */
> > -int inany_pton(const char *src, union inany_addr *dst)
> > -{
> > -	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
> > -		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
> > -		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
> > -		return 1;
> > -	}
> > -
> > -	if (inet_pton(AF_INET6, src, &dst->a6))
> > -		return 1;
> > -
> > -	return 0;
> > -}
> > -
> >  /**
> >   * inany_prefix_pton() - Parse an IPv[46] address with prefix length
> >   * @src:	IPv[46] address and prefix length string in CIDR format
> > @@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  	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;
> >  
> > @@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  		return 1;
> >  	}
> >  
> > -	if (inany_pton(astr, dst)) {
> > +	if (parse_inany(&p, dst) && parse_eoi(p)) {
> >  		if (plen > 32)
> >  			return 0;
> >  		*prefix_len = plen + 96;
> > diff --git a/inany.h b/inany.h
> > index 6bf3ecaa..93d98368 100644
> > --- a/inany.h
> > +++ b/inany.h
> > @@ -303,7 +303,6 @@ 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_pton(const char *src, union inany_addr *dst);
> >  int inany_prefix_pton(const char *src, union inany_addr *dst,
> >  		      uint8_t *prefix_len);
> >  
> > diff --git a/parse.c b/parse.c
> > index bd091fde..0349c5dc 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -16,9 +16,12 @@
> >  #include <stddef.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <limits.h>
> > +#include <arpa/inet.h>
> >  
> >  #include "common.h"
> >  #include "parse.h"
> > +#include "inany.h"
> >  
> >  /**
> >   * DOC: Theory of Operation
> > @@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
> >  	*cursor = p;
> >  	return true;
> >  }
> > +
> > +/**
> > + * parse_ipv4() - Parse an IPv4 address from a string
> > + * @abuf:	On success, updated with parsed address
> > + */
> > +bool parse_ipv4(const char **cursor, struct in_addr *abuf)
> > +{
> > +	/* Brackets are not typical on IPv4, but allow for consistency */
> > +	const char *p = *cursor;
> > +	bool bracket = parse_literal(&p, "[");
> > +	char buf[INET_ADDRSTRLEN];
> > +	struct in_addr addr;
> > +	size_t len;
> > +
> > +	if (bracket)
> > +		len = strcspn(p, "]");
> > +	else
> > +		len = strspn(p, "0123456789.");
> > +
> > +	if (len >= sizeof(buf))
> > +		return false;
> > +	memcpy(buf, p, len);
> > +	buf[len] = '\0';
> > +	p += len;
> > +
> > +	if (!inet_pton(AF_INET, buf, &addr))
> > +		return false;
> > +
> > +	if (bracket && !parse_literal(&p, "]"))
> > +		return false;
> > +
> > +	*cursor = p;
> > +	*abuf = addr;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_ipv6() - Parse an IPv6 address from a string
> > + * @abuf:	On success, updated with parsed address
> > + */
> > +static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> > +{
> > +	const char *p = *cursor;
> > +	bool bracket = parse_literal(&p, "[");
> > +	char buf[INET6_ADDRSTRLEN];
> > +	struct in6_addr addr;
> > +	size_t len;
> > +
> > +	if (bracket)
> > +		len = strcspn(p, "]");
> > +	else
> > +		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
> > +
> > +	if (len >= sizeof(buf))
> > +		return false;
> > +	memcpy(buf, p, len);
> > +	buf[len] = '\0';
> > +	p += len;
> > +
> > +	if (!inet_pton(AF_INET6, buf, &addr))
> > +		return false;
> > +
> > +	if (bracket && !parse_literal(&p, "]"))
> > +		return false;
> > +
> > +	*cursor = p;
> > +	*abuf = addr;
> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_inany() - Parse an IPv4 or IPv6 address from a string
> > + * @addr:	On success, updated with parsed address
> > + */
> > +bool parse_inany(const char **cursor, union inany_addr *addr)
> > +{
> > +	struct in_addr a4;
> > +
> > +	if (parse_ipv6(cursor, &addr->a6))
> > +		return true;
> > +
> > +	if (parse_ipv4(cursor, &a4)) {
> > +		*addr = inany_from_v4(a4);
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > diff --git a/parse.h b/parse.h
> > index 155b3995..2820a065 100644
> > --- a/parse.h
> > +++ b/parse.h
> > @@ -9,6 +9,8 @@
> >  #include <stdbool.h>
> >  #include <netinet/in.h>
> >  
> > +union inany_addr;
> > +
> >  /**
> >   * port_range() - Represents a non-empty range of ports
> >   * @first:	First port number in the range
> > @@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
> >  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);
> >  
> >  #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 --]

Re: [RESEND #2] Re: [PATCH 08/12] parse: Add helpers for parsing IP addresses

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:11:52AM +0200, Stefano Brivio wrote:
> *Actually* resending to jmaloy@redhat.com, the original email had:
> 
> On Wed, 1 Jul 2026 02:07:25 +0200
> Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > On Fri, 26 Jun 2026 17:09:59 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > parse_ipv[46]() are wrappers around inet_pton() that are more
> > > convenient for use when the IP is part of a longer string, rather than
> > > the entire string.  parse_inany() replaces inany_pton() which again
> > > will become more convenient for strings including IPs that aren't just
> > > an IP.
> > > 
> > > For now we only have some simple and sometimes awkward use cases,
> > > we'll replace these with more natural uses in future.
> > > 
> > > Cc: Jon Maloy <jamloy@redhat.com>
> 
> ...jamloy@redhat.com instead.

Yeah, caught that just to late.  Fixed.

> 
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  Makefile   |  1 +
> > >  conf.c     |  6 ++--
> > >  fwd_rule.c | 21 ++++---------
> > >  inany.c    | 24 ++------------
> > >  inany.h    |  1 -
> > >  parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  parse.h    |  4 +++
> > >  7 files changed, 109 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 85d279c0..e2b22ddf 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -227,4 +227,5 @@ 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
> > > +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
> > >  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > > diff --git a/conf.c b/conf.c
> > > index bd357117..2d1895d4 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
> > >  	struct in_addr mask;
> > >  	unsigned long len;
> > >  
> > > -	if (inet_pton(AF_INET, arg, &mask)) {
> > > +	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
> > >  		in_addr_t hmask = ntohl(mask.s_addr);
> > >  		len = __builtin_popcount(hmask);
> > >  		if ((hmask << len) == 0)
> > > @@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > >  			if (addr_has_prefix_len && prefix_len_from_opt)
> > >  				die("Redundant prefix length specification");
> > >  
> > > -			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
> > > +			p = optarg;
> > > +			if (!addr_has_prefix_len &&
> > > +			    !(parse_inany(&p, &addr) && parse_eoi(p)))  
> > 
> > parse_port_range() reports error if the input isn't NULL-terminated,
> > but these new functions don't, and in all the usages at least
> > introduced in this patch, you always need to couple that with a &&
> > parse_eoi(p) (or && !*p).
> > 
> > Is there a reason for this difference?
> > 
> > >  				die("Invalid address: %s", optarg);
> > >  
> > >  			if (prefix_len_from_opt && inany_v4(&addr))
> > > diff --git a/fwd_rule.c b/fwd_rule.c
> > > index 0b85b17e..6d7ec2c5 100644
> > > --- a/fwd_rule.c
> > > +++ b/fwd_rule.c
> > > @@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> > >  	strncpy(buf, optarg, sizeof(buf) - 1);
> > >  
> > >  	if ((spec = strchr(buf, '/'))) {
> > > +		const char *p = buf;
> > > +
> > >  		*spec = 0;
> > >  		spec++;
> > >  
> > > @@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> > >  			}
> > >  		}
> > >  
> > > -		if (ifname == buf + 1) {	/* Interface without address */
> > > +		if (ifname == buf + 1 || /* Interface without address */
> > > +		    !strcmp(buf, "*")) /* Explicit wildcard address */
> > >  			addr = NULL;
> > > -		} else {
> > > -			char *p = buf;
> > > -
> > > -			/* Allow square brackets for IPv4 too for convenience */
> > > -			if (*p == '[' && p[strlen(p) - 1] == ']') {
> > > -				p[strlen(p) - 1] = '\0';
> > > -				p++;
> > > -			}
> > > -
> > > -			if (strcmp(p, "*") == 0)
> > > -				addr = NULL;
> > > -			else if (!inany_pton(p, &addr_buf))
> > > -				die("Bad forwarding address '%s'", p);
> > > -		}
> > > +		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> > > +			die("Bad forwarding address '%s'", buf);
> > >  	} else {
> > >  		spec = buf;
> > >  
> > > diff --git a/inany.c b/inany.c
> > > index 23faf3ff..154f08b5 100644
> > > --- a/inany.c
> > > +++ b/inany.c
> > > @@ -27,6 +27,7 @@
> > >  #include "ip.h"
> > >  #include "inany.h"
> > >  #include "fwd.h"
> > > +#include "parse.h"
> > >  
> > >  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
> > >  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> > > @@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
> > >  	return inet_ntop(AF_INET6, &src->a6, dst, size);
> > >  }
> > >  
> > > -/** inany_pton - Parse an IPv[46] address from text format
> > > - * @src:	IPv[46] address
> > > - * @dst:	output buffer, filled with parsed address
> > > - *
> > > - * Return: on success, 1, if no parseable address is found, 0
> > > - */
> > > -int inany_pton(const char *src, union inany_addr *dst)
> > > -{
> > > -	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
> > > -		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
> > > -		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
> > > -		return 1;
> > > -	}
> > > -
> > > -	if (inet_pton(AF_INET6, src, &dst->a6))
> > > -		return 1;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  /**
> > >   * inany_prefix_pton() - Parse an IPv[46] address with prefix length
> > >   * @src:	IPv[46] address and prefix length string in CIDR format
> > > @@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> > >  	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;
> > >  
> > > @@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> > >  		return 1;
> > >  	}
> > >  
> > > -	if (inany_pton(astr, dst)) {
> > > +	if (parse_inany(&p, dst) && parse_eoi(p)) {
> > >  		if (plen > 32)
> > >  			return 0;
> > >  		*prefix_len = plen + 96;
> > > diff --git a/inany.h b/inany.h
> > > index 6bf3ecaa..93d98368 100644
> > > --- a/inany.h
> > > +++ b/inany.h
> > > @@ -303,7 +303,6 @@ 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_pton(const char *src, union inany_addr *dst);
> > >  int inany_prefix_pton(const char *src, union inany_addr *dst,
> > >  		      uint8_t *prefix_len);
> > >  
> > > diff --git a/parse.c b/parse.c
> > > index bd091fde..0349c5dc 100644
> > > --- a/parse.c
> > > +++ b/parse.c
> > > @@ -16,9 +16,12 @@
> > >  #include <stddef.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > +#include <limits.h>
> > > +#include <arpa/inet.h>
> > >  
> > >  #include "common.h"
> > >  #include "parse.h"
> > > +#include "inany.h"
> > >  
> > >  /**
> > >   * DOC: Theory of Operation
> > > @@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
> > >  	*cursor = p;
> > >  	return true;
> > >  }
> > > +
> > > +/**
> > > + * parse_ipv4() - Parse an IPv4 address from a string
> > > + * @abuf:	On success, updated with parsed address
> > > + */
> > > +bool parse_ipv4(const char **cursor, struct in_addr *abuf)
> > > +{
> > > +	/* Brackets are not typical on IPv4, but allow for consistency */
> > > +	const char *p = *cursor;
> > > +	bool bracket = parse_literal(&p, "[");
> > > +	char buf[INET_ADDRSTRLEN];
> > > +	struct in_addr addr;
> > > +	size_t len;
> > > +
> > > +	if (bracket)
> > > +		len = strcspn(p, "]");
> > > +	else
> > > +		len = strspn(p, "0123456789.");
> > > +
> > > +	if (len >= sizeof(buf))
> > > +		return false;
> > > +	memcpy(buf, p, len);
> > > +	buf[len] = '\0';
> > > +	p += len;
> > > +
> > > +	if (!inet_pton(AF_INET, buf, &addr))
> > > +		return false;
> > > +
> > > +	if (bracket && !parse_literal(&p, "]"))
> > > +		return false;
> > > +
> > > +	*cursor = p;
> > > +	*abuf = addr;
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * parse_ipv6() - Parse an IPv6 address from a string
> > > + * @abuf:	On success, updated with parsed address
> > > + */
> > > +static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> > > +{
> > > +	const char *p = *cursor;
> > > +	bool bracket = parse_literal(&p, "[");
> > > +	char buf[INET6_ADDRSTRLEN];
> > > +	struct in6_addr addr;
> > > +	size_t len;
> > > +
> > > +	if (bracket)
> > > +		len = strcspn(p, "]");
> > > +	else
> > > +		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
> > > +
> > > +	if (len >= sizeof(buf))
> > > +		return false;
> > > +	memcpy(buf, p, len);
> > > +	buf[len] = '\0';
> > > +	p += len;
> > > +
> > > +	if (!inet_pton(AF_INET6, buf, &addr))
> > > +		return false;
> > > +
> > > +	if (bracket && !parse_literal(&p, "]"))
> > > +		return false;
> > > +
> > > +	*cursor = p;
> > > +	*abuf = addr;
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * parse_inany() - Parse an IPv4 or IPv6 address from a string
> > > + * @addr:	On success, updated with parsed address
> > > + */
> > > +bool parse_inany(const char **cursor, union inany_addr *addr)
> > > +{
> > > +	struct in_addr a4;
> > > +
> > > +	if (parse_ipv6(cursor, &addr->a6))
> > > +		return true;
> > > +
> > > +	if (parse_ipv4(cursor, &a4)) {
> > > +		*addr = inany_from_v4(a4);
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > diff --git a/parse.h b/parse.h
> > > index 155b3995..2820a065 100644
> > > --- a/parse.h
> > > +++ b/parse.h
> > > @@ -9,6 +9,8 @@
> > >  #include <stdbool.h>
> > >  #include <netinet/in.h>
> > >  
> > > +union inany_addr;
> > > +
> > >  /**
> > >   * port_range() - Represents a non-empty range of ports
> > >   * @first:	First port number in the range
> > > @@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
> > >  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);
> > >  
> > >  #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 --]

Re: [PATCH 09/12] conf: Move address configuration into helper function

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:07:38AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:10:00 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > The handling of the -a option is getting complex enough that it's pretty
> > bulky inside it's switch label.  Move it into a new conf_addr() function.
> 
> Nit if you respin: 'its'.

Done.

> 
> > We also rename the bulky addr_has_prefix_len and prefix_len_from_opt
> > variables to the terser 'opt_a_is_prefix' and 'opt_n', since they are
> > specifically about those command line options.
> > 
> > Cc: Jon Maloy <jmaloy@redhat.com>
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c | 96 ++++++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 53 insertions(+), 43 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 2d1895d4..3614776c 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1168,6 +1168,53 @@ int conf_tap_fd(const char *arg)
> >  	return val;
> >  }
> >  
> > +/**
> > + * conf_addr() - Configure guest address with -a option
> > + * @c:		Execution context
> > + * @arg:	-a command line argument
> > + * @opt_n:	Value from -n option, if any
> > + */
> > +static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n)
> > +{
> > +	union inany_addr addr;
> > +	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 (inany_is_unspecified(&addr) || inany_is_multicast(&addr) ||
> > +	    inany_is_loopback(&addr) || IN6_IS_ADDR_V4COMPAT(&addr.a6))
> > +		die("Invalid address: %s", arg);
> > +
> > +	if (inany_v4(&addr)) {
> > +		c->ip4.addr = *inany_v4(&addr);
> > +		c->ip4.prefix_len = prefix_len - 96;
> > +		c->ip4.addr_fixed = true;
> > +		if (c->mode == MODE_PASTA)
> > +			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;
> > +	}
> > +
> > +	return is_prefix;
> > +}
> > +
> >  /**
> >   * conf() - Process command-line arguments and set configuration
> >   * @c:		Execution context
> > @@ -1262,12 +1309,12 @@ void conf(struct ctx *c, int argc, char **argv)
> >  	unsigned dns4_idx = 0, dns6_idx = 0;
> >  	unsigned long max_mtu = IP_MAX_MTU;
> >  	struct fqdn *dnss = c->dns_search;
> > -	bool addr_has_prefix_len = false;
> > -	uint8_t prefix_len_from_opt = 0;
> >  	unsigned int ifi4 = 0, ifi6 = 0;
> > +	bool opt_a_is_prefix = false;
> >  	const char *logfile = NULL;
> >  	char *runas = NULL;
> >  	size_t logsize = 0;
> > +	uint8_t opt_n = 0;
> >  	int name, ret;
> >  	uid_t uid;
> >  	gid_t gid;
> > @@ -1567,52 +1614,15 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			c->mtu = mtu;
> >  			break;
> >  		}
> > -		case 'a': {
> > -			union inany_addr addr;
> > -			uint8_t prefix_len;
> > -
> > -			addr_has_prefix_len = inany_prefix_pton(optarg, &addr,
> > -								&prefix_len);
> > -
> > -			if (addr_has_prefix_len && prefix_len_from_opt)
> > -				die("Redundant prefix length specification");
> > -
> > -			p = optarg;
> > -			if (!addr_has_prefix_len &&
> > -			    !(parse_inany(&p, &addr) && parse_eoi(p)))
> > -				die("Invalid address: %s", optarg);
> > -
> > -			if (prefix_len_from_opt && inany_v4(&addr))
> > -				prefix_len = prefix_len_from_opt;
> > -			else if (!addr_has_prefix_len)
> > -				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", optarg);
> > -
> > -			if (inany_v4(&addr)) {
> > -				c->ip4.addr = *inany_v4(&addr);
> > -				c->ip4.prefix_len = prefix_len - 96;
> > -				c->ip4.addr_fixed = true;
> > -				if (c->mode == MODE_PASTA)
> > -					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;
> > -			}
> > +		case 'a':
> > +			opt_a_is_prefix = conf_addr(c, optarg, opt_n);
> >  			break;
> > -		}
> >  		case 'n':
> > -			if (addr_has_prefix_len)
> > +			if (opt_a_is_prefix)
> >  				die("Redundant prefix length specification");
> >  
> >  			c->ip4.prefix_len = conf_ip4_prefix(optarg);
> > -			prefix_len_from_opt = c->ip4.prefix_len + 96;
> > +			opt_n = c->ip4.prefix_len + 96;
> >  			break;
> >  		case 'M':
> >  			parse_mac(c->our_tap_mac, optarg);
> 
> -- 
> 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 --]

Re: [PATCH 10/12] conf: Use new parsing tools to handle -a option

[David Gibson]

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

Re: [PATCH 11/12] fwd_rule: Allow "all" port specs to be combined with other options

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:07:56AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:10:02 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently we handle -t all and the like as a special case, it can't be
> > combined with other port specifier options.  Remove that restriction,
> > allowing combined options like:
> >      -t all,~9999          # Forward everything non-ephemeral except 9999
> >      -t all,auto           # Equivalent to -t auto
> >      -t all,33000          # Forward non-ephemeral plus port 33,000
> > 
> > This isn't particularly useful immediately,
> 
> Actually, -t all,33000 could be useful right away. I didn't realise we
> weren't allowing that.

Ok, well, bonus.

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

Re: [PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers

[David Gibson]

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

On Wed, Jul 01, 2026 at 02:08:02AM +0200, Stefano Brivio wrote:
> On Fri, 26 Jun 2026 17:10:03 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Forwarding rules for the -[tTuU] options are parsed in fwd_rule_parse()
> > and fwd_rule_parse_ports().  Currently those use a mix of loosely
> > recursive descent parsing helpers, consuming input from left to right,
> > and ad-hoc C parsing - searching for delimeters with strchr() or the like
> 
> Nit if you respin: delimiters.

Fixed.

> > and breaking down on that basis.
> > 
> > As well as being a slightly awkward mix, the current approach will not work
> > for adding target addresses to the rules: we can't easily split the
> > listening from target parts first, because the ':' delimiter could appear
> > multiple times for multiple port ranges, or in IPv6 addresses.  However,
> > without splitting the target part first, we can't first split off the
> > listening address and interface because the '/' delimiter could appear in
> > the target portion, but not the listening portion, e.g.:
> >     -t 12345:192.0.1.1/12345
> > 
> > To address this, rewrite the entirety of the parsing so we consume the
> > input left to right in a more-or-less recursive descent manner.  This means
> > we no longer rely on certain delimiters having a single meaning over the
> > whole input, just the next part of it.
> > 
> > Because of the semantics of port specs, we need to make several passes
> > over the list of comma separated chunks.  Previously, some of the logic was
> > duplicated between the two passes, making it fragile.  Now, we introduce
> > a parse_port_chunk() helper which we re-use in each pass to make it clearer
> > we parse exactly the same way on each pass.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  fwd_rule.c | 262 ++++++++++++++++++++++++++++++-----------------------
> >  parse.c    |  29 ++++++
> >  parse.h    |   2 +
> >  3 files changed, 181 insertions(+), 112 deletions(-)
> > 
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index b14df340..8ae21b99 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -439,17 +439,96 @@ fail:
> >  	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
> >  }
> >  
> > -/*
> > - * for_each_chunk - Step through delimited chunks of a string
> > - * @p_:		Pointer to start of each chunk (updated)
> > - * @ep_:	Pointer to end of each chunk (updated)
> > - * @s_:		String to step through
> > - * @sep_:	String of all allowed delimiters
> > +/**
> > + * enum fwd_port_chunk_kind - Kind of port specifier piece
> 
> Nit: I think we should use kerneldoc-style comments also for enum
> (see enum udp_iov_idx, udp.c for an example).

Sure, done.

> > + */
> > +enum fwd_port_chunk_kind {
> > +	CHUNK_ALL,		/* "all" */
> > +	CHUNK_AUTO,		/* "auto" */
> > +	CHUNK_EXCLUDE,		/* "~1111-2222" */
> > +	CHUNK_INCLUDE,		/* "1111-2222" */
> > +};
> > +
> > +/**
> > + * parse_port_chunk() - Parse one chunk of a port specifier
> > + * @cursor:	Parsing point (see parse.c)
> > + * @kindp:	Updated with kind of chunk we parsed
> > + * @lrange:	Updated with listening port range (for INCLUDE & EXCLUDE)
> > + * @trange:	Updated with target port range (for EXCLUDE)
> > + */
> > +static bool parse_port_chunk(const char **cursor, enum fwd_port_chunk_kind *kindp,
> > +			     struct port_range *lrange, struct port_range *trange)
> 
> Nit: this exceeds 80 columns.

Oops, fixed.

> I'm wondering whether this doesn't rather belong in parse.c: having it
> here makes it hard to spot what it returns (the "Theory of Operation"
> comment from parse.c), and anyway it's pure parsing, nothing else.

Maybe.  I left it (and parse_laddrif()) here because they're so
specific to the syntax of -[tTuU] and unlikely to be wanted for
anything else.

> > +{
> > +	struct port_range lr = { 0 }, tr = { 0 };
> > +	enum fwd_port_chunk_kind kind;
> > +	const char *p = *cursor;
> > +
> > +	if (parse_literal(&p, "all")) {
> > +		kind = CHUNK_ALL;
> > +	} else if (parse_literal(&p, "auto")) {
> > +		kind = CHUNK_AUTO;
> > +	} else if (parse_literal(&p, "~")) {
> > +		kind = CHUNK_EXCLUDE;
> > +		if (!parse_port_range(&p, &lr))
> > +			return false;
> > +	} else if (parse_port_range(&p, &lr)) {
> > +		kind = CHUNK_INCLUDE;
> > +
> > +		if (parse_literal(&p, ":")) {
> > +			if (!parse_port_range(&p, &tr))
> > +				return false;
> > +		} else {
> > +			tr = lr;
> > +		}
> > +	} else {
> > +		return false;
> > +	}
> > +
> > +	*kindp = kind;
> > +	*lrange = lr;
> > +	if (trange)
> > +		*trange = tr;
> > +	*cursor = p;
> 
> I wonder if it's worth it being rigid about avoiding side effects on
> failure, in these functions. In general I agree it's desirable, but in
> practice we don't need it here and it's quite a few extra lines with
> slightly confusing variable names.

So, side effects on failure is a pretty big footgun:

	foo_t foo = FOO_DEFAULT;

	if (parse_foo(&p, &foo)) {
		/* handle foo override */
	} else {
	 	/* uh oh foo could be clobbered */
	}

Which is why I was strict about this.  On the other hand, the
footgun's still kind of there when you start combining things.

	foo_t foo = FOO_DEFAULT;
	bar_t bar;

	if (parse_foo(&p, &foo) && parse_bar(&p, &bar)) {
		/* do stuff */
	} else {
		/* Uh oh, foo could be clobbered if parse_foo() succeded, but
		   parse_bar() failed */
	}

This is one of the nasties which makes me less happy with this
framework than I was hoping.

> > +	return true;
> > +}
> > +
> > +/**
> > + * parse_laddrif() - Parse listening address+ifname
> 
> This makes it look like + is a separator, maybe "address%ifname"?
> 
> The name isn't really descriptive I think. Maybe we could take for
> granted that it's about the listening part, and call it
> parse_addrifname()? Or parse_addr_ifname()?

Sure, adjusted.

> > + * @cursor:	Parsing cursor (see parse.c)
> > + * @addr:	Updated with parsed inany address (NULL for *)
> > + * @abuf:	Buffer to store address
> > + * @ifname:	Updated with parsed interface name ("" if none)
> >   */
> > -#define for_each_chunk(p_, ep_, s_, sep_)			\
> > -	for ((p_) = (s_);					\
> > -	     (ep_) = (p_) + strcspn((p_), (sep_)), *(p_);	\
> > -	     (p_) = *(ep_) ? (ep_) + 1 : (ep_))
> > +static bool parse_laddrif(const char **cursor,
> > +			  const union inany_addr **addr,
> > +			  union inany_addr *abuf,
> > +			  char *ifname)
> > +{
> > +	union inany_addr atmp = inany_any6;
> > +	char iftmp[IFNAMSIZ] = {0};
> > +	const char *p;
> > +
> > +	if (p = *cursor,
> 
> The advantage of using the comma operator isn't really clear to me
> here. Can't we just do this assignment unconditionally as
> initialisation?

We could here, but not for the else if below.  We need the
reinitialization for the else if, because if some but not all of the
parse chain for the if clause suceeds, we'll arrive at the else if
with p no longer at the start of the string.  So, doing it the same
way for both if and else if seemed the least bad.

This is the #1 example of the sequencing subtlety that I'm not
thrilled with in this scheme.  I experimented with soe other
approaches, but they had their own problems:

 * Many of the earlier drafts had parse_foo(p, &ep, ...), strtoul()
   style.  With *ep not updated on failure (which seems like the
   obvious choice), there's a different footgun:
	have_optional_foo = parse_foo(start, &p, &foo);
	if (!parse_the_rest(p, &p))
		die();
   We need p, not start as the first parameter to parse_the_rest(), so
   it starts after the foo.  If there was a foo, if there wasn't a
   foo, p is uninitialized.  Damn.
 * We could change the convention so that *ep is updated to the entry
   value of p on failure.  Maybe that would be better, but I'm worried
   there's some other gotcha.  At the least it means more caution in
   all the helpers to enforce that rigid requirement.
 * We could have each parser take the start point, and return the end
   point, or NULL on failure.  But that means more hassle and
   verbosity in the callers because we need to keep around parse
   points to go back to on any non-fatal failure of a sub-parser.
   Plus it means chaining things together is the rather awkward:
     ep = parse_last_thing(parse_middle_thing(parse_first_thing(p)));

I'd love to have a better way, but I don't see it.  Or at least, not
in C: I can see lovely ways to do it with closures, generics and
higher-order functions.

> By the way, while we don't adhere to it, I think MISRA C contains many
> guidelines that are actually useful (maybe more than half of them?), and
> MISRA C:2012 forbids this kind of usage because of readability issues:
> 
>   https://www.mathworks.com/help/bugfinder/ref/misrac2012rule12.3.html

As a rule, I completely agree.  It was still the least worst way I
could see.  The , operator is the only way to reinitialize the cursor
between the if condition and the else if condition, short of
introducing a bunch of extra booleans.

> > +	    parse_inany(&p, &atmp)		&&
> > +	    parse_ifspec(&p, iftmp)		&&
> > +	    parse_literal(&p, "/")) {
> > +		/* Specific listening address */
> > +		*addr = abuf;
> > +	} else if (p = *cursor,
> > +		   parse_literal(&p, "*"),
> > +		   parse_ifspec(&p, iftmp)	&&
> > +		   parse_literal(&p, "/")) {
> > +		/* Missing or "*" address */
> > +		*addr = NULL;
> > +	} else {
> > +		return false;
> > +	}
> > +
> > +	*abuf = atmp;
> > +	memcpy(ifname, iftmp, IFNAMSIZ);
> > +	*cursor = p;
> > +	return true;
> > +}
> >  
> >  /**
> >   * fwd_rule_parse_ports() - Parse port range(s) specifier
> > @@ -466,87 +545,70 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  				 const char *spec)
> >  {
> >  	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
> > +	enum fwd_port_chunk_kind kind;
> > +	struct port_range lrange;
> >  	bool exclude_only = true;
> > -	const char *p, *ep;
> >  	uint8_t flags = 0;
> > +	const char *p;
> >  	unsigned i;
> >  
> > -	/* Parse excluded ranges and "auto" in the first pass */
> > -	for_each_chunk(p, ep, spec, ",") {
> > -		struct port_range xrange;
> > -
> > -		/* Include range, parse later */
> > -		if (parse_literal(&p, "all") || isdigit(*p))
> > -			continue;
> > -
> > -		if (parse_literal(&p, "auto")) {
> > -			if (p != ep) /* Garbage after the keyword */
> > -				goto bad;
> > +	/* Consider excluded ranges and "auto" in the first pass */
> > +	p = spec;
> > +	do {
> > +		if (!parse_port_chunk(&p, &kind, &lrange, NULL))
> > +			goto bad;
> >  
> > +		switch (kind) {
> > +		case CHUNK_AUTO:
> >  			if (!(fwd->caps & FWD_CAP_SCAN)) {
> >  				die(
> >  "'auto' port forwarding is only allowed for pasta");
> >  			}
> > -
> >  			flags |= FWD_SCAN;
> > -			continue;
> > -		}
> > -
> > -		/* Should be an exclude range */
> > -		if (!parse_literal(&p, "~"))
> > -			goto bad;
> > -
> > -		if (!parse_port_range(&p, &xrange))
> > -			goto bad;
> > -		if (p != ep) /* Garbage after the range */
> > -			goto bad;
> > -
> > -		for (i = xrange.first; i <= xrange.last; i++)
> > -			bitmap_set(exclude, i);
> > -	}
> > -
> > -	/* Now process base ranges, skipping exclusions */
> > -	for_each_chunk(p, ep, spec, ",") {
> > -		struct port_range orig_range, mapped_range;
> > -
> > -		/* Handle "all" like exclude only */
> > -		if (parse_literal(&p, "all")) {
> > -			if (p != ep) /* Garbage after the keyword */
> > -				goto bad;
> > +			break;
> >  
> > -			continue;
> > +		case CHUNK_EXCLUDE:
> > +			for (i = lrange.first; i <= lrange.last; i++)
> > +				bitmap_set(exclude, i);
> > +			break;
> > +		default:
> > +			; /* Handled later */
> >  		}
> > +	} while (parse_literal(&p, ","));
> >  
> > -		if (!isdigit(*p))
> > -			/* Already parsed */
> > -			continue;
> > +	/* Consider included ranges in next pass */
> > +	p = spec;
> > +	do {
> > +		struct port_range trange;
> >  
> > -		if (!parse_port_range(&p, &orig_range))
> > +		if (!parse_port_chunk(&p, &kind, &lrange, &trange))
> >  			goto bad;
> >  
> > -		exclude_only = false;
> > +		switch (kind) {
> > +		case CHUNK_AUTO:	/* already handled */
> > +		case CHUNK_EXCLUDE:	/* already handled */
> > +		case CHUNK_ALL:		/* handled later */
> > +			continue;
> >  
> > -		if (parse_literal(&p, ":")) {
> > -			/* There's a range to map to as well */
> > -			if (!parse_port_range(&p, &mapped_range))
> > -				goto bad;
> > -			if ((mapped_range.last - mapped_range.first) !=
> > -			    (orig_range.last - orig_range.first))
> > +		case CHUNK_INCLUDE:
> > +			exclude_only = false;
> > +			if (trange.last - trange.first !=
> > +			    lrange.last - lrange.first)
> >  				goto bad;
> > -		} else {
> > -			mapped_range = orig_range;
> > -		}
> >  
> > -		if (p != ep) /* Garbage after the ranges */
> > +			fwd_rule_range_except(fwd, del, proto, addr, ifname,
> > +					      lrange.first, lrange.last,
> > +					      exclude, trange.first, flags);
> > +			break;
> > +		default:
> >  			goto bad;
> > +		}
> > +	} while (parse_literal(&p, ","));
> >  
> > -		fwd_rule_range_except(fwd, del, proto, addr, ifname,
> > -				      orig_range.first, orig_range.last,
> > -				      exclude,
> > -				      mapped_range.first, flags);
> > -	}
> > +	if (!parse_eoi(p))
> > +		goto bad; /* trailing garbage */
> >  
> > -	/* Finally handle "all" and exclude only specs */
> > +	/* Finally handle "all" and exclude only cases */
> >  	if (exclude_only) {
> >  		fwd_port_map_ephemeral(exclude);
> >  
> > @@ -569,10 +631,11 @@ bad:
> >  void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  		    struct fwd_table *fwd)
> >  {
> > -	char buf[BUFSIZ], *spec, *ifname = NULL;
> > -	union inany_addr addr_buf = inany_any6;
> > -	const union inany_addr *addr = &addr_buf;
> > +	const union inany_addr *addr;
> > +	union inany_addr addr_buf;
> > +	char ifname[IFNAMSIZ];
> >  	uint8_t proto;
> > +	const char *p;
> >  
> >  	if (optname == 't' || optname == 'T')
> >  		proto = IPPROTO_TCP;
> > @@ -581,7 +644,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  	else
> >  		assert(0);
> >  
> > -	if (!strcmp(optarg, "none")) {
> > +	if (p = optarg,
> 
> Same as above: do we really need this? It's local anyway.

Here, no, but the idea was using this as the consistent style was
safer than only using it when we strictly need it for an else if.

> > +	    parse_literal(&p, "none") && parse_eoi(p)) {
> >  		unsigned i;
> >  
> >  		for (i = 0; i < fwd->count; i++) {
> > @@ -593,45 +657,19 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  		return;
> >  	}
> >  
> > -	strncpy(buf, optarg, sizeof(buf) - 1);
> > -
> > -	if ((spec = strchr(buf, '/'))) {
> > -		const char *p = buf;
> > -
> > -		*spec = 0;
> > -		spec++;
> > -
> > -		if (optname != 't' && optname != 'u')
> > +	if (p = optarg,
> > +	    parse_laddrif(&p, &addr, &addr_buf, ifname)) {
> > +		if (optname == 'T' || optname == 'U')
> >  			die("Listening address not allowed for -%c %s",
> >  			    optname, optarg);
> > -
> > -		if ((ifname = strchr(buf, '%'))) {
> > -			*ifname = 0;
> > -			ifname++;
> > -
> > -			/* spec is already advanced one past the '/',
> > -			 * so the length of the given ifname is:
> > -			 * (spec - ifname - 1)
> > -			 */
> > -			if (spec - ifname - 1 >= IFNAMSIZ) {
> > -				die("Interface name '%s' is too long (max %u)",
> > -				    ifname, IFNAMSIZ - 1);
> > -			}
> > -		}
> > -
> > -		if (ifname == buf + 1 || /* Interface without address */
> > -		    !strcmp(buf, "*")) /* Explicit wildcard address */
> > -			addr = NULL;
> > -		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> > -			die("Bad forwarding address '%s'", buf);
> >  	} else {
> > -		spec = buf;
> > -
> > +		/* No address or ifname */
> >  		addr = NULL;
> > +		ifname[0] = '\0';
> 
> Actually, here, having side effects from parse_laddrif() failing would
> be rather convenient and intuitive.

Yes, but it kind of just moves the awkwardness into the function.
This is roughly the difference between having a sub-parser "fail",
requiring the caller to fall back, or having it "succeed" by parsing
an empty string into a default value.  parse_laddrif() uses the former
style.  parse_ifspec() uses the latter.

> >  	}
> >  
> >  	if (optname == 'T' || optname == 'U') {
> > -		assert(!addr && !ifname);
> > +		assert(!addr && !*ifname);
> >  
> >  		if (!(fwd->caps & FWD_CAP_IFNAME)) {
> >  			warn(
> > @@ -640,18 +678,18 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  
> >  			if (fwd->caps & FWD_CAP_IPV4) {
> >  				fwd_rule_parse_ports(fwd, del, proto,
> > -						     &inany_loopback4, NULL,
> > -						     spec);
> > +						     &inany_loopback4, NULL, p);
> >  			}
> >  			if (fwd->caps & FWD_CAP_IPV6) {
> >  				fwd_rule_parse_ports(fwd, del, proto,
> > -						     &inany_loopback6, NULL,
> > -						     spec);
> > +						     &inany_loopback6, NULL, p);
> >  			}
> >  			return;
> >  		}
> >  
> > -		ifname = "lo";
> > +		static_assert(sizeof("lo") <= sizeof(ifname),
> > +			      "ifname buffer too small");
> > +		strcpy(ifname, "lo");
> >  	}
> >  
> >  	/* No need for dual stack if we only have one IP version */
> > @@ -660,13 +698,13 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> >  	else if (!addr && !(fwd->caps & FWD_CAP_IPV6))
> >  		addr = &inany_any4;
> >  
> > -	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
> > +	if (*ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
> >  		die(
> >  "Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
> >  		    optname, optarg);
> >  	}
> >  
> > -	fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec);
> > +	fwd_rule_parse_ports(fwd, del, proto, addr, *ifname ? ifname : NULL, p);
> >  }
> >  
> >  /**
> > diff --git a/parse.c b/parse.c
> > index 3e0dbd45..d83ea9f9 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -18,6 +18,7 @@
> >  #include <string.h>
> >  #include <limits.h>
> >  #include <arpa/inet.h>
> > +#include <net/if.h>
> >  
> >  #include "common.h"
> >  #include "parse.h"
> > @@ -212,3 +213,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
> >  
> >  	return false;
> >  }
> > +
> > +/**
> > + * parse_ifspec() - Parse a interface name specifier (starting with %)
> > + * @ifname:	On success updated with parsed name (must have IFNAMSIZ space)
> > + *
> > + * This will accept a missing specifier (empty string), setting ifname to ""
> > + */
> > +bool parse_ifspec(const char **cursor, char *ifname)
> > +{
> > +	const char *p = *cursor;
> > +	size_t len;
> > +
> > +	if (!parse_literal(&p, "%")) {
> > +		/* No interface specifier */
> > +		ifname[0] = '\0';
> > +		return true;
> > +	}
> > +
> > +	/* ifnames can have anything that's not '/' or whitespace */
> 
> dev_valid_name(), net/core/dev.c in the Linux kernel, also forbids '.'
> and '..', to avoid breaking sysfs. We don't really _need_ to check for
> that as we don't create interfaces, but strictly speaking the comment
> isn't accurate, and we might want to filter for consistency anyway.

Ah, missed that.  Fixed.

> 
> > +	len = strcspn(p, "/ \f\n\r\t\v");
> > +	if (!len || len >= IFNAMSIZ)
> > +		return false;
> > +
> > +	memcpy(ifname, p, len);
> > +	ifname[len] = '\0';
> > +	*cursor = p + len;
> > +	return true;
> > +}
> > diff --git a/parse.h b/parse.h
> > index 08b038cf..1079f5bf 100644
> > --- a/parse.h
> > +++ b/parse.h
> > @@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
> >  
> >  #define parse_inany(cursor, addr)	parse_inany_((cursor), (addr), NULL)
> >  
> > +bool parse_ifspec(const char **cursor, char *ifname);
> > +
> >  #endif /* _PARSE_H */
> 
> Other than those comments, it all looks good to me, and also like an
> obvious, much needed improvement. I tested it quite a bit and I didn't
> manage to break things.
> 
> -- 
> 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 --]