Re: [PATCH v3 2/8] util, flow, pif: Simplify sock_l4_sa() interface

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 29 Oct 2025 17:26:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> sock_l4_sa() has a somewhat confusing 'v6only' option controlling whether
> to set the IPV6_V6ONLY socket option.  Usually it's set when the given
> address is IPv6, but not when we want to create a dual stack listening
> socket.  The latter only makes sense when the address is :: however.
> 
> Clarify this by only keeping the v6only option in an internal helper
> sock_l4_().  External users will call either sock_l4() which always creates
> a socket bound to a specific IP version, or sock_l4_dualstack() which
> creates a dual stack socket, but takes only a port not an address.

I'm not sure if we'll ever need anything different, but I guess that
this is not the only obvious semantic of sock_l4_dualstack(), as it
could take a sockaddr_inany eventually, and bind() IPv6 address and its
v4-mapped equivalent (...does that even work?).

> We drop the '_sa' suffix while we're at it - it exists because this used
> to be an internal version with a sock_l4() wrapper.  The wrapper no longer
> exists so the '_sa' is no longer useful.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  flow.c |  6 ++----
>  pif.c  | 10 +++-------
>  util.c | 27 +++++++++++++++++++++++----
>  util.h |  8 +++++---
>  4 files changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/flow.c b/flow.c
> index 9926f408..fd530ddb 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -186,8 +186,7 @@ static int flowside_sock_splice(void *arg)
>  
>  	ns_enter(a->c);
>  
> -	a->fd = sock_l4_sa(a->c, a->type, a->sa, NULL,
> -	                   a->sa->sa_family == AF_INET6, a->data);
> +	a->fd = sock_l4(a->c, a->type, a->sa, NULL, a->data);
>  	a->err = errno;
>  
>  	return 0;
> @@ -222,8 +221,7 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
>  		else if (sa.sa_family == AF_INET6)
>  			ifname = c->ip6.ifname_out;
>  
> -		return sock_l4_sa(c, type, &sa, ifname,
> -				  sa.sa_family == AF_INET6, data);
> +		return sock_l4(c, type, &sa, ifname, data);
>  
>  	case PIF_SPLICE: {
>  		struct flowside_sock_args args = {
> diff --git a/pif.c b/pif.c
> index 31723b29..5fb1f455 100644
> --- a/pif.c
> +++ b/pif.c
> @@ -75,11 +75,7 @@ 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)
>  {
> -	union sockaddr_inany sa = {
> -		.sa6.sin6_family = AF_INET6,
> -		.sa6.sin6_addr = in6addr_any,
> -		.sa6.sin6_port = htons(port),
> -	};
> +	union sockaddr_inany sa;
>  
>  	ASSERT(pif_is_socket(pif));
>  
> @@ -90,8 +86,8 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
>  	}
>  
>  	if (!addr)
> -		return sock_l4_sa(c, type, &sa, ifname, false, data);
> +		return sock_l4_dualstack(c, type, port, ifname, data);
>  
>  	pif_sockaddr(c, &sa, pif, addr, port);
> -	return sock_l4_sa(c, type, &sa, ifname, sa.sa_family == AF_INET6, data);
> +	return sock_l4(c, type, &sa, ifname, data);
>  }
> diff --git a/util.c b/util.c
> index 976fcabe..c94efae4 100644
> --- a/util.c
> +++ b/util.c
> @@ -40,7 +40,7 @@
>  #endif
>  
>  /**
> - * sock_l4_sa() - Create and bind socket to socket address, add to epoll list
> + * sock_l4_() - Create and bind socket to socket address, add to epoll list
>   * @c:		Execution context
>   * @type:	epoll type
>   * @sa:		Socket address to bind to
> @@ -50,9 +50,9 @@
>   *
>   * Return: newly created socket, negative error code on failure
>   */
> -int sock_l4_sa(const struct ctx *c, enum epoll_type type,
> -	       const union sockaddr_inany *sa, const char *ifname,
> -	       bool v6only, uint32_t data)
> +static int sock_l4_(const struct ctx *c, enum epoll_type type,
> +		    const union sockaddr_inany *sa, const char *ifname,
> +		    bool v6only, uint32_t data)
>  {
>  	sa_family_t af = sa->sa_family;
>  	union epoll_ref ref = { .type = type, .data = data };
> @@ -182,6 +182,25 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  	return fd;
>  }
>  
> +int sock_l4(const struct ctx *c, enum epoll_type type,
> +	    const union sockaddr_inany *sa, const char *ifname,
> +	    uint32_t data)

Not extremely useful but it saves one "lookup":

/**
 * sock_l4() - Create and bind socket to given address, add to epoll list
 * @c:		Execution context
 * @type:	epoll type
 * @sa:		Socket address to bind to
 * @ifname:	Interface for binding, NULL for any
 *
 * Return: newly created socket, negative error code on failure
 */

> +{
> +	return sock_l4_(c, type, sa, ifname, sa->sa_family == AF_INET6, data);
> +}
> +
> +int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
> +		      in_port_t port, const char *ifname, uint32_t data)

...same here, and the comment might be used to clarify the
functionality.

> +{
> +	union sockaddr_inany sa = {
> +		.sa6.sin6_family = AF_INET6,
> +		.sa6.sin6_addr = in6addr_any,
> +		.sa6.sin6_port = htons(port),
> +	};
> +
> +	return sock_l4_(c, type, &sa, ifname, 0, data);
> +}
> +
>  /**
>   * sock_unix() - Create and bind AF_UNIX socket
>   * @sock_path:	Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
> diff --git a/util.h b/util.h
> index e1a1ebc9..7f0cf686 100644
> --- a/util.h
> +++ b/util.h
> @@ -203,9 +203,11 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
>  struct ctx;
>  union sockaddr_inany;
>  
> -int sock_l4_sa(const struct ctx *c, enum epoll_type type,
> -	       const union sockaddr_inany *sa, const char *ifname,
> -	       bool v6only, uint32_t data);
> +int sock_l4(const struct ctx *c, enum epoll_type type,
> +	    const union sockaddr_inany *sa, const char *ifname,
> +	    uint32_t data);
> +int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
> +		      in_port_t port, const char *ifname, uint32_t data);
>  int sock_unix(char *sock_path);
>  void sock_probe_mem(struct ctx *c);
>  long timespec_diff_ms(const struct timespec *a, const struct timespec *b);

-- 
Stefano