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 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.

> > +	num = (unsigned)rule->last - rule->first + 1;
> > +
> > +	fwd->count--;
> > +
> > +	memmove((void *)(fwd->rulesocks + i), (void *)(fwd->rulesocks + i + 1),
> > +		(fwd->count - i) * sizeof(*fwd->rulesocks));
> > +
> > +	/* TODO: move sockets stored starting from fwd->rulesocks[i + i], should  
> 
> In comment ^ "[i + 1]"

Oops, I'll fix that in v10.

> > +	 * we ever need to delete rules from a table with open sockets.
> > +	 */
> > +	fwd->sock_count -= num;
> > +
> > +	memmove(fwd->rules + i, fwd->rules + i + 1,
> > +		(fwd->count - i) * sizeof(*fwd->rules));
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * fwd_rule_add() - Validate and add a rule to a forwarding table
> >    * @fwd:	Table to add to
> > @@ -370,6 +439,7 @@ static int parse_keyword(const char *s, const char **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 adding
> >    * @proto:	Protocol to forward
> >    * @addr:	Listening address
> >    * @ifname:	Listening interface
> > @@ -379,8 +449,8 @@ static int parse_keyword(const char *s, const char **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 proto,
> > -				  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,
> > @@ -420,8 +490,13 @@ static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t proto,
> >   		rule.last = i - 1;
> >   		rule.to = base + delta;
> >   
> > -		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;
> > +		}
> >   
> >   		base = i - 1;
> >   	}
> > @@ -447,12 +522,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 adding
> >    * @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 proto,
> > +static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >   				 const union inany_addr *addr,
> >   				 const char *ifname,
> >   				 const char *spec)
> > @@ -509,7 +585,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t proto,
> >   		/* Exclude ephemeral ports */
> >   		fwd_port_map_ephemeral(exclude);
> >   
> > -		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;
> > @@ -539,7 +615,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t proto,
> >   		if (p != ep) /* Garbage after the ranges */
> >   			goto bad;
> >   
> > -		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);
> > @@ -553,10 +629,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 adding
> >    * @optarg:	Option argument (port specification)
> >    * @fwd:	Forwarding table to be updated
> >    */
> > -void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd)
> > +void fwd_rule_parse(char optname, bool del, const char *optarg,
> > +		    struct fwd_table *fwd)
> >   {
> >   	union inany_addr addr_buf = inany_any6, *addr = &addr_buf;
> >   	char buf[BUFSIZ], *spec, *ifname = NULL;
> > @@ -634,12 +712,12 @@ void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd)
> >   			     optname, optarg);
> >   
> >   			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);
> >   			}
> > @@ -655,7 +733,7 @@ void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd)
> >   		    optname, optarg);
> >   	}
> >   
> > -	fwd_rule_parse_ports(fwd, proto, addr, ifname, spec);
> > +	fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec);
> >   }
> >   
> >   /**
> > 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);
> >   
> >   const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule);
> >   const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size);
> > -void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *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);
> >   
> >   /**
> > diff --git a/pesto.1 b/pesto.1
> > index 1e1c0f3..c684dc6 100644
> > --- a/pesto.1
> > +++ b/pesto.1
> > @@ -35,6 +35,42 @@ Display a help message and exit.
> >   .BR \-s ", " \-\-show
> >   Show the forwarding configuration before and after changes are applied.
> >   
> > +.TP
> > +.BR \-A ", " \-\-add
> > +Add the port forwarding specifiers following this option to the current
> > +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 following it,
> > +before a further \fB--delete\fR option occurs, will be handled as additions.
> > +
> > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\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.  
> 
> "adding them to it"?

Fixing in v10.

> > +
> > +This option can be given multiple times, as it might follow previous additions
> > +(see \fB--add\fR above), and implies that all the specifiers following it,
> > +before a further \fB--add\fR option occurs, will be handled as deletions.
> > +
> > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\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) representing a
> > +specific data path and direction.
> > +
> > +The available \fIpif\fR names can be obtained by querying the current forwarding
> > +configuration, which can be done by calling \fBpesto\fR(1) without options.
> > +
> > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\fR for
> > +more details.
> > +
> >   .TP
> >   .BR \-t ", " \-\-tcp-ports " " \fIspec
> >   Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of:
> > @@ -166,6 +202,55 @@ Configure UDP port forwarding from target namespace to init namespace.
> >   .BR \-\-version
> >   Show version and exit.
> >   
> > +.SH NOTES
> > +
> > +.SS Adding, deleting, clearing rules
> > +
> > +The options \fB--add\fR, \fB--delete\fR, and \fB--clear\fR are handled as
> > +sequential commands to manipulate the current forwarding tables. If none of them
> > +is given, forwarding specifiers for a given table are intended as replacement of
> > +the corresponding table. That is:
> > +
> > +.nf
> > +	pesto -t 1024 -U 1025
> > +.fi
> > +
> > +will \fBreplace\fR the current TCP inbound port forwarding table with a single
> > +rule, forwarding port 1024, and will similarly replace the UDP outbound
> > +forwarding table with a single forwarding rule for port 1025. This usage 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 specific
> > +rules or delete existing ones\fR, instead of replacing tables. For example:
> > +
> > +.nf
> > +	pesto -A -t 2000 -D -t 3000 -U 5000
> > +.fi
> > +
> > +will add a forwarding rule for inbound TCP port 2000, and delete inbound 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
> >   
> >   Stefano Brivio <sbrivio@redhat.com>,
> > diff --git a/pesto.c b/pesto.c
> > index 73fdc39..c5ffbb5 100644
> > --- a/pesto.c
> > +++ b/pesto.c
> > @@ -55,6 +55,9 @@ static void usage(const char *name, FILE *f, int status)
> >   	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 = MODE_CLEAR;
> > +	bool inbound_cleared = false, outbound_cleared = false;
> >   	struct pif_configuration *inbound, *outbound;
> > +	const char *optstring = "dhADC:t:u:T:U:s";
> >   	struct sockaddr_un a = { AF_UNIX, "" };
> > -	const char *optstring = "dht:u:T:U:s";
> >   	struct configuration conf = { 0 };
> >   	bool update = false, show = 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 = true;
> >   			break;
> >   		case 's':
> > @@ -436,16 +449,43 @@ int main(int argc, char **argv)
> >   
> >   	optind = 0;
> >   	do {
> > +		struct pif_configuration *pif_to_clear;
> > +
> >   		optname = getopt_long(argc, argv, optstring, options, NULL);
> >   
> >   		switch (optname) {
> > +		case 'A':
> > +			mode = MODE_ADD;
> > +			break;
> > +		case 'D':
> > +			mode = MODE_DEL;
> > +			break;
> > +		case 'C':
> > +			if (!(pif_to_clear = pif_conf_by_name(&conf, optarg)))
> > +				die("Unsupported pif name %s", optarg);
> > +
> > +			fwd_rule_clear(&pif_to_clear->fwd);
> > +
> > +			if (!strcmp(optarg, "HOST"))
> > +				inbound_cleared = true;
> > +			else if (!strcmp(optarg, "SPLICE"))
> > +				outbound_cleared = true;
> > +
> > +			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 == MODE_CLEAR && !inbound_cleared) {
> > +				fwd_rule_clear(&inbound->fwd);
> > +				inbound_cleared = true;
> > +			}
> > +
> > +			fwd_rule_parse(optname, mode == MODE_DEL, optarg,
> > +				       &inbound->fwd);
> >   			break;
> >   		case 'T':
> >   		case 'U':
> > @@ -453,7 +493,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 == MODE_CLEAR && !outbound_cleared) {
> > +				fwd_rule_clear(&outbound->fwd);
> > +				outbound_cleared = true;
> > +			}
> > +
> > +			fwd_rule_parse(optname, mode == MODE_DEL, optarg,
> > +				       &inbound->fwd);  
> 
> Should be &outbound->fwd

Ouch, nice catch, I rewrote this part after testing -T / -U and forgot
to check it again. :( I'll fix this in v10 as well.

-- 
Stefano