Re: [PATCH 2/3] udp: Unify some more inbound/outbound parts of udp_sock_init()

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 17 Oct 2025 11:34:46 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> udp_sock_init() takes an 'ns' parameter determining if it creates a socket
> in the guest namespace or host namespace.  Alter it to take a pif
> parameter instead, like tcp_sock_init(), and use that change to slightly
> reduce code duplication between the HOST and SPLICE cases.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c |  2 +-
>  udp.c  | 60 +++++++++++++++++++++++++++++-----------------------------
>  udp.h  |  5 +++--
>  3 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 26f1bcc0..08cb50aa 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -171,7 +171,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
>  		if (optname == 't')
>  			ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i);
>  		else if (optname == 'u')
> -			ret = udp_sock_init(c, 0, addr, ifname, i);
> +			ret = udp_sock_init(c, PIF_HOST, addr, ifname, i);
>  		else
>  			/* No way to check in advance for -T and -U */
>  			ret = 0;
> diff --git a/udp.c b/udp.c
> index 86585b7e..49dd0144 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1093,64 +1093,63 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>  /**
>   * udp_sock_init() - Initialise listening sockets for a given port
>   * @c:		Execution context
> - * @ns:		In pasta mode, if set, bind with loopback address in namespace
> + * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
>   * @addr:	Pointer to address for binding, NULL if not configured
>   * @ifname:	Name of interface to bind to, NULL if not configured
>   * @port:	Port, host order
>   *
>   * Return: 0 on (partial) success, negative error code on (complete) failure
>   */
> -int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr,
> -		  const char *ifname, in_port_t port)
> +int udp_sock_init(const struct ctx *c, uint8_t pif,
> +		  const union inany_addr *addr, const char *ifname,
> +		  in_port_t port)
>  {
> +	int (*socks)[NUM_PORTS] = pif == PIF_HOST ?
> +		udp_splice_init : udp_splice_ns;

Same as on v1: I'd rather avoid cramping ternary operators like this.

>  	union udp_listen_epoll_ref uref = {
> -		.pif = ns ? PIF_SPLICE : PIF_HOST,
> +		.pif = pif,
>  		.port = port,
>  	};
>  	int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
>  
>  	ASSERT(!c->no_udp);
>  
> -	if (!addr && c->ifi4 && c->ifi6 && !ns) {
> +	if (!addr && c->ifi4 && c->ifi6 && (pif == PIF_HOST)) {

I think it's more readable without the extra parentheses around the
comparison, because when I see those I automatically think of an
assignment we want to use in a conditional clause, but that's just a
comparison.

>  		int s;
>  
>  		/* Attempt to get a dual stack socket */
>  		s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
>  				NULL, ifname, port, uref.u32);
> -		udp_splice_init[V4][port] = s < 0 ? -1 : s;
> -		udp_splice_init[V6][port] = s < 0 ? -1 : s;
> +		socks[V4][port] = s < 0 ? -1 : s;
> +		socks[V6][port] = s < 0 ? -1 : s;
>  		if (IN_INTERVAL(0, FD_REF_MAX, s))
>  			return 0;
>  	}
>  
>  	if ((!addr || inany_v4(addr)) && c->ifi4) {
> -		if (!ns) {
> -			r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
> -					 addr ? addr : &inany_any4, ifname,
> -					 port, uref.u32);
> +		const union inany_addr *a = addr ?
> +			addr : &inany_any4;
>  
> -			udp_splice_init[V4][port] = r4 < 0 ? -1 : r4;
> -		} else {
> -			r4  = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_SPLICE,
> -					  &inany_loopback4, ifname,
> -					  port, uref.u32);
> -			udp_splice_ns[V4][port] = r4 < 0 ? -1 : r4;
> -		}
> +		if (pif == PIF_SPLICE)
> +			a = &inany_loopback4;
> +
> +		r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
> +				 port, uref.u32);
> +
> +		socks[V4][port] = r4 < 0 ? -1 : r4;
>  	}
>  
>  	if ((!addr || !inany_v4(addr)) && c->ifi6) {
> -		if (!ns) {
> -			r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
> -					 addr ? addr : &inany_any6, ifname,
> -					 port, uref.u32);
> +		const union inany_addr *a = addr ?
> +			addr : &inany_any6;
>  
> -			udp_splice_init[V6][port] = r6 < 0 ? -1 : r6;
> -		} else {
> -			r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_SPLICE,
> -					 &inany_loopback6, ifname,
> -					 port, uref.u32);
> -			udp_splice_ns[V6][port] = r6 < 0 ? -1 : r6;
> -		}
> +		if (pif == PIF_SPLICE)
> +			a = &inany_loopback6;
> +
> +		r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
> +				 port, uref.u32);
> +
> +		socks[V6][port] = r6 < 0 ? -1 : r6;
>  	}
>  
>  	if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6))
> @@ -1216,7 +1215,8 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
>  
>  		if ((c->ifi4 && socks[V4][port] == -1) ||
>  		    (c->ifi6 && socks[V6][port] == -1))
> -			udp_sock_init(c, outbound, NULL, NULL, port);
> +			udp_sock_init(c, outbound ? PIF_SPLICE : PIF_HOST,
> +				      NULL, NULL, port);
>  	}
>  }
>  
> diff --git a/udp.h b/udp.h
> index 8f8531ad..f78dc528 100644
> --- a/udp.h
> +++ b/udp.h
> @@ -17,8 +17,9 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>  		    sa_family_t af, const void *saddr, const void *daddr,
>  		    uint8_t ttl, const struct pool *p, int idx,
>  		    const struct timespec *now);
> -int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr,
> -		  const char *ifname, in_port_t port);
> +int udp_sock_init(const struct ctx *c, uint8_t pif,
> +		  const union inany_addr *addr, const char *ifname,
> +		  in_port_t port);
>  int udp_init(struct ctx *c);
>  void udp_timer(struct ctx *c, const struct timespec *now);
>  void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s);

-- 
Stefano