Re: [PATCH v2 bpf-next 4/5] selftests/bpf: Add connect_fd_to_fd, connect_wait net helpers

[Yonghong Song <yhs@fb.com>]

On 5/13/20 2:38 PM, Andrey Ignatov wrote:
> Add two new network helpers.
> 
> connect_fd_to_fd connects an already created client socket fd to address
> of server fd. Sometimes it's useful to separate client socket creation
> and connecting this socket to a server, e.g. if client socket has to be
> created in a cgroup different from that of server cgroup.
> 
> Additionally connect_to_fd is now implemented using connect_fd_to_fd,
> both helpers don't treat EINPROGRESS as an error and let caller decide
> how to proceed with it.
> 
> connect_wait is a helper to work with non-blocking client sockets so
> that if connect_to_fd or connect_fd_to_fd returned -1 with errno ==
> EINPROGRESS, caller can wait for connect to finish or for connection
> timeout. The helper returns -1 on error, 0 on timeout (1sec,
> hard-coded), and positive number on success.
> 
> Signed-off-by: Andrey Ignatov <rdna@fb.com>

Ack with a minor nit below.
Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/testing/selftests/bpf/network_helpers.c | 66 +++++++++++++++----
>   tools/testing/selftests/bpf/network_helpers.h |  2 +
>   2 files changed, 56 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index 0ff64b70b746..542d71ed7f5d 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -4,10 +4,14 @@
>   #include <stdio.h>
>   #include <string.h>
>   #include <unistd.h>
> +
> +#include <sys/epoll.h>
> +
>   #include <linux/err.h>
>   #include <linux/in.h>
>   #include <linux/in6.h>
>   
> +#include "bpf_util.h"
>   #include "network_helpers.h"
>   
>   #define clean_errno() (errno == 0 ? "None" : strerror(errno))
> @@ -77,8 +81,6 @@ static const size_t timeo_optlen = sizeof(timeo_sec);
>   
>   int connect_to_fd(int family, int type, int server_fd)
>   {
> -	struct sockaddr_storage addr;
> -	socklen_t len = sizeof(addr);
>   	int fd;
>   
>   	fd = socket(family, type, 0);
> @@ -87,24 +89,64 @@ int connect_to_fd(int family, int type, int server_fd)
>   		return -1;
>   	}
>   
> -	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, timeo_optlen)) {
> +	if (connect_fd_to_fd(fd, server_fd) < 0 && errno != EINPROGRESS) {
> +		close(fd);

Remote possibility. close(fd) may change error code?

In my opinion, maybe convert the original syscall failure errno to 
return value and carrying on might be a simpler choice?

> +		return -1;
> +	}
> +
> +	return fd;
> +}
> +
> +int connect_fd_to_fd(int client_fd, int server_fd)
> +{
> +	struct sockaddr_storage addr;
> +	socklen_t len = sizeof(addr);
> +
> +	if (setsockopt(client_fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec,
> +		       timeo_optlen)) {
>   		log_err("Failed to set SO_RCVTIMEO");
> -		goto out;
> +		return -1;
>   	}
>   
>   	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
>   		log_err("Failed to get server addr");
> -		goto out;
> +		return -1;
>   	}
>   
> -	if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
> -		log_err("Fail to connect to server with family %d", family);
> -		goto out;
> +	if (connect(client_fd, (const struct sockaddr *)&addr, len) < 0) {
> +		if (errno != EINPROGRESS)
> +			log_err("Failed to connect to server");

Not saying it is possible, but any remote possibility log_err()
may change error code to EINPROGRESS?

> +		return -1;
>   	}
>   
> -	return fd;
> +	return 0;
> +}
[...]