diff mbox series

[bpf-next,v10,06/12] selftests/bpf: Add expect_errno for network_helper_opts

Message ID 35ab4f6f094e3c700aa9ec20ee6d6d1a91284b5a.1720405046.git.tanggeliang@kylinos.cn (mailing list archive)
State New
Headers show
Series use network helpers, part 8 | expand

Commit Message

Geliang Tang July 8, 2024, 2:29 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

The errno EPERM is skipped in connect_fd_to_addr() by cgroup_v1v2 tests.
More generally, it makes sense to add a struct member "expect_errno" for
network_helper_opts to identify the expect errno to be skipped.

Correspondingly, connect_fd_to_addr() helper needs to add a new parameter
"expect_errno" too to accept "opts->expect_errno" passed from the caller
connect_to_addr() or connect_fd_to_fd().

With this change, only need to set "expect_errno" as EPERM in run_test()
in prog_tests/cgroup_v1v2.c.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/network_helpers.c        | 11 +++++++----
 tools/testing/selftests/bpf/network_helpers.h        |  1 +
 tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

Eduard Zingerman July 8, 2024, 7:15 p.m. UTC | #1 
On Mon, 2024-07-08 at 10:29 +0800, Geliang Tang wrote:

[...]

> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index fcda6b2333ad..14d161d35248 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -24,6 +24,7 @@ typedef __u16 __sum16;
>  struct network_helper_opts {
>  	int timeout_ms;
>  	bool must_fail;
> +	int expect_errno;

I think this option obfuscates actual test cases.
Each helper that accepts network_helper_opts as a parameter, does
multiple system calls. It is not obvious which of these calls is
expected to fail.

>  	int proto;
>  	/* The backlog argument for listen(), defines the maximum length to which
>  	 * the queue of pending connections for sockfd may grow.

[...]
Martin KaFai Lau July 8, 2024, 8:06 p.m. UTC | #2 
On 7/8/24 12:15 PM, Eduard Zingerman wrote:
> On Mon, 2024-07-08 at 10:29 +0800, Geliang Tang wrote:
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
>> index fcda6b2333ad..14d161d35248 100644
>> --- a/tools/testing/selftests/bpf/network_helpers.h
>> +++ b/tools/testing/selftests/bpf/network_helpers.h
>> @@ -24,6 +24,7 @@ typedef __u16 __sum16;
>>   struct network_helper_opts {
>>   	int timeout_ms;
>>   	bool must_fail;
>> +	int expect_errno;
> 
> I think this option obfuscates actual test cases.
> Each helper that accepts network_helper_opts as a parameter, does
> multiple system calls. It is not obvious which of these calls is
> expected to fail.

+1

Please no. On top of that, handling specific errno is not something that belongs 
to the generic network_helpers.c. The individual test should check for the errno 
that it expects on the error path.

Is it for make_client()? The caller of make_client can check the errno instead, no?

[ The same goes for the existing "bool must_fail;" but it can be left for a 
separate cleanup patch ]

pw-bot: cr

> 
>>   	int proto;
>>   	/* The backlog argument for listen(), defines the maximum length to which
>>   	 * the queue of pending connections for sockfd may grow.
> 
> [...]
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 475a5a04e61e..062170d6be1c 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -279,7 +279,8 @@  int client_socket(int family, int type,
 
 static int connect_fd_to_addr(int fd,
 			      const struct sockaddr_storage *addr,
-			      socklen_t addrlen, const bool must_fail)
+			      socklen_t addrlen, const bool must_fail,
+			      const int expect_errno)
 {
 	int ret;
 
@@ -290,7 +291,7 @@  static int connect_fd_to_addr(int fd,
 			log_err("Unexpected success to connect to server");
 			return -1;
 		}
-		if (errno != EPERM) {
+		if (errno != expect_errno) {
 			log_err("Unexpected error from connect to server");
 			return -1;
 		}
@@ -318,7 +319,8 @@  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_to_addr(fd, addr, addrlen, opts->must_fail,
+			       opts->expect_errno))
 		goto error_close;
 
 	return fd;
@@ -386,7 +388,8 @@  int connect_fd_to_fd(int client_fd, int server_fd,
 		return -1;
 	}
 
-	if (connect_fd_to_addr(client_fd, &addr, len, opts->must_fail))
+	if (connect_fd_to_addr(client_fd, &addr, len, opts->must_fail,
+			       opts->expect_errno))
 		return -1;
 
 	return 0;
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index fcda6b2333ad..14d161d35248 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -24,6 +24,7 @@  typedef __u16 __sum16;
 struct network_helper_opts {
 	int timeout_ms;
 	bool must_fail;
+	int expect_errno;
 	int proto;
 	/* The backlog argument for listen(), defines the maximum length to which
 	 * the queue of pending connections for sockfd may grow.
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
index 9709c8db7275..ff477163f0ea 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
@@ -11,6 +11,7 @@  static int run_test(int cgroup_fd, int server_fd, bool classid)
 {
 	struct network_helper_opts opts = {
 		.must_fail = true,
+		.expect_errno = EPERM,
 	};
 	struct connect4_dropper *skel;
 	int fd, err = 0;