Re: [PATCH bpf 6/6] selftest/bpf: Introduce __attribute__((cleanup)) in create_pair()

[Jakub Sitnicki <jakub@cloudflare.com>]

On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
> Rewrite function to have (unneeded) socket descriptors automatically
> close()d when leaving the scope. Make sure the "ownership" of fds is
> correctly passed via take_fd(); i.e. descriptor returned to caller will
> remain valid.
>
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  .../selftests/bpf/prog_tests/sockmap_helpers.h     | 57 ++++++++++++----------
>  1 file changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> index ead8ea4fd0da..2e0f9fe459be 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> @@ -182,6 +182,21 @@
>  		__ret;                                                         \
>  	})
>  
> +#define take_fd(fd)                                                            \
> +	({                                                                     \
> +		__auto_type __val = (fd);                                      \
> +		fd = -EBADF;                                                   \
> +		__val;                                                         \
> +	})

Probably should operate on a pointer to fd to avoid side effects, like
__get_and_null macro in include/linux/cleanup.h. take_fd is effectively
__get_and_null(fd, -EBADFD).

> +
> +static inline void close_fd(int *fd)
> +{
> +	if (*fd >= 0)
> +		xclose(*fd);
> +}
> +
> +#define __close_fd __attribute__((cleanup(close_fd)))
> +
>  static inline int poll_connect(int fd, unsigned int timeout_sec)
>  {
>  	struct timeval timeout = { .tv_sec = timeout_sec };
> @@ -369,9 +384,10 @@ static inline int socket_loopback(int family, int sotype)
>  
>  static inline int create_pair(int family, int sotype, int *p0, int *p1)
>  {
> +	__close_fd int s, c = -1, p = -1;
>  	struct sockaddr_storage addr;
>  	socklen_t len = sizeof(addr);
> -	int s, c, p, err;
> +	int err;
>  
>  	s = socket_loopback(family, sotype);
>  	if (s < 0)
> @@ -379,25 +395,23 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
>  
>  	err = xgetsockname(s, sockaddr(&addr), &len);
>  	if (err)
> -		goto close_s;
> +		return err;
>  
>  	c = xsocket(family, sotype, 0);
> -	if (c < 0) {
> -		err = c;
> -		goto close_s;
> -	}
> +	if (c < 0)
> +		return c;
>  
>  	err = connect(c, sockaddr(&addr), len);
>  	if (err) {
>  		if (errno != EINPROGRESS) {
>  			FAIL_ERRNO("connect");
> -			goto close_c;
> +			return err;
>  		}
>  
>  		err = poll_connect(c, IO_TIMEOUT_SEC);
>  		if (err) {
>  			FAIL_ERRNO("poll_connect");
> -			goto close_c;
> +			return err;
>  		}
>  	}
>  
> @@ -405,36 +419,29 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
>  	case SOCK_DGRAM:
>  		err = xgetsockname(c, sockaddr(&addr), &len);
>  		if (err)
> -			goto close_c;
> +			return err;
>  
>  		err = xconnect(s, sockaddr(&addr), len);
> -		if (!err) {
> -			*p0 = s;
> -			*p1 = c;
> +		if (err)
>  			return err;
> -		}
> +
> +		*p0 = take_fd(s);
>  		break;
>  	case SOCK_STREAM:
>  	case SOCK_SEQPACKET:
>  		p = xaccept_nonblock(s, NULL, NULL);
> -		if (p >= 0) {
> -			*p0 = p;
> -			*p1 = c;
> -			goto close_s;
> -		}
> +		if (p < 0)
> +			return p;
>  
> -		err = p;
> +		*p0 = take_fd(p);
>  		break;
>  	default:
>  		FAIL("Unsupported socket type %#x", sotype);
> -		err = -EOPNOTSUPP;
> +		return -EOPNOTSUPP;
>  	}
>  
> -close_c:
> -	close(c);
> -close_s:
> -	close(s);
> -	return err;
> +	*p1 = take_fd(c);
> +	return 0;
>  }
>  
>  static inline int create_socket_pairs(int family, int sotype, int *c0, int *c1,

This turned out nice and readable, IMHO.