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

[Stefano Brivio <sbrivio@redhat.com>]

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