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

[Jon Maloy <jmaloy@redhat.com>]

On 2026-04-21 02:25, David Gibson wrote:
> 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>
> Message-ID: <20260322141843.4095972-3-sbrivio@redhat.com>
> [dwg: Based on an early draft from Stefano]\
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

[...]
>   
> +/**
> + * 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++) {
> +			fwd_rule_read(fd, &r);

Since we don't check the return value I think we risk passing an only 
partially initialized fwd_rule to fwd_rule_add() if the read fails.
Maybe:
   if (fwd_rule_read(fd, &r))
       return -1;

/jon
> +			if (fwd_rule_add(fwd, &r) < 0)
> +				return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * conf_close() - Close configuration / control socket and clean up
>    * @c:		Execution context
> @@ -2072,21 +2119,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 8849cfcd..d93d2e5d 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 b3f049de..1726965d 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 ebac6bd6..c6c595a4 100644
> --- a/pesto.c
> +++ b/pesto.c
> @@ -225,6 +225,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;
> +
> +	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
> @@ -427,6 +460,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");