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

[Laurent Vivier <lvivier@redhat.com>]

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

> +	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");