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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7121 bytes --]

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?

> 
> >  
> >  enum fwd_ports_mode {
> >  	FWD_UNSET = 0,
> > diff --git a/pif.c b/pif.c
> > index 6ae970a0..82a3b5e4 100644
> > --- a/pif.c
> > +++ b/pif.c
> > @@ -62,23 +62,23 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
> >  	}
> >  }
> >  
> > -/** pif_sock_l4() - Open a socket bound to an address on a specified interface
> > +/** pif_listen() - Open a listening socket on a specified pif
> >   * @c:		Execution context
> >   * @type:	Socket epoll type
> >   * @pif:	Interface for this socket
> >   * @addr:	Address to bind to, or NULL for dual-stack any
> >   * @ifname:	Interface for binding, NULL for any
> >   * @port:	Port number to bind to (host byte order)
> > - * @data:	epoll reference portion for protocol handlers
> > + * @rule:	Forwarding rule index this socket belongs to
> >   *
> >   * NOTE: For namespace pifs, this must be called having already entered the
> >   * relevant namespace.
> >   *
> >   * Return: newly created socket, negative error code on failure
> >   */
> > -int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
> > -		const union inany_addr *addr, const char *ifname,
> > -		in_port_t port, uint32_t data)
> > +int pif_listen(const struct ctx *c, enum epoll_type type, uint8_t pif,
> > +	       const union inany_addr *addr, const char *ifname,
> > +	       in_port_t port, unsigned rule)
> >  {
> >  	union epoll_ref ref;
> >  	int ret;
> > @@ -98,7 +98,9 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
> >  		return ref.fd;
> >  
> >  	ref.type = type;
> > -	ref.data = data;
> > +	ref.listen.port = port;
> > +	ref.listen.pif = pif;
> > +	ref.listen.rule = rule;
> >  
> >  	ret = epoll_add(c->epollfd, EPOLLIN, ref);
> >  	if (ret < 0) {
> > diff --git a/pif.h b/pif.h
> > index 0f7f6672..0eb3626a 100644
> > --- a/pif.h
> > +++ b/pif.h
> > @@ -59,8 +59,8 @@ static inline bool pif_is_socket(uint8_t pif)
> >  
> >  void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
> >  		  uint8_t pif, const union inany_addr *addr, in_port_t port);
> > -int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
> > -		const union inany_addr *addr, const char *ifname,
> > -		in_port_t port, uint32_t data);
> > +int pif_listen(const struct ctx *c, enum epoll_type type, uint8_t pif,
> > +	       const union inany_addr *addr, const char *ifname,
> > +	       in_port_t port, unsigned rule);
> >  
> >  #endif /* PIF_H */
> > diff --git a/tcp.c b/tcp.c
> > index 16470703..a0d7cd86 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -2698,11 +2698,6 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
> >  int tcp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
> >  	       const union inany_addr *addr, const char *ifname, in_port_t port)
> >  {
> > -	union fwd_listen_ref ref = {
> > -		.port = port,
> > -		.pif = pif,
> > -		.rule = rule,
> > -	};
> >  	int s;
> >  
> >  	ASSERT(!c->no_tcp);
> > @@ -2722,8 +2717,7 @@ int tcp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
> >  			return -EAFNOSUPPORT;
> >  	}
> >  
> > -	s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, pif, addr, ifname,
> > -			port, ref.u32);
> > +	s = pif_listen(c, EPOLL_TYPE_TCP_LISTEN, pif, addr, ifname, port, rule);
> >  
> >  	return s;
> >  }
> > diff --git a/udp.c b/udp.c
> > index 19adda06..464aa093 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -1139,11 +1139,6 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
> >  int udp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
> >  	       const union inany_addr *addr, const char *ifname, in_port_t port)
> >  {
> > -	union fwd_listen_ref ref = {
> > -		.pif = pif,
> > -		.port = port,
> > -		.rule = rule,
> > -	};
> >  	int s;
> >  
> >  	ASSERT(!c->no_udp);
> > @@ -1163,8 +1158,7 @@ int udp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
> >  			return -EAFNOSUPPORT;
> >  	}
> >  
> > -	s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif,
> > -			addr, ifname, port, ref.u32);
> > +	s = pif_listen(c, EPOLL_TYPE_UDP_LISTEN, pif, addr, ifname, port, rule);
> >  
> >  	return s;
> >  }
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]