diff mbox series

[bpf-next,v3,5/5] selftests/bpf: Drop make_socket in sk_lookup

Message ID bab06e6d0f71064dadb43f16e330b9e43fad1c95.1721282219.git.tanggeliang@kylinos.cn (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series use network helpers, part 9 | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 7 this patch: 7
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: sdf@fomichev.me
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Geliang Tang July 18, 2024, 6:22 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

Use local helper make_client() in drop_on_lookup(), drop_on_reuseport()
and run_multi_prog_lookup() instead of using make_socket() + connect().
Then make_socket() and inetaddr_len() can be dropped.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/bpf/prog_tests/sk_lookup.c      | 58 ++-----------------
 1 file changed, 6 insertions(+), 52 deletions(-)

Comments

Martin KaFai Lau July 18, 2024, 10:38 p.m. UTC | #1 
On 7/17/24 11:22 PM, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Use local helper make_client() in drop_on_lookup(), drop_on_reuseport()
> and run_multi_prog_lookup() instead of using make_socket() + connect().
> Then make_socket() and inetaddr_len() can be dropped.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>   .../selftests/bpf/prog_tests/sk_lookup.c      | 58 ++-----------------
>   1 file changed, 6 insertions(+), 52 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
> index beea7866b37f..3d1c315841b7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
> @@ -108,46 +108,6 @@ static int attach_reuseport(int sock_fd, struct bpf_program *reuseport_prog)
>   	return 0;
>   }
>   
> -static socklen_t inetaddr_len(const struct sockaddr_storage *addr)
> -{
> -	return (addr->ss_family == AF_INET ? sizeof(struct sockaddr_in) :
> -		addr->ss_family == AF_INET6 ? sizeof(struct sockaddr_in6) : 0);
> -}
> -
> -static int make_socket(int sotype, const char *ip, int port,
> -		       struct sockaddr_storage *addr)
> -{
> -	struct timeval timeo = { .tv_sec = IO_TIMEOUT_SEC };
> -	int err, family, fd;
> -
> -	family = is_ipv6(ip) ? AF_INET6 : AF_INET;
> -	err = make_sockaddr(family, ip, port, addr, NULL);
> -	if (CHECK(err, "make_address", "failed\n"))
> -		return -1;
> -
> -	fd = socket(addr->ss_family, sotype, 0);
> -	if (CHECK(fd < 0, "socket", "failed\n")) {
> -		log_err("failed to make socket");
> -		return -1;
> -	}
> -
> -	err = setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeo, sizeof(timeo));
> -	if (CHECK(err, "setsockopt(SO_SNDTIMEO)", "failed\n")) {
> -		log_err("failed to set SNDTIMEO");
> -		close(fd);
> -		return -1;
> -	}
> -
> -	err = setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
> -	if (CHECK(err, "setsockopt(SO_RCVTIMEO)", "failed\n")) {
> -		log_err("failed to set RCVTIMEO");
> -		close(fd);
> -		return -1;
> -	}
> -
> -	return fd;
> -}
> -
>   static int setsockopts(int fd, void *opts)
>   {
>   	struct cb_opts *co = (struct cb_opts *)opts;
> @@ -856,8 +816,7 @@ static void test_redirect_lookup(struct test_sk_lookup *skel)
>   
>   static void drop_on_lookup(const struct test *t)
>   {
> -	struct sockaddr_storage dst = {};
> -	int client_fd, server_fd, err;
> +	int client_fd, server_fd, err = 0;
>   	struct bpf_link *lookup_link;
>   	ssize_t n;
>   
> @@ -870,12 +829,11 @@ static void drop_on_lookup(const struct test *t)
>   	if (server_fd < 0)
>   		goto detach;
>   
> -	client_fd = make_socket(t->sotype, t->connect_to.ip,
> -				t->connect_to.port, &dst);
> +	client_fd = make_client(t->sotype, t->connect_to.ip,
> +				t->connect_to.port, ECONNREFUSED);
>   	if (client_fd < 0)
>   		goto close_srv;
>   
> -	err = connect(client_fd, (void *)&dst, inetaddr_len(&dst));
>   	if (t->sotype == SOCK_DGRAM) {
>   		err = send_byte(client_fd);
>   		if (err)
> @@ -970,7 +928,6 @@ static void test_drop_on_lookup(struct test_sk_lookup *skel)
>   
>   static void drop_on_reuseport(const struct test *t)
>   {
> -	struct sockaddr_storage dst = { 0 };
>   	int client, server1, server2, err;
>   	struct bpf_link *lookup_link;
>   	ssize_t n;
> @@ -994,12 +951,11 @@ static void drop_on_reuseport(const struct test *t)
>   	if (server2 < 0)
>   		goto close_srv1;
>   
> -	client = make_socket(t->sotype, t->connect_to.ip,
> -			     t->connect_to.port, &dst);
> +	client = make_client(t->sotype, t->connect_to.ip,
> +			     t->connect_to.port, ECONNREFUSED);
>   	if (client < 0)
>   		goto close_srv2;
>   
> -	err = connect(client, (void *)&dst, inetaddr_len(&dst));
>   	if (t->sotype == SOCK_DGRAM) {
>   		err = send_byte(client);
>   		if (err)
> @@ -1209,7 +1165,6 @@ struct test_multi_prog {
>   
>   static void run_multi_prog_lookup(const struct test_multi_prog *t)
>   {
> -	struct sockaddr_storage dst = {};
>   	int map_fd, server_fd, client_fd;
>   	struct bpf_link *link1, *link2;
>   	int prog_idx, done, err;
> @@ -1242,11 +1197,10 @@ static void run_multi_prog_lookup(const struct test_multi_prog *t)
>   	if (err)
>   		goto out_close_server;
>   
> -	client_fd = make_socket(SOCK_STREAM, EXT_IP4, EXT_PORT, &dst);
> +	client_fd = make_client(SOCK_STREAM, EXT_IP4, EXT_PORT, t->expect_errno);
>   	if (client_fd < 0)
>   		goto out_close_server;
>   
> -	err = connect(client_fd, (void *)&dst, inetaddr_len(&dst));
>   	if (CHECK(err && !t->expect_errno, "connect",

I haven't looked at the details in run_multi_prog_lookup(). I am quite sure this 
CHECK is wrong or at least unnecessary. "err" does not have the return value of 
connect() now but the t->expect_errno is for connect(), so what is the CHECK 
testing?

imo, burying the errno checking in make_client is confusing.

On top of removing make_socket, lets remove the make_client() also. Directly 
call connect_to_addr_str() instead. The caller of connect_to_addr_str() can 
handle the returned fd and errno. The drop_on_{lookup, reuseport} should handle 
them differently based on SOCK_STREAM or SOCK_DGRAM.

In the next respin, please take time to change the error handling properly after 
the refactoring/cleanup.

I am going to apply patch 1-3 with the small nits related to "log_err" in patch 2.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
index beea7866b37f..3d1c315841b7 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
@@ -108,46 +108,6 @@  static int attach_reuseport(int sock_fd, struct bpf_program *reuseport_prog)
 	return 0;
 }
 
-static socklen_t inetaddr_len(const struct sockaddr_storage *addr)
-{
-	return (addr->ss_family == AF_INET ? sizeof(struct sockaddr_in) :
-		addr->ss_family == AF_INET6 ? sizeof(struct sockaddr_in6) : 0);
-}
-
-static int make_socket(int sotype, const char *ip, int port,
-		       struct sockaddr_storage *addr)
-{
-	struct timeval timeo = { .tv_sec = IO_TIMEOUT_SEC };
-	int err, family, fd;
-
-	family = is_ipv6(ip) ? AF_INET6 : AF_INET;
-	err = make_sockaddr(family, ip, port, addr, NULL);
-	if (CHECK(err, "make_address", "failed\n"))
-		return -1;
-
-	fd = socket(addr->ss_family, sotype, 0);
-	if (CHECK(fd < 0, "socket", "failed\n")) {
-		log_err("failed to make socket");
-		return -1;
-	}
-
-	err = setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeo, sizeof(timeo));
-	if (CHECK(err, "setsockopt(SO_SNDTIMEO)", "failed\n")) {
-		log_err("failed to set SNDTIMEO");
-		close(fd);
-		return -1;
-	}
-
-	err = setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
-	if (CHECK(err, "setsockopt(SO_RCVTIMEO)", "failed\n")) {
-		log_err("failed to set RCVTIMEO");
-		close(fd);
-		return -1;
-	}
-
-	return fd;
-}
-
 static int setsockopts(int fd, void *opts)
 {
 	struct cb_opts *co = (struct cb_opts *)opts;
@@ -856,8 +816,7 @@  static void test_redirect_lookup(struct test_sk_lookup *skel)
 
 static void drop_on_lookup(const struct test *t)
 {
-	struct sockaddr_storage dst = {};
-	int client_fd, server_fd, err;
+	int client_fd, server_fd, err = 0;
 	struct bpf_link *lookup_link;
 	ssize_t n;
 
@@ -870,12 +829,11 @@  static void drop_on_lookup(const struct test *t)
 	if (server_fd < 0)
 		goto detach;
 
-	client_fd = make_socket(t->sotype, t->connect_to.ip,
-				t->connect_to.port, &dst);
+	client_fd = make_client(t->sotype, t->connect_to.ip,
+				t->connect_to.port, ECONNREFUSED);
 	if (client_fd < 0)
 		goto close_srv;
 
-	err = connect(client_fd, (void *)&dst, inetaddr_len(&dst));
 	if (t->sotype == SOCK_DGRAM) {
 		err = send_byte(client_fd);
 		if (err)
@@ -970,7 +928,6 @@  static void test_drop_on_lookup(struct test_sk_lookup *skel)
 
 static void drop_on_reuseport(const struct test *t)
 {
-	struct sockaddr_storage dst = { 0 };
 	int client, server1, server2, err;
 	struct bpf_link *lookup_link;
 	ssize_t n;
@@ -994,12 +951,11 @@  static void drop_on_reuseport(const struct test *t)
 	if (server2 < 0)
 		goto close_srv1;
 
-	client = make_socket(t->sotype, t->connect_to.ip,
-			     t->connect_to.port, &dst);
+	client = make_client(t->sotype, t->connect_to.ip,
+			     t->connect_to.port, ECONNREFUSED);
 	if (client < 0)
 		goto close_srv2;
 
-	err = connect(client, (void *)&dst, inetaddr_len(&dst));
 	if (t->sotype == SOCK_DGRAM) {
 		err = send_byte(client);
 		if (err)
@@ -1209,7 +1165,6 @@  struct test_multi_prog {
 
 static void run_multi_prog_lookup(const struct test_multi_prog *t)
 {
-	struct sockaddr_storage dst = {};
 	int map_fd, server_fd, client_fd;
 	struct bpf_link *link1, *link2;
 	int prog_idx, done, err;
@@ -1242,11 +1197,10 @@  static void run_multi_prog_lookup(const struct test_multi_prog *t)
 	if (err)
 		goto out_close_server;
 
-	client_fd = make_socket(SOCK_STREAM, EXT_IP4, EXT_PORT, &dst);
+	client_fd = make_client(SOCK_STREAM, EXT_IP4, EXT_PORT, t->expect_errno);
 	if (client_fd < 0)
 		goto out_close_server;
 
-	err = connect(client_fd, (void *)&dst, inetaddr_len(&dst));
 	if (CHECK(err && !t->expect_errno, "connect",
 		  "unexpected error %d\n", errno))
 		goto out_close_client;