Re: [PATCH bpf-next 2/5] selftests/bpf: Drop must_fail from network_helper_opts

[Martin KaFai Lau <martin.lau@linux.dev>]

On 7/10/24 7:52 PM, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The struct member "must_fail" of network_helper_opts() is only used in
> cgroup_v1v2 tests, it makes sense to drop it from network_helper_opts.
> 
> Return value (fd) of connect_to_fd_opts() and the expect errno (EPERM)
> can be checked in cgroup_v1v2.c directly, no need to check them in
> connect_fd_to_addr() in network_helpers.c.
> 
> This also makes connect_fd_to_addr() function useless. It can be replaced
> by connect().
> 
> Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>   tools/testing/selftests/bpf/network_helpers.c | 31 ++-----------------
>   tools/testing/selftests/bpf/network_helpers.h |  1 -
>   .../selftests/bpf/prog_tests/cgroup_v1v2.c    |  6 ++--
>   3 files changed, 4 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index 15e0e0bb7553..48c27c810db7 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -277,33 +277,6 @@ int client_socket(int family, int type,
>   	return -1;
>   }
>   
> -static int connect_fd_to_addr(int fd,
> -			      const struct sockaddr_storage *addr,
> -			      socklen_t addrlen, const bool must_fail)
> -{
> -	int ret;
> -
> -	errno = 0;
> -	ret = connect(fd, (const struct sockaddr *)addr, addrlen);
> -	if (must_fail) {
> -		if (!ret) {
> -			log_err("Unexpected success to connect to server");
> -			return -1;
> -		}
> -		if (errno != EPERM) {
> -			log_err("Unexpected error from connect to server");
> -			return -1;
> -		}
> -	} else {
> -		if (ret) {
> -			log_err("Failed to connect to server");
> -			return -1;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t addrlen,
>   		    const struct network_helper_opts *opts)
>   {
> @@ -318,7 +291,7 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
>   		return -1;
>   	}
>   
> -	if (connect_fd_to_addr(fd, addr, addrlen, opts->must_fail))
> +	if (connect(fd, (const struct sockaddr *)addr, addrlen))
>   		goto error_close;
>   
>   	return fd;
> @@ -383,7 +356,7 @@ int connect_fd_to_fd(int client_fd, int server_fd, int timeout_ms)
>   		return -1;
>   	}
>   
> -	if (connect_fd_to_addr(client_fd, &addr, len, false))
> +	if (connect(client_fd, (const struct sockaddr *)&addr, len))
>   		return -1;
>   
>   	return 0;
> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index 5b548c0c60de..f39eeb5a4594 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -23,7 +23,6 @@ typedef __u16 __sum16;
>   
>   struct network_helper_opts {
>   	int timeout_ms;
> -	bool must_fail;
>   	int proto;
>   	/* +ve: Passed to listen() as-is.
>   	 *   0: Default when the test does not set
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
> index addf720428f7..eda67b629c67 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
> @@ -9,9 +9,7 @@
>   
>   static int run_test(int cgroup_fd, int server_fd, bool classid)
>   {
> -	struct network_helper_opts opts = {
> -		.must_fail = true,
> -	};
> +	struct network_helper_opts opts = {};
>   	struct connect4_dropper *skel;
>   	int fd, err = 0;
>   
> @@ -33,7 +31,7 @@ static int run_test(int cgroup_fd, int server_fd, bool classid)
>   	}
>   
>   	fd = connect_to_fd_opts(server_fd, &opts);
> -	if (fd < 0)
> +	if (fd < 0 && errno != EPERM)


The if test is incorrect.

>   		err = -1;
>   	else
>   		close(fd);

Beside close(-1) which is not good, it does not treat an OK_FD as an error and 
that is the whole test trying to catch.

pw-bot: cr