Re: [PATCH 1/3] tcp: Merge tcp_ns_sock_init[46]() into tcp_sock_init_one()

On Fri, 17 Oct 2025 11:34:45 +1100 David Gibson wrote:

Surprisingly little logic is shared between the path for creating a listen()ing socket in the guest namespace versus in the host namespace. Improve this, by extending tcp_sock_init_one() to take a pif parameter indicating where it should open the socket. This allows tcp_ns_sock_init[46]() to be removed entirely.

We generalise tcp_sock_init() in the same way, although we don't use it yet, due to some subtle differences in how we bind for -t versus -T.

Signed-off-by: David Gibson --- conf.c | 2 +- tcp.c | 96 ++++++++++++++++++---------------------------------------- tcp.h | 5 +-- 3 files changed, 33 insertions(+), 70 deletions(-)

diff --git a/conf.c b/conf.c index 66b9e634..26f1bcc0 100644 --- a/conf.c +++ b/conf.c @@ -169,7 +169,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, fwd->delta[i] = to - first;

if (optname == 't') - ret = tcp_sock_init(c, addr, ifname, i); + ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') ret = udp_sock_init(c, 0, addr, ifname, i); else diff --git a/tcp.c b/tcp.c index 0f9e9b3f..15c012d7 100644 --- a/tcp.c +++ b/tcp.c @@ -2515,29 +2515,38 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, /** * tcp_sock_init_one() - Initialise listening socket for address and port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL for dual stack any * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: fd for the new listening socket, negative error code on failure + * + * If pif == PIF_SPLICE, must have already entered the namespace. */ -static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr, - const char *ifname, in_port_t port) +static int tcp_sock_init_one(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port) { + const struct fwd_ports *fwd = pif == PIF_HOST ? + &c->tcp.fwd_in : &c->tcp.fwd_out;

While I appreciate the resulting brevity, I wonder if it would make more sense to have this as an explicit if / else clause, for readability. Same for similar occurrences in the next patches (which I didn't fully review, yet). Another alternative is: const struct fwd_ports *fwd; fwd = (pif == PIF_HOST) ? &c->tcp.fwd_in : &c->tcp.fwd_out; ...still two lines of code, perhaps just slightly less readable than the five obvious ones: const struct fwd_ports *fwd; if (pif == PIF_HOST) fwd = &c->tcp.fwd_in; else fwd = &c->tcp.fwd_out; -- Stefano