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=XRkDXla/; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 6B70D5A0265 for ; Wed, 06 May 2026 10:48:19 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1778057295; bh=qjb2gV7ULexbyhe5F7o9zlYJbYZoNiK2l2M+ru825Co=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XRkDXla/X7FfLYL46LE1ManhGlB2vNt2PlAtWBuBC/BqXTpOmYXQqJny+hAADVdGj 3iivqwAAL6AlBrXc0Av4wmCBW0cXMVhmCJSKPRavGF62rowgFiRNzpeqSebf4D5crI rhJ1Vq7p0fHZXiI5zOcv8MQ+QHv0y7cRZ7LA0JRw7kbyeAIDGxG/JwydmVFG6iFjLm tMAO2/T9IfDY9Xn/HG5+WXfpfohFuTX8Y3pMyfckvz2gTv0QXqYXzQjZtkx7RZG7hJ 99Jk4YncDhDikH/JL6diyoXw2L7clbwy1LKKJ0H4Y8RiMy/LnoFK5lyTqFBSXZRut1 FDswwfpZIZQgg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g9TYW6VXJz4wLQ; Wed, 06 May 2026 18:48:15 +1000 (AEST) Date: Wed, 6 May 2026 18:48:10 +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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="c07Yze1Yb9uSxmQU" Content-Disposition: inline In-Reply-To: <20260506102220.66a5c7a5@elisabeth> Message-ID-Hash: IURC4DLTENMWM6JZD6ITNHTQ45ZTY3SN X-Message-ID-Hash: IURC4DLTENMWM6JZD6ITNHTQ45ZTY3SN 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: --c07Yze1Yb9uSxmQU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > > > Instead of just being able to replace the existing forwarding table, = =20 > >=20 > > As of my last version, we already added, rather than replacing. >=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 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. > > >=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 **arg= v) > > > =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 **arg= v) > > > =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 even= ts) > > > 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 > 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 forw= arding 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 val= id 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 > 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. > > 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 > 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. That's > > not very clear from the description of @rule. >=20 > 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). =2E.. 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. 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. > > > + > > > + if (i =3D=3D 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 validate= d */ > > > + rule =3D &fwd->rules[i]; > > > + num =3D (unsigned)rule->last - rule->first + 1; > > > + > > > + fwd->count--; > > > + > > > + memmove((void *)(fwd->rulesocks + i), (void *)(fwd->rulesocks + i += 1), =20 > >=20 > > I don't think the explicit (void *) casts are necessary - they should > > be implicit from memmove()s signature. >=20 > They aren't from a C point of view, but clang-tidy reports a > bugprone-multi-level-implicit-pointer-conversion warning if I don't do > that, because it's not obvious that we want the same kind of cast as the > implicit one. Ah, ok. > > > + (fwd->count - i) * sizeof(*fwd->rulesocks)); =20 > >=20 > > Is memmove() guaranteed to be safe if given a zero length? That will > > occur if deleting the last entry. >=20 > In practice, this is quite commonly done in the Linux kernel and > elsewhere and I never hit issues, against different C libraries. Ok. > I'm actually not sure what issue I should get, C11 says that it "copies > n characters from the object pointed to by s2 into the object pointed to > by s1", and if "n" is 0, nothing is done. >=20 > There's no specific mention of that in C11, but I don't think there > needs to be one, unlike with realloc() with a zero size and possibly > others problematic cases. >=20 > So, yes, as far as I know it's "safe", but with no explicit guarantee > (and I don't think any is needed). >=20 > > > + /* TODO: move sockets stored starting from fwd->rulesocks[i + i], s= hould > > > + * we ever need to delete rules from a table with open sockets. > > > + */ > > > + fwd->sock_count -=3D num; > > > + > > > + memmove(fwd->rules + i, fwd->rules + i + 1, > > > + (fwd->count - i) * sizeof(*fwd->rules) =20 > >=20 > > Again, is this safe if i =3D=3D fwd->count? >=20 > Same as above. >=20 > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * fwd_rule_add() - Validate and add a rule to a forwarding table > > > * @fwd: Table to add to > > > @@ -368,6 +428,7 @@ static int parse_keyword(const char *s, const cha= r **endptr, const char *kw) > > > * fwd_rule_range_except() - Set up forwarding for a range of ports = minus a > > > * bitmap of exclusions > > > * @fwd: Forwarding table to be updated > > > + * @del: Delete resulting rules from forwarding table, instead of ad= ding =20 > >=20 > > Clunky, but it gets the job done. > >=20 > > > * @proto: Protocol to forward > > > * @addr: Listening address > > > * @ifname: Listening interface > > > @@ -377,8 +438,8 @@ static int parse_keyword(const char *s, const cha= r **endptr, const char *kw) > > > * @to: Port to translate @first to when forwarding > > > * @flags: Flags for forwarding entries > > > */ > > > -static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t pro= to, > > > - const union inany_addr *addr, > > > +static void fwd_rule_range_except(struct fwd_table *fwd, bool del, > > > + uint8_t proto, const union inany_addr *addr, > > > const char *ifname, > > > uint16_t first, uint16_t last, > > > const uint8_t *exclude, uint16_t to, > > > @@ -418,8 +479,13 @@ static void fwd_rule_range_except(struct fwd_tab= le *fwd, uint8_t proto, > > > rule.last =3D i - 1; > > > rule.to =3D base + delta; > > > =20 > > > - if (fwd_rule_add(fwd, &rule) < 0) > > > - goto fail; > > > + if (del) { > > > + if (fwd_rule_del(fwd, &rule) < 0) > > > + goto fail; > > > + } else { > > > + if (fwd_rule_add(fwd, &rule) < 0) > > > + goto fail; > > > + } > > > =20 > > > base =3D i - 1; > > > } > > > @@ -445,12 +511,13 @@ fail: > > > /** > > > * fwd_rule_parse_ports() - Parse port range(s) specifier > > > * @fwd: Forwarding table to be updated > > > + * @del: Delete resulting rules from forwarding table, instead of ad= ding > > > * @proto: Protocol to forward > > > * @addr: Listening address for forwarding > > > * @ifname: Interface name for listening > > > * @spec: Port range(s) specifier > > > */ > > > -static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t prot= o, > > > +static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, ui= nt8_t proto, > > > const union inany_addr *addr, > > > const char *ifname, > > > const char *spec) > > > @@ -507,7 +574,7 @@ static void fwd_rule_parse_ports(struct fwd_table= *fwd, uint8_t proto, > > > /* Exclude ephemeral ports */ > > > fwd_port_map_ephemeral(exclude); > > > =20 > > > - fwd_rule_range_except(fwd, proto, addr, ifname, > > > + fwd_rule_range_except(fwd, del, proto, addr, ifname, > > > 1, NUM_PORTS - 1, exclude, > > > 1, flags | FWD_WEAK); > > > return; > > > @@ -537,7 +604,7 @@ static void fwd_rule_parse_ports(struct fwd_table= *fwd, uint8_t proto, > > > if (p !=3D ep) /* Garbage after the ranges */ > > > goto bad; > > > =20 > > > - fwd_rule_range_except(fwd, proto, addr, ifname, > > > + fwd_rule_range_except(fwd, del, proto, addr, ifname, > > > orig_range.first, orig_range.last, > > > exclude, > > > mapped_range.first, flags); > > > @@ -551,10 +618,12 @@ bad: > > > /** > > > * fwd_rule_parse() - Parse port configuration option > > > * @optname: Short option name, t, T, u, or U > > > + * @del: Delete resulting rules from forwarding table, instead of ad= ding > > > * @optarg: Option argument (port specification) > > > * @fwd: Forwarding table to be updated > > > */ > > > -void fwd_rule_parse(char optname, const char *optarg, struct fwd_tab= le *fwd) > > > +void fwd_rule_parse(char optname, bool del, const char *optarg, > > > + struct fwd_table *fwd) > > > { > > > union inany_addr addr_buf =3D inany_any6, *addr =3D &addr_buf; > > > char buf[BUFSIZ], *spec, *ifname =3D NULL; > > > @@ -632,12 +701,12 @@ void fwd_rule_parse(char optname, const char *o= ptarg, struct fwd_table *fwd) > > > optname, optarg); > > > =20 > > > if (fwd->caps & FWD_CAP_IPV4) { > > > - fwd_rule_parse_ports(fwd, proto, > > > + fwd_rule_parse_ports(fwd, del, proto, > > > &inany_loopback4, NULL, > > > spec); > > > } > > > if (fwd->caps & FWD_CAP_IPV6) { > > > - fwd_rule_parse_ports(fwd, proto, > > > + fwd_rule_parse_ports(fwd, del, proto, > > > &inany_loopback6, NULL, > > > spec); > > > } > > > @@ -653,7 +722,7 @@ void fwd_rule_parse(char optname, const char *opt= arg, struct fwd_table *fwd) > > > optname, optarg); > > > } > > > =20 > > > - fwd_rule_parse_ports(fwd, proto, addr, ifname, spec); > > > + fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec); > > > } > > > =20 > > > /** > > > diff --git a/fwd_rule.h b/fwd_rule.h > > > index f43b37d..ae9a3cb 100644 > > > --- a/fwd_rule.h > > > +++ b/fwd_rule.h > > > @@ -100,9 +100,11 @@ void fwd_probe_ephemeral(void); > > > =20 > > > const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule); > > > const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, siz= e_t size); > > > -void fwd_rule_parse(char optname, const char *optarg, struct fwd_tab= le *fwd); > > > +void fwd_rule_parse(char optname, bool del, const char *optarg, > > > + struct fwd_table *fwd); > > > int fwd_rule_read(int fd, struct fwd_rule *rule); > > > int fwd_rule_write(int fd, const struct fwd_rule *rule); > > > +void fwd_rule_clear(struct fwd_table *fwd); > > > int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new); > > > =20 > > > /** > > > diff --git a/pesto.1 b/pesto.1 > > > index 1ea1d12..cd0f3dc 100644 > > > --- a/pesto.1 > > > +++ b/pesto.1 > > > @@ -31,6 +31,42 @@ Be verbose. > > > .BR \-h ", " \-\-help > > > Display a help message and exit. > > > =20 > > > +.TP > > > +.BR \-A ", " \-\-add > > > +Add the port forwarding specifiers following this option to the curr= ent > > > +forwarding table, rather than replacing it. > > > + > > > +This option can be given multiple times, as it might follow previous= deletions > > > +(see \fB--delete\fR below), and implies that all the specifiers foll= owing it, > > > +before a further \fB--delete\fR option occurs, will be handled as ad= ditions. > > > + > > > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOT= ES\fR for > > > +more details. > > > + > > > +.TP > > > +.BR \-D ", " \-\-delete > > > +Delete the port forwarding specifiers following this option from the= current > > > +forwarding table, rather than adding them it. > > > + > > > +This option can be given multiple times, as it might follow previous= additions > > > +(see \fB--add\fR above), and implies that all the specifiers followi= ng it, > > > +before a further \fB--add\fR option occurs, will be handled as delet= ions. > > > + > > > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOT= ES\fR for > > > +more details. > > > + > > > +.TP > > > +.BR \-C ", " \-\-clear " " \fIpif > > > +Clear the forwarding table associated to a given \fIpif\fR, that is,= a > > > +conceptual type of interface in \fBpasst\fR(1) or \fBpasta\fR(1) rep= resenting a > > > +specific data path and direction. > > > + > > > +The available \fIpif\fR names can be obtained by querying the curren= t forwarding > > > +configuration, which can be done by calling \fBpesto\fR(1) without o= ptions. > > > + > > > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOT= ES\fR for > > > +more details. > > > + > > > .TP > > > .BR \-t ", " \-\-tcp-ports " " \fIspec > > > Configure TCP port forwarding to guest or namespace. \fIspec\fR can = be one of: > > > @@ -162,6 +198,55 @@ Configure UDP port forwarding from target namesp= ace to init namespace. > > > .BR \-\-version > > > Show version and exit. > > > =20 > > > +.SH NOTES > > > + > > > +.SS Adding, deleting, clearing rules > > > + > > > +The options \fB--add\fR, \fB--delete\fR, and \fB--clear\fR are handl= ed as > > > +sequential commands to manipulate the current forwarding tables. If = none of them > > > +is given, forwarding specifiers for a given table are intended as re= placement of > > > +the corresponding table. That is: =20 > >=20 > > I thought we wanted to default to add, rather than replace. That > > seems both a little simpler to implement and agruably more likely to > > be what peopke want. >=20 > We previously discussed we would default to replace for brevity, Oh, ok. Apparently I'd forgotten. > because otherwise users would need to explicitly clear tables, but also > because it's consistent with: >=20 > - the idea that we replace the current table >=20 > - the existing command line syntax that just "programs" a new table. >=20 > That is, the cost of typing: >=20 > -A -t 22 >=20 > is marginally higher compared to: >=20 > -t 22 >=20 > but: >=20 > -C HOST -t 22 >=20 > in case a replacement is desired looks substantially more to me. Ok, fair enough. > It's really not that important at the moment as Podman will anyway > explicitly pass options. Of course it's not a great idea to change this > later for non-automated users, but in any case I think the reason I > explained above (and we discussed in the past) are still valid. >=20 > > > +.nf > > > + pesto -t 1024 -U 1025 > > > +.fi > > > + > > > +will \fBreplace\fR the current TCP inbound port forwarding table wit= h a single > > > +rule, forwarding port 1024, and will similarly replace the UDP outbo= und > > > +forwarding table with a single forwarding rule for port 1025. This u= sage is a > > > +short-hand form for: > > > + > > > +.nf > > > + pesto -C HOST -t 1024 -C SPLICE -U 1025 > > > +.fi > > > + > > > +The options \fB--add\fR and \fB--delete\fR are used to \fBadd new sp= ecific > > > +rules or delete existing ones\fR, instead of replacing tables. For e= xample: > > > + > > > +.nf > > > + pesto -A -t 2000 -D -t 3000 -U 5000 > > > +.fi > > > + > > > +will add a forwarding rule for inbound TCP port 2000, and delete inb= ound TCP > > > +port 3000 as well as outbound UDP port 5000 from the existing set of= rules. > > > + > > > +All these options are interpreted as sequential commands and can be = arbitrarily > > > +combined. For example: > > > + > > > +.nf > > > + pesto -A -t 2000 -C HOST -A -T 3000 -t 2001 -D -u 5000 > > > +.fi > > > + > > > +will, in order: > > > + > > > +.RS > > > +- add inbound TCP port 2000 > > > +- clear inbound ports, reverting the addition above > > > +- add outbound TCP port 3000 > > > +- add inbound TCP port 2001 > > > +- delete inbound UDP port 5000 > > > +.RE > > > + > > > .SH AUTHORS > > > =20 > > > Stefano Brivio , > > > diff --git a/pesto.c b/pesto.c > > > index 73fdc39..143d4c6 100644 > > > --- a/pesto.c > > > +++ b/pesto.c > > > @@ -55,6 +55,9 @@ static void usage(const char *name, FILE *f, int st= atus) > > > FPRINTF(f, "Usage: %s [OPTION]... PATH\n", name); > > > FPRINTF(f, > > > "\n" > > > + " -A, --add Add following specifiers to forwards\n" > > > + " -D, --delete Delete following specifiers instead\n" > > > + " -C, --clear PIF Clear forwarding table for given PIF\n" > > > " -t, --tcp-ports SPEC TCP inbound port forwarding\n" > > > " can be specified multiple times\n" > > > " SPEC can be:\n" > > > @@ -298,6 +301,9 @@ int main(int argc, char **argv) > > > {"debug", no_argument, NULL, 'd' }, > > > {"help", no_argument, NULL, 'h' }, > > > {"version", no_argument, NULL, 1 }, > > > + {"add", no_argument, NULL, 'A' }, > > > + {"delete", no_argument, NULL, 'D' }, > > > + {"clear", required_argument, NULL, 'C' }, > > > {"tcp-ports", required_argument, NULL, 't' }, > > > {"udp-ports", required_argument, NULL, 'u' }, > > > {"tcp-ns", required_argument, NULL, 'T' }, > > > @@ -305,9 +311,11 @@ int main(int argc, char **argv) > > > {"show", no_argument, NULL, 's' }, > > > { 0 }, > > > }; > > > + enum { MODE_CLEAR, MODE_ADD, MODE_DEL } mode =3D MODE_CLEAR; =20 > >=20 > > MODE_CLEAR doesn't make sense to me. Unlike add or del, clear is a > > once-off operation, it's not clear to me how it would affect the > > interpretation of -[tTuU]. >=20 > It needs to be a mode, and the default one, to make sure we default to > replacing tables for a given direction of forwarding. >=20 > It's not exactly one-off as it's a starting mode until -A or -D are > given. The interpretation is as described in the man page. Yeah, ok, it's necessary to support the replace-by-default semantics. > > > + bool inbound_cleared =3D false, outbound_cleared =3D false; > > > struct pif_configuration *inbound, *outbound; > > > + const char *optstring =3D "dhADC:t:u:T:U:s"; > > > struct sockaddr_un a =3D { AF_UNIX, "" }; > > > - const char *optstring =3D "dht:u:T:U:s"; > > > struct configuration conf =3D { 0 }; > > > bool update =3D false, show =3D false; > > > struct pesto_hello hello; > > > @@ -339,11 +347,16 @@ int main(int argc, char **argv) > > > case -1: > > > case 0: > > > break; > > > + case 'C': > > > case 't': > > > case 'u': > > > case 'T': > > > case 'U': > > > - /* Parse these options after we've read state from passt/pasta */ > > > + case 'A': > > > + case 'D': > > > + /* Parse these options after we've read state from > > > + * passt/pasta > > > + */ > > > update =3D true; > > > break; > > > case 's': > > > @@ -439,13 +452,38 @@ int main(int argc, char **argv) > > > optname =3D getopt_long(argc, argv, optstring, options, NULL); > > > =20 > > > switch (optname) { > > > + case 'A': > > > + mode =3D MODE_ADD; > > > + break; > > > + case 'D': > > > + mode =3D MODE_DEL; > > > + break; > > > + case 'C': > > > + if (!strcmp(optarg, "HOST")) { > > > + fwd_rule_clear(&inbound->fwd); > > > + inbound_cleared =3D true; > > > + } else if (!strcmp(optarg, "SPLICE")) { > > > + fwd_rule_clear(&outbound->fwd); =20 > >=20 > > outbound will be NULL if talking to passt, so this could SEGV. >=20 > Ah, right, nice catch, I'll fix that in v9. In principle 'inbound' could also be NULL, although not with any current passt version. Generalising as mentioned below should generalise the fix as well though. > > > + outbound_cleared =3D true; > > > + } else { > > > + die("Unsupported pif name %s", optarg); > > > + } =20 > >=20 > > For the time being pesto is limited to a single "inbound" and single > > "outbound" table, simply because we haven't devised syntax for > > anything else. However, we don't actually need that for --clear. > > Since it already takes a pif name we can use pif_conf_by_name() to > > clear an arbitrary named pif's rules. >=20 > I'll change that in v9, assuming it works (I haven't tried yet). >=20 > > > + > > > + break; > > > case 't': > > > case 'u': > > > if (!inbound) { > > > die("Can't use -%c, no inbound interface", > > > optname); > > > } > > > - fwd_rule_parse(optname, optarg, &inbound->fwd); > > > + > > > + if (mode =3D=3D MODE_CLEAR && !inbound_cleared) { > > > + fwd_rule_clear(&inbound->fwd); > > > + inbound_cleared =3D true; > > > + } > > > + > > > + fwd_rule_parse(optname, mode =3D=3D MODE_DEL, optarg, > > > + &inbound->fwd); > > > break; > > > case 'T': > > > case 'U': > > > @@ -453,7 +491,14 @@ int main(int argc, char **argv) > > > die("Can't use -%c, no outbound interface", > > > optname); > > > } > > > - fwd_rule_parse(optname, optarg, &outbound->fwd); > > > + > > > + if (mode =3D=3D MODE_CLEAR && !outbound_cleared) { > > > + fwd_rule_clear(&outbound->fwd); > > > + outbound_cleared =3D true; > > > + } > > > + > > > + fwd_rule_parse(optname, mode =3D=3D MODE_DEL, optarg, > > > + &inbound->fwd); > > > break; > > > default: > > > continue; > > > --=20 > > > 2.43.0 > > > =20 > >=20 >=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 --c07Yze1Yb9uSxmQU Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn7ADsACgkQzQJF27ox 2Gfm5Q/9H6g0zFV8aeOWtSOAXzPerltgIi81cDBSKcB0vvi1WsgXp/shdNlByvAq zJlMGP5yT2r5L1mIgpllX/gOSsnryKK9vmYo8b1Izg7dUxc48QiUl21YeZjlGidy llZEww2DmdZHC2Hl/J5lEt5mIpEFtXVeinBeGdxEXzdcq+4llsFi+gStjdecR0MK 4kBT9eGNqDjZ13Snzs3VfnpATugkpV2FV26x5iMtgWuSdBUymL6086D1eGPuT7DP ALZZ6NC/yNFtdmFIY+tD1XUHvOkv6U65oLdAhWgOn5A1Qcf22AHNWwYyG0xTccc0 HrYAnmO6wNTNUWRkVm2MTIq3aDEgBapBz2JrJaoQ4ol3xD7beWDr6GDyJfDaMFsd IwZKu75KmyKynv1mnED8QFkPyjH5E39Dgfq8GHs9Pz7E6z0gWq35YzX+wCIREwWr IqRyx9vHygJMpzcZdkehuUsLsF9T5TCN6oTkYepwdADAdGxGYyuomUtYeHUrSYBv mnuc9z4konGAtATeTG7cE/u33bKKohbd9sr3tjCgJFWdWp6qWP0ikhvh+fHTSDzp g6UkT4NfTDgEVPSBTy4F1JZa3MEY6e62NsYfyCh2EISaGYNu5/7+gRSXoslta8xo Q6oK2SSz4cRtrXO+8PNfOdcW9C+bJFydCVUOEC2B7Pxxu2YZRE8= =6irY -----END PGP SIGNATURE----- --c07Yze1Yb9uSxmQU--