On Wed, May 06, 2026 at 10:56:02AM +0200, Stefano Brivio wrote: > On Wed, 6 May 2026 18:48:10 +1000 > David Gibson 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 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 > > > > > --- > > > > > 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