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

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

On 7/12/24 12:04 AM, 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    | 10 ++----
>   3 files changed, 5 insertions(+), 37 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..04e82a486102 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
> @@ -9,9 +9,6 @@
>   
>   static int run_test(int cgroup_fd, int server_fd, bool classid)
>   {
> -	struct network_helper_opts opts = {
> -		.must_fail = true,
> -	};
>   	struct connect4_dropper *skel;
>   	int fd, err = 0;
>   
> @@ -32,11 +29,10 @@ static int run_test(int cgroup_fd, int server_fd, bool classid)
>   		goto out;
>   	}
>   
> -	fd = connect_to_fd_opts(server_fd, &opts);
> -	if (fd < 0)
> -		err = -1;
> -	else
> +	fd = connect_to_fd_opts(server_fd, NULL);
> +	if (fd >= 0)
>   		close(fd);
> +	err = errno != EPERM ? -1 : 0;

Still not right. Can errno be used when fd >= 0? That makes the fd >= 0 case 
flaky. fd >= 0 should always be treated as error here (i.e. err = -1). just 
always set err = -1 in the fd >= 0 case. No need to be smart here to save a few 
lines.

Please take time to review the patches before posting to the list.

pw-bot: cr