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

On Tue, Oct 21, 2025 at 11:51:07PM +0200, Stefano Brivio wrote:

On Fri, 17 Oct 2025 11:34:46 +1100 David Gibson 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 --- 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.

Good idea, done.

...

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.

Done. Thomas Huth finally convinced me not to put precautionary parens on anything next to && and ||, but I forgot this time :).

...

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

-- 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