Re: [PATCH v7 17/18] conf, fwd: Allow switching to new rules received from pesto

[Laurent Vivier <lvivier@redhat.com>]

On 5/5/26 01:11, Stefano Brivio wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> We can now receive updates to the forwarding rules from the pesto client
> and store them in a "pending" copy of the forwarding tables.  Implement
> switching to using the new rules.
> 
> The logic is in a new fwd_listen_switch().  For now this closes all
> listening sockets related to the old tables, swaps the active and pending
> tables, then listens based on the new tables.  In future we look to improve
> this so that we don't temporarily stop listening on ports that both the
> old and new tables specify.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>   conf.c |  5 ++---
>   fwd.c  | 34 ++++++++++++++++++++++++++++++++++
>   fwd.h  |  1 +
>   3 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index f035fd3..75b8291 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -2159,15 +2159,14 @@ void conf_handler(struct ctx *c, uint32_t events)
>   			fwd_rules_dump(info, fwd->rules, fwd->count,
>   				       "    ", "");
>   		}
> +
> +		fwd_listen_switch(c);
>   	}
>   
>   	if (events & EPOLLHUP) {
>   		debug("Configuration client hangup");
> -		goto close;
>   	}
>   
> -	return;
> -
>   close:
>   	conf_close(c);
>   
> diff --git a/fwd.c b/fwd.c
> index d93d2e5..35b9e2b 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -534,6 +534,40 @@ int fwd_listen_init(const struct ctx *c)
>   	return 0;
>   }
>   
> +/**
> + * fwd_listen_switch() - Switch from current to pending rules table
> + * @c:		Execution context
> + */
> +void fwd_listen_switch(struct ctx *c)
> +{
> +	struct fwd_table *tmp[PIF_NUM_TYPES];
> +	unsigned i;
> +
> +	/* Stop listening on the old tables */
> +	for (i = 0; i < PIF_NUM_TYPES; i++) {
> +		struct fwd_table *fwd = c->fwd[i];
> +
> +		if (!fwd)
> +			continue;
> +
> +		debug("Flushing %u old %s rules", fwd->count, pif_name(i));
> +		fwd_listen_close(fwd);
> +		fwd->count = fwd->sock_count = 0;

Perhaps we can reset fwd->count and fwd->sock_count in fwd_listen_close() as after 
fwd_listen_close() these values are wrong?

> +	}
> +
> +	/* Swap active and pending tables */
> +	static_assert(sizeof(tmp) == sizeof(c->fwd) &&
> +		      sizeof(tmp) == sizeof(c->fwd_pending),
> +		      "Temporary has wrong size");
> +	memcpy(&tmp, (void *)c->fwd, sizeof(tmp));
> +	memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(tmp));
> +	memcpy((void *)c->fwd_pending, &tmp, sizeof(tmp));

I know we have the static_assert(), but with memcpy() we usually use the sizeof() of the 
destination to avoid write overflow.

Why do we keep the old active table? Do we plan to have a "--restore" option?

> +
> +	/* Start listening on the new tables */
> +	if (fwd_listen_init(c) < 0)
> +		err("Error switching to new forwarding rules");
> +}
> +
>   /* See enum in kernel's include/net/tcp_states.h */
>   #define UDP_LISTEN	0x07
>   #define TCP_LISTEN	0x0a
> diff --git a/fwd.h b/fwd.h
> index ac24782..b60697d 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -61,6 +61,7 @@ int fwd_listen_sync(const struct ctx *c, uint8_t pif,
>   		    const struct fwd_scan *tcp, const struct fwd_scan *udp);
>   void fwd_listen_close(const struct fwd_table *fwd);
>   int fwd_listen_init(const struct ctx *c);
> +void fwd_listen_switch(struct ctx *c);
>   
>   bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
>   		 union inany_addr *translated);