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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 6 May 2026 14:07:37 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Wed, 6 May 2026 13:43:07 +0200
> Laurent Vivier <lvivier@redhat.com> 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 <sbrivio@redhat.com>
> > > ---
> > >   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:
> 
>   <afsASt0TWXFQovjJ@zatzit>
>   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