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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 17 Oct 2025 11:34:45 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> ---
>  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