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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9927 bytes --]

On Wed, May 06, 2026 at 10:56:02AM +0200, Stefano Brivio wrote:
> On Wed, 6 May 2026 18:48:10 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, May 06, 2026 at 10:22:20AM +0200, Stefano Brivio wrote:
> > > On Wed, 6 May 2026 16:45:27 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Wed, May 06, 2026 at 01:47:19AM +0200, Stefano Brivio wrote:  
> > > > > Instead of just being able to replace the existing forwarding table,    
> > > > 
> > > > As of my last version, we already added, rather than replacing.  
> > > 
> > > Right, I noticed that, but this isn't the default behaviour we
> > > discussed, so I assumed it was accidental, and planned to go back and
> > > check the reason why.
> > > 
> > > Given that it wasn't accidental, I'll simply adjust this part of the
> > > commit message.
> > >   
> > > > > implement --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 | 91 +++++++++++++++++++++++++++++++++++++++++++++++-------
> > > > >  fwd_rule.h |  4 ++-
> > > > >  pesto.1    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  pesto.c    | 53 ++++++++++++++++++++++++++++---
> > > > >  5 files changed, 227 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/conf.c b/conf.c
> > > > > index 3f48793..909c34c 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 03e8e80..eb9a601 100644
> > > > > --- a/fwd_rule.c
> > > > > +++ b/fwd_rule.c
> > > > > @@ -180,6 +180,66 @@ 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;
> > > > > +    
> > > > 
> > > > Not essential, but I wonder if it would be wise to verify that there
> > > > are no currently open sockets associated with any of the rules.  
> > > 
> > > With a loop, I suppose. I can add it as a TODO comment because I guess
> > > it would be good to handle that case (open sockets left) for
> > > fwd_rule_del() as well, and a part of the implementation can probably
> > > be common.
> > >   
> > > > > +	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 match 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 valid sockets
> > > > > + * stored in fwd->rulesocks.    
> > > > 
> > > > The exact meaning of this isn't very clear to me.  Does "valid" mean
> > > > "open" or something else?  
> > > 
> > > It means valid at some point, not necessarily open right now. I'll
> > > change it to "open" for clarity.  
> > 
> > I'm not sure what "valid at some point" means, either.
> 
> That it was a valid socket file descriptor (an open one) at some point.
> 
> > > > I think what you're getting at is that every entry in fwd->socks[]
> > > > must be -1.  Or at least every entry with index in [0,sock_count)  
> > > 
> > > Yes.
> > >   
> > > > > + */
> > > > > +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_rule *rule)
> > > > > +{
> > > > > +	unsigned num, i;
> > > > > +
> > > > > +	for (i = 0; i < fwd->count; i++) {
> > > > > +		if (fwd_rule_conflicts(rule, &fwd->rules[i]))
> > > > > +			break;
> > > > > +	}    
> > > > 
> > > > So, this deletes any conflicting rule, not only exact matches.  That's
> > > > not very clear from the description of @rule.  
> > > 
> > > It deletes the first one  
> > 
> > Oh, good point.  Which actually elevates this to a bug, not just a
> > debate about the best semantics, because...
> > 
> > > (but given that fwd_rule_conflicts() is called
> > > on insertion, there should be a single one).  
> > 
> > ... that's not correct.  "conflicts" is not transitive, so (for
> > example) in the cases below:
> > 	-t 1000-2000 -t 4000-5000 --delete -t 500-5500
> > 	-t 127.0.0.1/100 -t 127.0.0.2/100 --delete -t 100
> > The deleted rule conflicts with both the added rules, but they don't
> > conflict with each other.
> 
> Right, yes, for partially overlapping rules that's true. But that's not
> what Podman needs right now, so I think it can be fixed later.

The second example involves no ranges at all.

> > I don't think "delete all conflicting rules" is a great either, since
> > it means that:
> > 	-t 1000-1999 -t 2000-2999 --delete -t 1500-2500
> > maps nothing at all, which seems pretty surprising.
> > 
> > > It's good enough for our purposes right now, even though we might want
> > > to make that more sophisticated in the future. I'll change the
> > > description of @rule.  
> > 
> > I really think the current behaviour is too confusing.  Only deleting
> > exact matches (and giving an error if there's a conflict that's not an
> > exact match) I think *is* good enough for now, so that's what I'd
> > suggest.
> 
> ...except that it's not implemented by any function and it's not exactly
> trivial either, and delaying the implementation further makes this
> useless (at least for Podman, which we can approximate to "essentially
> useless"), so I'd rather go with something that doesn't take care of
> partially overlapping ranges, rather than no feature at all.
> 
> -- 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]