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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 6 May 2026 11:11:05 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 5/6/26 10:52, David Gibson wrote:
> > On Wed, May 06, 2026 at 10:49:55AM +0200, Stefano Brivio wrote:  
> >> On Wed, 6 May 2026 10:39:30 +0200
> >> Stefano Brivio <sbrivio@redhat.com> wrote:
> >>  
> >>> On Wed, 6 May 2026 10:12:21 +0200
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>  
> >>>> On 5/6/26 01:47, 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>
> >>>>> [sbrivio: In fwd_listen_switch(), use the destination size as argument
> >>>>>    to memcpy(), instead of sizeof(tmp), as suggested by Laurent]
> >>>>> 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 76344da..3f48793 100644
> >>>>> --- a/conf.c
> >>>>> +++ b/conf.c
> >>>>> @@ -2160,15 +2160,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..0697435 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;
> >>>>> +	}
> >>>>> +
> >>>>> +	/* Swap active and pending tables */
> >>>>> +	static_assert(sizeof(tmp) == sizeof(c->fwd) &&
> >>>>> +		      sizeof(tmp) == sizeof(c->fwd_pending),
> >>>>> +		      "Temporary has wrong size");  
> >>>>
> >>>> At this point:
> >>>>
> >>>> c->fwd[PIF_HOST] = &fwd_in;
> >>>> c->fwd[PIF_SPLICE] = &fwd_out;
> >>>>
> >>>> c->fwd_pending[PIF_HOST] = &fwd_in_pending;
> >>>> c->fwd_pending[PIF_SPLICE] = &fwd_out_pending;
> >>>>      
> >>>>> +	memcpy(&tmp, (void *)c->fwd, sizeof(tmp));
> >>>>> +	memcpy((void *)c->fwd, (void *)c->fwd_pending, sizeof(c->fwd));
> >>>>> +	memcpy((void *)c->fwd_pending, &tmp, sizeof(c->fwd_pending));  
> >>>>
> >>>> At this point:
> >>>>
> >>>> c->fwd[PIF_HOST] = &fwd_in_pending;
> >>>> c->fwd[PIF_SPLICE] = &fwd_out_pending;
> >>>>
> >>>> c->fwd_pending[PIF_HOST] = &fwd_in;
> >>>> c->fwd_pending[PIF_SPLICE] = &fwd_out;  
> >>>
> >>> Yeah, makes sense, I can change that in v9.
> >>>  
> >>>> Perhaps it should be noted somewhere to avoid confusion in the future?  
> >>>
> >>> What do you think should be noted exactly, and where? Can you show a
> >>> practical example of the change you're proposing?  
> >>
> >> ...I'm leaving like it is in v9 to make sure I'm not misinterpreting
> >> you, and also because the current (v8) version is obviously correct and
> >> I also tested it fairly heavily by now.
> >>
> >> I'd suggest optimising this (and commenting as needed) in a separate
> >> patch later.  
> > 
> > As noted in another branch of this thread, I think all it really needs
> > is renaming the globals that c->fwd and c->fwd_pending point to.  They
> > should just be fwd_in_[12] (or an array of 2 tables), instead of
> > implying a semantic difference between the plain and "pending" copies.
> 
> I agree with that.

Okay, I see, and I agree as well. I'd still keep that as a separate
patch (there are quite a few follow-ups we'll need anyway) as it's not
really fundamental right now.

-- 
Stefano