[PATCH v3 00/13] Rework option parsing in preparation for destination remapping
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. v3: * Fixed validation of ifnames in 13/13. * No other changes. v2: * Numerous minor tweaks based on Stefano's feedback. * The "minor issues" aren't actually minor - they're hard to solve in C, remain. David Gibson (13): 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: Remove unnecessary mode checks from conf_addr() 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 | 400 ++++++++++++++++++++++++----------------------------- fwd_rule.h | 2 - inany.c | 70 +--------- inany.h | 3 - parse.c | 244 ++++++++++++++++++++++++++++++++ parse.h | 37 +++++ util.c | 12 +- 11 files changed, 600 insertions(+), 403 deletions(-) create mode 100644 parse.c create mode 100644 parse.h -- 2.54.0
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
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
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
The handling of the -a option is getting complex enough that it's pretty
bulky inside its 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
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
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
conf_addr() sets c->ip[46]no_copy_addrs conditional upon being in pasta
mode. That sort of makes sense, since address copying is only a thing for
pasta mode. However, setting the variables anyway is harmless, and
arguably more logically consistent. If we had a way of copying addresses
for passt mode (or some future mode), it would still be incorrect to do so
in these circumstances.
So, make the assignments unconditional.
Signed-off-by: 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
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
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
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 delimiters 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
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
On Thu, Jul 02, 2026 at 10:58:48AM +1000, David Gibson wrote:
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.
Reposting again, because I realised 12/13 was missing matching manpage updates. -- 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
participants (1)
-
David Gibson