Re: [PATCH v9 19/23] pesto, conf, fwd_rule: Add options and modes to add, delete, clear rules

On Wed, 6 May 2026 14:07:37 +0200 Stefano Brivio wrote:

On Wed, 6 May 2026 13:43:07 +0200 Laurent Vivier wrote:

...

On 5/6/26 11:22, Stefano Brivio wrote:

...

Instead of just being able to add to the existing tables, implement an explicit --clear option to replace them, which now becomes the default behaviour, and implement explicit --add and --delete options to maintain the table and add or delete specific ports.

The option --clear PIF forces the clearing of a table, instead.

These options can be combined arbitrarily and are handled as sequential commands, as now described in pesto(1).

If no option is given before forwarding specifiers for a matching table, the command line is interpreted as a replacement of the existing rules.

To this end:

- there's no protocol change, as pesto is anyway sending updated copies of the table

- the forwarding table functions now include a new fwd_rule_del(), which deletes existing rule only if a matching one is found

- a trivial fwd_rule_clear() is factored out from the existing conf_handler() implementation, so that it can be directly used in pesto

The entry points for parsing of port specifiers now take an additional 'del' parameter which is passed down all the way before reaching the fwd_rule_add() implementation. If a rule should be deleted, at that point, fwd_rule_del() is called instead.

Signed-off-by: Stefano Brivio --- conf.c | 26 ++++++-------- fwd_rule.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++------ fwd_rule.h | 4 ++- pesto.1 | 85 +++++++++++++++++++++++++++++++++++++++++++++ pesto.c | 55 ++++++++++++++++++++++++++--- 5 files changed, 238 insertions(+), 32 deletions(-)

diff --git a/conf.c b/conf.c index d7c837d..929a889 100644 --- a/conf.c +++ b/conf.c @@ -1849,16 +1849,16 @@ void conf(struct ctx *c, int argc, char **argv)

if (name == 't') { opt_t = true; - fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]); + fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]); } else if (name == 'u') { opt_u = true; - fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]); + fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]); } else if (name == 'T') { opt_T = true; - fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]); + fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]); } else if (name == 'U') { opt_U = true; - fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]); + fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]); } } while (name != -1);

@@ -1910,13 +1910,13 @@ void conf(struct ctx *c, int argc, char **argv)

if (c->mode == MODE_PASTA) { if (!opt_t) - fwd_rule_parse('t', "auto", c->fwd[PIF_HOST]); + fwd_rule_parse('t', false, "auto", c->fwd[PIF_HOST]); if (!opt_T) - fwd_rule_parse('T', "auto", c->fwd[PIF_SPLICE]); + fwd_rule_parse('T', false, "auto", c->fwd[PIF_SPLICE]); if (!opt_u) - fwd_rule_parse('u', "auto", c->fwd[PIF_HOST]); + fwd_rule_parse('u', false, "auto", c->fwd[PIF_HOST]); if (!opt_U) - fwd_rule_parse('U', "auto", c->fwd[PIF_SPLICE]); + fwd_rule_parse('U', false, "auto", c->fwd[PIF_SPLICE]); }

conf_sock_listen(c); @@ -2135,14 +2135,8 @@ void conf_handler(struct ctx *c, uint32_t events) unsigned pif;

/* Clear pending tables */ - for (pif = 0; pif < PIF_NUM_TYPES; pif++) { - struct fwd_table *fwd = c->fwd_pending[pif]; - - if (!fwd) - continue; - fwd->count = 0; - fwd->sock_count = 0; - } + for (pif = 0; pif < PIF_NUM_TYPES; pif++) + fwd_rule_clear(c->fwd_pending[pif]);

/* FIXME: this could block indefinitely if the client doesn't * write as much as it should diff --git a/fwd_rule.c b/fwd_rule.c index 200f4b5..c886ef3 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -180,6 +180,75 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule * return true; }

+/** + * fwd_rule_clear() - Clear a forwarding table + * @fwd: Table to clear (might be NULL) + */ +void fwd_rule_clear(struct fwd_table *fwd) +{ + if (!fwd) + return; + + /* TODO: check that there are no open sockets in the table before + * going on. See also a related item in fwd_rule_del(). + */ + + fwd->count = 0; + fwd->sock_count = 0; +} + +/** + * fwd_rule_del() - Partially validate and delete a rule from a forwarding table + * @fwd: Table to delete from + * @rule: Rule to delete (must conflict with an existing rule) + * + * Return: 0 on success, negative error code on failure (-ENOENT if not found) + * + * NOTE: This function can't be used for a forwarding table with any open socket + * stored in fwd->rulesocks. + */ +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_rule *rule) +{ + unsigned num, i; + + for (i = 0; i < fwd->count; i++) { + /* FIXME: This isn't entirely correct, as ideally we would like + * to match only *matching* rules here, not just conflicting + * ones. This is convenient at the moment for the current + * implementation, though. + */ + if (fwd_rule_conflicts(rule, &fwd->rules[i])) + break; + } + + if (i == fwd->count) { + char newstr[FWD_RULE_STRLEN]; + + warn("Couldn't find forwarding rule to delete: %s", + fwd_rule_fmt(rule, newstr, sizeof(newstr))); + return -ENOENT; + } + + /* Don't use anything else from 'rule' as passed, it's not validated */ + rule = &fwd->rules[i];

So we remove the entire rule that matches, not only the part of the new rule that matches?

We could / should split ranges derived from a rule, yes, but it's not implemented here. For now, for simplicity, we'll just drop the whole thing.

Podman doesn't allow overlapping ranges between different containers anyway, so this isn't a practical problem anybody is going to hit in the short term. In the slightly longer term, we should definitely do better than this.

...

And what if a new rule conflicts with two existing rules?

David raised that concern in:

https://archives.passt.top/passt-dev/afsASt0TWXFQovjJ@zatzit/

...we'll delete the first one which conflicts, at the moment. It's not great but again it's fine for Podman usage, and we can improve the implementation in a bit.

...on the other hand, I just checked things with a trivial function just doing memcmp(), to implement David's proposal about sticking to exact matches only, and it all seems to work (with or without interface names, with or without IPv4 / IPv6 mix-ups, etc.). I didn't expect it to be so simple. Let me change that in v10 as well. -- Stefano