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

On Wed, Jul 01, 2026 at 02:07:12AM +0200, Stefano Brivio wrote:

On Fri, 26 Jun 2026 17:09:54 +1000 David Gibson 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 --- 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 \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 + */ + +#include +#include +#include +#include +#include +#include + +#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 + */ + +#ifndef PARSE_H +#define PARSE_H + +#include + +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