From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=aQh5fXbj; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id D38305A0265 for ; Thu, 07 May 2026 00:20:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1778106036; bh=JAGphNl4H6tro27D8cCE1V51YNNUM2vO/p5tO6UR360=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=aQh5fXbjVX818/061YRwWS70ZeVK+W4mCJ3Jabb8dgMFjy1XCwkb9rIjeexjxVrXm WJuAbZIUOqTFPfLcdO6P6oTx8AaN5yLSOereiDoTeY7nXB0yD6+Qubnn9/A6W2uG8i jHtdnO4tbanu2BwwJM23U0gVbvvV9k5XBvGIUq348kQ+95SSyhlC8txxki5OzeBCja c0+I/oGSwlY5jBXN1DbQE+a64moGoY38pUeWZ68Njq3a/bUZqXVoOJLsCZQ1vh6aF4 82pSrdF3MAm3CrEPXLR7yORTz2O9TbWJJedSSCUvOl8JDho+d4E2GM5NsGbShQxT59 PVwc+SsnNiKCQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g9qZr5hvrz4wJg; Thu, 07 May 2026 08:20:36 +1000 (AEST) Date: Wed, 6 May 2026 19:22:00 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v8 19/19] pesto, conf, fwd_rule: Add options and modes to add, delete, clear rules Message-ID: References: <20260505234719.1437340-1-sbrivio@redhat.com> <20260505234719.1437340-20-sbrivio@redhat.com> <20260506102220.66a5c7a5@elisabeth> <20260506105601.5bfa57c7@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="cQ43nTycQvupJTZZ" Content-Disposition: inline In-Reply-To: <20260506105601.5bfa57c7@elisabeth> Message-ID-Hash: Q7QQXD22FW5VAJK2QRSG6BHQBEOENTH6 X-Message-ID-Hash: Q7QQXD22FW5VAJK2QRSG6BHQBEOENTH6 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Jon Maloy , Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --cQ43nTycQvupJTZZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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: > > > =20 > > > > On Wed, May 06, 2026 at 01:47:19AM +0200, Stefano Brivio wrote: =20 > > > > > Instead of just being able to replace the existing forwarding tab= le, =20 > > > >=20 > > > > As of my last version, we already added, rather than replacing. =20 > > >=20 > > > 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. > > >=20 > > > Given that it wasn't accidental, I'll simply adjust this part of the > > > commit message. > > > =20 > > > > > implement --add and --delete options to maintain the table and add > > > > > or delete specific ports. > > > > >=20 > > > > > The option --clear PIF forces the clearing of a table, instead. > > > > >=20 > > > > > These options can be combined arbitrarily and are handled as > > > > > sequential commands, as now described in pesto(1). > > > > >=20 > > > > > If no option is given before forwarding specifiers for a matching > > > > > table, the command line is interpreted as a replacement of the > > > > > existing rules. > > > > >=20 > > > > > To this end: > > > > >=20 > > > > > - there's no protocol change, as pesto is anyway sending updated > > > > > copies of the table > > > > >=20 > > > > > - the forwarding table functions now include a new fwd_rule_del(), > > > > > which deletes existing rule only if a matching one is found > > > > >=20 > > > > > - a trivial fwd_rule_clear() is factored out from the existing > > > > > conf_handler() implementation, so that it can be directly used > > > > > in pesto > > > > >=20 > > > > > The entry points for parsing of port specifiers now take an addit= ional > > > > > 'del' parameter which is passed down all the way before reaching = the > > > > > fwd_rule_add() implementation. If a rule should be deleted, at th= at > > > > > point, fwd_rule_del() is called instead. > > > > >=20 > > > > > 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(-) > > > > >=20 > > > > > 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) > > > > > =20 > > > > > if (name =3D=3D 't') { > > > > > opt_t =3D true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]); > > > > > } else if (name =3D=3D 'u') { > > > > > opt_u =3D true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]); > > > > > } else if (name =3D=3D 'T') { > > > > > opt_T =3D true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]); > > > > > } else if (name =3D=3D 'U') { > > > > > opt_U =3D true; > > > > > - fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]); > > > > > + fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]); > > > > > } > > > > > } while (name !=3D -1); > > > > > =20 > > > > > @@ -1910,13 +1910,13 @@ void conf(struct ctx *c, int argc, char *= *argv) > > > > > =20 > > > > > if (c->mode =3D=3D 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]); > > > > > } > > > > > =20 > > > > > conf_sock_listen(c); > > > > > @@ -2135,14 +2135,8 @@ void conf_handler(struct ctx *c, uint32_t = events) > > > > > unsigned pif; > > > > > =20 > > > > > /* Clear pending tables */ > > > > > - for (pif =3D 0; pif < PIF_NUM_TYPES; pif++) { > > > > > - struct fwd_table *fwd =3D c->fwd_pending[pif]; > > > > > - > > > > > - if (!fwd) > > > > > - continue; > > > > > - fwd->count =3D 0; > > > > > - fwd->sock_count =3D 0; > > > > > - } > > > > > + for (pif =3D 0; pif < PIF_NUM_TYPES; pif++) > > > > > + fwd_rule_clear(c->fwd_pending[pif]); > > > > > =20 > > > > > /* 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; > > > > > } > > > > > =20 > > > > > +/** > > > > > + * 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; > > > > > + =20 > > > >=20 > > > > 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. =20 > > >=20 > > > 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. > > > =20 > > > > > + fwd->count =3D 0; > > > > > + fwd->sock_count =3D 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. =20 > > > >=20 > > > > The exact meaning of this isn't very clear to me. Does "valid" mean > > > > "open" or something else? =20 > > >=20 > > > It means valid at some point, not necessarily open right now. I'll > > > change it to "open" for clarity. =20 > >=20 > > I'm not sure what "valid at some point" means, either. >=20 > That it was a valid socket file descriptor (an open one) at some point. >=20 > > > > 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) = =20 > > >=20 > > > Yes. > > > =20 > > > > > + */ > > > > > +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_= rule *rule) > > > > > +{ > > > > > + unsigned num, i; > > > > > + > > > > > + for (i =3D 0; i < fwd->count; i++) { > > > > > + if (fwd_rule_conflicts(rule, &fwd->rules[i])) > > > > > + break; > > > > > + } =20 > > > >=20 > > > > So, this deletes any conflicting rule, not only exact matches. Tha= t's > > > > not very clear from the description of @rule. =20 > > >=20 > > > It deletes the first one =20 > >=20 > > Oh, good point. Which actually elevates this to a bug, not just a > > debate about the best semantics, because... > >=20 > > > (but given that fwd_rule_conflicts() is called > > > on insertion, there should be a single one). =20 > >=20 > > ... 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. >=20 > 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. > >=20 > > > 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. =20 > >=20 > > 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. >=20 > ...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. >=20 > --=20 > Stefano >=20 --=20 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 --cQ43nTycQvupJTZZ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn7CCsACgkQzQJF27ox 2GfodQ//TgyPMzWgPfstLWuUC2gu5PScvoNjYOPHFw3FF/r163ceJlBBAU/hVn5U yyqjxOHmNwj9n+Ye9l85tfGhyC8xqV5a0iUZm8ztJDJh6WDBFM2yvvDG6wFDMV91 6ncG3cqlR+zd37ZiYbeYWYu72dEVZUIDZe0SkjY4WzCpFPO7tvnHEGHT4Is8ffWL q6JQgaQ8rzVL9upxoiVkmXz8p9XpV671KSoEQ+8sm7L7DaM8K5jZNciOsIW6jIJS F+SydrVy79NROQjYR2S6S2ff1n+/R2aTLm56fsUwXE8V0kaUE+eDU3Tg6Rhzlaky D9MonHuuBc3ums7+JdcVM37z+XwseKy8mxtBj0zXrjI25vW4Ezj43Q6lkMQMujyK znz2AnppGxo4tlfCFFLHAOoQx4/xE81V7545iZlZPR5IyQYpPhFA83Gi82Tc/IdY pQr9LcbekLy0HCOUYIeesoeXGQ6c4+OajZljKTr40rEv4He0Zyvr5wHrx6K0dSqt zyaz0oisGMA4eO3NNxvJ2NlutJAuh4WWT7wvdvfb1swUI9vXVwqwtOQFoVxLwgJH OjOYdlAtbm00en5jfsBG/csTOmdWeOojdV/mJlYYWz0pQ0pSHhgXkCX9Ou+6yhEU 5AjVEV7dxJXY2RtOTzQb/SX8fvYJzL/KKKwa36lWHetW/clglAA= =WzFf -----END PGP SIGNATURE----- --cQ43nTycQvupJTZZ--