Re: [PATCH 2/3] fwd, pif: Replace with pif_sock_l4() with pif_listen()

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 4 Mar 2026 09:55:21 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 03, 2026 at 08:11:39PM +0100, Stefano Brivio wrote:
> > On Mon,  2 Mar 2026 15:31:34 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > It turns out all users of pif_sock_l4() use it for "listening" sockets,
> > > which now all have a common epoll reference format.  We can take advantage
> > > of that to pass the necessary epoll information into pif_sock_l4() in a
> > > more natural way, rather than as an opaque u32.
> > > 
> > > That in turn allows union fwd_listen_ref can become a struct, since the
> > > union only exist to allow the meaningful fields to be coerced into a u32
> > > for pif_sock_l4().
> > > 
> > > Rename pif_sock_l4() to pif_listen() to reflect the new semantics.  While
> > > we're there, remove the static_assert() on the fwd_listen_ref's size.  We
> > > do still need it to fit into 32 bits, but that constraint is imposed only
> > > by the fact that it needs to fit into the whole, epoll_ref structure,
> > > which we already check with a static_assert() in epoll_ctl.h.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  epoll_ctl.h |  2 +-
> > >  fwd.h       | 14 +++++---------
> > >  pif.c       | 14 ++++++++------
> > >  pif.h       |  6 +++---
> > >  tcp.c       |  8 +-------
> > >  udp.c       |  8 +-------
> > >  6 files changed, 19 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/epoll_ctl.h b/epoll_ctl.h
> > > index 2c103cde..879763cb 100644
> > > --- a/epoll_ctl.h
> > > +++ b/epoll_ctl.h
> > > @@ -35,7 +35,7 @@ union epoll_ref {
> > >  		union {
> > >  			uint32_t flow;
> > >  			flow_sidx_t flowside;
> > > -			union fwd_listen_ref listen;
> > > +			struct fwd_listen_ref listen;
> > >  			uint32_t data;
> > >  			int nsdir_fd;
> > >  			int queue;
> > > diff --git a/fwd.h b/fwd.h
> > > index 16070111..b0e2373d 100644
> > > --- a/fwd.h
> > > +++ b/fwd.h
> > > @@ -49,20 +49,16 @@ struct fwd_rule {
> > >  #define FWD_NO_HINT	(-1)
> > >  
> > >  /**
> > > - * union fwd_listen_ref - information about a single listening socket
> > > + * struct fwd_listen_ref - information about a single listening socket
> > >   * @port:	Bound port number of the socket
> > >   * @pif:	pif in which the socket is listening
> > >   * @rule:	Index of forwarding rule
> > >   */
> > > -union fwd_listen_ref {
> > > -	struct {
> > > -		in_port_t	port;
> > > -		uint8_t		pif;
> > > -		unsigned	rule :FWD_RULE_BITS;
> > > -	};
> > > -	uint32_t u32;
> > > +struct fwd_listen_ref {
> > > +	in_port_t	port;
> > > +	uint8_t		pif;
> > > +	unsigned	rule :FWD_RULE_BITS;
> > >  };
> > > -static_assert(sizeof(union fwd_listen_ref) == sizeof(uint32_t));  
> > 
> > Reported-by: Peter Foley <pefoley@google.com>  
> 
> Good point.  Can you add this on merge?

Sure, done. Applied now.

-- 
Stefano