Re: [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4713 bytes --]

On Wed, Mar 08, 2023 at 01:33:47PM +0100, Stefano Brivio wrote:
> The comments say we should return 0 on partial success, and an error
> code on complete failure. Rationale: if the user configures a port
> forwarding, and we succeed to bind that port for IPv4 or IPv6 only,
> that might actually be what the user intended.
> 
> Adjust the two functions to reflect the comments.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp.c | 21 +++++++++------------
>  udp.c | 30 ++++++++++++++----------------
>  2 files changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index e209483..a29e387 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2993,7 +2993,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  		  const char *ifname, in_port_t port)
>  {
> -	int ret = 0, af_ret;
> +	int r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
>  
>  	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
>  		/* Attempt to get a dual stack socket */
> @@ -3001,19 +3001,16 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  			return 0;
>  
>  	/* Otherwise create a socket per IP version */
> -	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4) {
> -		af_ret = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
> -		if (af_ret < 0)
> -			ret = af_ret;
> -	}
> +	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4)
> +		r4 = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
>  
> -	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> -		af_ret = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
> -		if (af_ret < 0)
> -			ret = af_ret;
> -	}
> +	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6)
> +		r6 = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
>  
> -	return ret;
> +	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
> +		return 0;
> +
> +	return r4 < 0 ? r4 : r6;
>  }
>  
>  /**
> diff --git a/udp.c b/udp.c
> index 41e0afd..a5f7537 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -983,7 +983,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		  const void *addr, const char *ifname, in_port_t port)
>  {
>  	union udp_epoll_ref uref = { .u32 = 0 };
> -	int s, ret = 0;
> +	int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
>  
>  	if (ns) {
>  		uref.udp.port = (in_port_t)(port +
> @@ -999,8 +999,8 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		uref.udp.orig = true;
>  
>  		if (!ns) {
> -			s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname,
> -				    port, uref.u32);
> +			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, addr,
> +					 ifname, port, uref.u32);
>  
>  			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
>  			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
> @@ -1008,13 +1008,10 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
>  			uref.udp.ns = true;
>  
> -			s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
> -				    ifname, port, uref.u32);
> +			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
> +					 ifname, port, uref.u32);
>  			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
>  		}
> -
> -		if (s < 0)
> -			ret = s;
>  	}
>  
>  	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> @@ -1023,24 +1020,25 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		uref.udp.orig = true;
>  
>  		if (!ns) {
> -			s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname,
> -				    port, uref.u32);
> +			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr,
> +					 ifname, port, uref.u32);
>  
>  			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
>  			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
>  		} else {
>  			uref.udp.ns = true;
>  
> -			s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback,
> -				    ifname, port, uref.u32);
> +			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
> +					 &in6addr_loopback,
> +					 ifname, port, uref.u32);
>  			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
>  		}
> -
> -		if (s < 0)
> -			ret = s;
>  	}
>  
> -	return ret;
> +	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
> +		return 0;
> +
> +	return r4 < 0 ? r4 : r6;

Same comment (unless sock_l4() already looks for 

>  }
>  
>  /**

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]