Re: [PATCH v7 16/18] pesto, conf: Send updated rules from pesto back to passt/pasta

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

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

On Tue, May 05, 2026 at 09:53:04AM +0200, Laurent Vivier wrote:
> On 5/5/26 01:11, Stefano Brivio wrote:
> > From: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Extend pesto to send the updated rule configuration back to passt/pasta.
> > Extend passt/pasta to read the new configuration and store the new rules in
> > a "pending" table.   We don't yet attempt to activate them.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > [dwg: Based on an early draft from Stefano]
> > [sbrivio: Add redundant check on interface names being terminated in
> >   conf_recv_rules(), to make static checkers happy]
> > [sbrivio: Make conf_recv_rules() return -1 if fwd_rule_read() fails,
> >   as suggested by Jon Maloy]
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> But one comment below
> 
> > ---
> >   Makefile |  5 ---
> >   conf.c   | 94 ++++++++++++++++++++++++++++++++++++++++++++++++--------
> >   fwd.c    | 10 +++++-
> >   passt.h  |  2 ++
> >   pesto.c  | 35 +++++++++++++++++++++
> >   5 files changed, 127 insertions(+), 19 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index c746b55..ae755a0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -224,10 +224,6 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck
> >   	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
> >   passt.cppcheck: BASE_CPPFLAGS += -UPESTO
> > -passt.cppcheck: CPPCHECK_FLAGS += \
> > -	--suppress=unusedFunction:fwd_rule.c \
> > -	--suppress=staticFunction:fwd_rule.c \
> > -	--suppress=unusedFunction:serialise.c
> >   passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
> >   passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
> > @@ -238,7 +234,6 @@ pesto.cppcheck: CPPCHECK_FLAGS += \
> >   	--suppress=unusedFunction:inany.h \
> >   	--suppress=unusedFunction:inany.c \
> >   	--suppress=unusedFunction:ip.h \
> > -	--suppress=unusedFunction:fwd_rule.c \
> >   	--suppress=staticFunction:fwd_rule.c \
> >   	--suppress=unusedFunction:serialise.c
> >   pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > diff --git a/conf.c b/conf.c
> > index 5e4e81e..f035fd3 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1971,6 +1971,62 @@ static int conf_send_rules(const struct ctx *c, int fd)
> >   	return 0;
> >   }
> > +/**
> > + * conf_recv_rules() - Receive forwarding rules from configuration client
> > + * @c:		Execution context
> > + * @fd:		Socket to the client
> > + *
> > + * Return: 0 on success, -1 on failure
> > + */
> > +static int conf_recv_rules(const struct ctx *c, int fd)
> > +{
> > +	while (1) {
> > +		struct fwd_table *fwd;
> > +		struct fwd_rule r;
> > +		uint32_t count;
> > +		uint8_t pif;
> > +		unsigned i;
> > +
> > +		if (read_u8(fd, &pif))
> > +			return -1;
> > +
> > +		if (pif == PIF_NONE)
> > +			break;
> > +
> > +		if (pif >= ARRAY_SIZE(c->fwd_pending) ||
> > +		    !(fwd = c->fwd_pending[pif])) {
> > +			err("Received rules for non-existent table");
> > +			return -1;
> > +		}
> > +
> > +		if (read_u32(fd, &count))
> > +			return -1;
> > +
> > +		if (count > MAX_FWD_RULES) {
> > +			err("Received %"PRIu32" rules (maximum %u)",
> > +			    count, MAX_FWD_RULES);
> > +			return -1;
> > +		}
> > +
> > +		for (i = 0; i < count; i++) {
> > +			if (fwd_rule_read(fd, &r))
> > +				return -1;
> > +
> > +			if (r.ifname[sizeof(r.ifname) - 1]) {
> > +				err("Interface name was not NULL terminated");
> > +				return -1;
> > +			}
> > +			/* Redundant, to make static checkers happy */
> > +			r.ifname[sizeof(r.ifname) - 1] = '\0';
> > +
> > +			if (fwd_rule_add(fwd, &r) < 0)
> > +				return -1;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * conf_close() - Close configuration / control socket and clean up
> >    * @c:		Execution context
> > @@ -2075,21 +2131,33 @@ fail:
> >   void conf_handler(struct ctx *c, uint32_t events)
> >   {
> >   	if (events & EPOLLIN) {
> > -		char discard[BUFSIZ];
> > -		ssize_t n;
> > -
> > -		do {
> > -			n = read(c->fd_control, discard, sizeof(discard));
> > -			if (n > 0)
> > -				debug("Discarded %zd bytes of config data", n);
> > -		} while (n > 0);
> > -		if (n == 0) {
> > -			debug("Configuration client EOF");
> > -			goto close;
> > +		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;
> >   		}
> > -		if (errno != EAGAIN && errno != EWOULDBLOCK) {
> > -			err_perror("Error reading config data");
> > +
> > +		/* FIXME: this could block indefinitely if the client doesn't
> > +		 * write as much as it should
> > +		 */
> > +		if (conf_recv_rules(c, c->fd_control) < 0)
> >   			goto close;
> > +
> > +		for (pif = 0; pif < PIF_NUM_TYPES; pif++) {
> > +			struct fwd_table *fwd = c->fwd_pending[pif];
> > +
> > +			if (!fwd)
> > +				continue;
> > +
> > +			info("New forwarding rules for %s:", pif_name(pif));
> > +			fwd_rules_dump(info, fwd->rules, fwd->count,
> > +				       "    ", "");
> >   		}
> >   	}
> > diff --git a/fwd.c b/fwd.c
> > index 8849cfc..d93d2e5 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -247,6 +247,9 @@ void fwd_neigh_table_init(const struct ctx *c)
> >   static struct fwd_table fwd_in;
> >   static struct fwd_table fwd_out;
> > +static struct fwd_table fwd_in_pending;
> > +static struct fwd_table fwd_out_pending;
> > +
> >   /**
> >    * fwd_rule_init() - Initialise forwarding tables
> >    * @c:		Execution context
> > @@ -269,10 +272,15 @@ void fwd_rule_init(struct ctx *c)
> >   		caps |= FWD_CAP_IFNAME;
> >   	fwd_in.caps = fwd_out.caps = caps;
> > +	fwd_in_pending.caps = fwd_out_pending.caps = caps;
> >   	c->fwd[PIF_HOST] = &fwd_in;
> > -	if (c->mode == MODE_PASTA)
> > +	c->fwd_pending[PIF_HOST] = &fwd_in_pending;
> > +
> > +	if (c->mode == MODE_PASTA) {
> >   		c->fwd[PIF_SPLICE] = &fwd_out;
> > +		c->fwd_pending[PIF_SPLICE] = &fwd_out_pending;
> > +	}
> >   }
> >   /**
> > diff --git a/passt.h b/passt.h
> > index b3f049d..1726965 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -188,6 +188,7 @@ struct ip6_ctx {
> >    * @pasta_ifi:		Index of namespace interface for pasta
> >    * @pasta_conf_ns:	Configure namespace after creating it
> >    * @fwd:		Forwarding tables
> > + * @fwd_pending:	Pending forward tables
> >    * @no_tcp:		Disable TCP operation
> >    * @tcp:		Context for TCP protocol handler
> >    * @no_udp:		Disable UDP operation
> > @@ -270,6 +271,7 @@ struct ctx {
> >   	int pasta_conf_ns;
> >   	struct fwd_table *fwd[PIF_NUM_TYPES];
> > +	struct fwd_table *fwd_pending[PIF_NUM_TYPES];
> >   	int no_tcp;
> >   	struct tcp_ctx tcp;
> > diff --git a/pesto.c b/pesto.c
> > index 16b3a5a..73fdc39 100644
> > --- a/pesto.c
> > +++ b/pesto.c
> > @@ -230,6 +230,39 @@ static bool read_pif_conf(int fd, struct configuration *conf)
> >   	return true;
> >   }
> > +/**
> > + * send_conf() - Send updated configuration to passt/pasta
> > + * @fd:		Control socket
> > + * @conf:	Updated configuration
> > + */
> > +static void send_conf(int fd, const struct configuration *conf)
> > +{
> > +	unsigned i;
> > +
> 
> Perhaps it could be interesting to send a magic number (or a type id) if we
> want to be able to update something else than the rules in the future?
> We also can send the length of the data if we want to be able to ignore it
> if the type id is not supported?
> (Something like the chunks in IFF or PNG file format... but perhaps it's
> overcomplicated for our purpose...)

Undoubtedly, amongst a bunch of other ways we could make the protocol
more flexible.  However, I'm inclined to leave that v2.

> 
> > +	for (i = 0; i < conf->npifs; i++) {
> > +		const struct pif_configuration *pc = &conf->pif[i];
> > +		unsigned j;
> > +
> > +		if (write_u8(fd, pc->pif) < 0)
> > +			goto fail;
> > +
> > +		if (write_u32(fd, pc->fwd.count) < 0)
> > +			goto fail;
> > +
> > +		for (j = 0; j < pc->fwd.count; j++) {
> > +			if (fwd_rule_write(fd, &pc->fwd.rules[j]) < 0)
> > +				goto fail;
> > +		}
> > +	}
> > +
> > +	if (write_u8(fd, PIF_NONE) < 0)
> > +		goto fail;
> > +	return;
> > +
> > +fail:
> > +	die_perror("Error writing to control socket");
> > +}
> > +
> >   /**
> >    * show_conf() - Show current configuration obtained from passt/pasta
> >    * @conf:	Configuration description
> > @@ -432,6 +465,8 @@ int main(int argc, char **argv)
> >   		show_conf(&conf);
> >   	}
> > +	send_conf(s, &conf);
> > +
> >   noupdate:
> >   	if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0)
> >   		die_perror("Error shutting down control socket");
> 

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