From: Geliang Tang tanggeliang@kylinos.cn
v3: - patch 2: - clear errno before connect_to_fd_opts. - print err logs in run_test. - set err to -1 when fd >= 0. - patch 3: - drop "int err".
v2: - update patch 2 as Martin suggested.
This is the 9th part of series "use network helpers" all BPF selftests wide.
Patches 1-2 update network helpers interfaces suggested by Martin. Patch 3 adds a new helper connect_to_addr_str() as Martin suggested instead of adding connect_fd_to_addr_str(). Patch 4 uses this newly added helper in make_client(). Patch 5 uses make_client() in sk_lookup and drop make_socket().
Geliang Tang (5): selftests/bpf: Drop type of connect_to_fd_opts selftests/bpf: Drop must_fail from network_helper_opts selftests/bpf: Add connect_to_addr_str helper selftests/bpf: Use connect_to_addr_str in sk_lookup selftests/bpf: Drop make_socket in sk_lookup
tools/testing/selftests/bpf/network_helpers.c | 65 ++++++-------- tools/testing/selftests/bpf/network_helpers.h | 5 +- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 2 +- .../selftests/bpf/prog_tests/cgroup_v1v2.c | 16 ++-- .../selftests/bpf/prog_tests/sk_lookup.c | 84 ++++--------------- 5 files changed, 56 insertions(+), 116 deletions(-)
From: Geliang Tang tanggeliang@kylinos.cn
The "type" parameter of connect_to_fd_opts() is redundant of "server_fd". Since the "type" can be obtained inside by invoking getsockopt(SO_TYPE), without passing it in as a parameter.
This patch drops the "type" parameter of connect_to_fd_opts() and updates its callers.
Suggested-by: Martin KaFai Lau martin.lau@kernel.org Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- tools/testing/selftests/bpf/network_helpers.c | 21 ++++++++++--------- tools/testing/selftests/bpf/network_helpers.h | 2 +- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 2 +- .../selftests/bpf/prog_tests/cgroup_v1v2.c | 4 ++-- 4 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c index e0cba4178e41..15e0e0bb7553 100644 --- a/tools/testing/selftests/bpf/network_helpers.c +++ b/tools/testing/selftests/bpf/network_helpers.c @@ -328,14 +328,21 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add return -1; }
-int connect_to_fd_opts(int server_fd, int type, const struct network_helper_opts *opts) +int connect_to_fd_opts(int server_fd, const struct network_helper_opts *opts) { struct sockaddr_storage addr; - socklen_t addrlen; + socklen_t addrlen, optlen; + int type;
if (!opts) opts = &default_opts;
+ optlen = sizeof(type); + if (getsockopt(server_fd, SOL_SOCKET, SO_TYPE, &type, &optlen)) { + log_err("getsockopt(SOL_TYPE)"); + return -1; + } + addrlen = sizeof(addr); if (getsockname(server_fd, (struct sockaddr *)&addr, &addrlen)) { log_err("Failed to get server addr"); @@ -350,14 +357,8 @@ int connect_to_fd(int server_fd, int timeout_ms) struct network_helper_opts opts = { .timeout_ms = timeout_ms, }; - int type, protocol; socklen_t optlen; - - optlen = sizeof(type); - if (getsockopt(server_fd, SOL_SOCKET, SO_TYPE, &type, &optlen)) { - log_err("getsockopt(SOL_TYPE)"); - return -1; - } + int protocol;
optlen = sizeof(protocol); if (getsockopt(server_fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen)) { @@ -366,7 +367,7 @@ int connect_to_fd(int server_fd, int timeout_ms) } opts.proto = protocol;
- return connect_to_fd_opts(server_fd, type, &opts); + return connect_to_fd_opts(server_fd, &opts); }
int connect_fd_to_fd(int client_fd, int server_fd, int timeout_ms) diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h index aac5b94d6379..5b548c0c60de 100644 --- a/tools/testing/selftests/bpf/network_helpers.h +++ b/tools/testing/selftests/bpf/network_helpers.h @@ -71,7 +71,7 @@ int client_socket(int family, int type, int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t len, const struct network_helper_opts *opts); int connect_to_fd(int server_fd, int timeout_ms); -int connect_to_fd_opts(int server_fd, int type, const struct network_helper_opts *opts); +int connect_to_fd_opts(int server_fd, const struct network_helper_opts *opts); int connect_fd_to_fd(int client_fd, int server_fd, int timeout_ms); int fastopen_connect(int server_fd, const char *data, unsigned int data_len, int timeout_ms); diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 63422f4f3896..1d494b4453f4 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -49,7 +49,7 @@ static bool start_test(char *addr_str, goto err;
/* connect to server */ - *cli_fd = connect_to_fd_opts(*srv_fd, SOCK_STREAM, cli_opts); + *cli_fd = connect_to_fd_opts(*srv_fd, cli_opts); if (!ASSERT_NEQ(*cli_fd, -1, "connect_to_fd_opts")) goto err;
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c index 9709c8db7275..addf720428f7 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c @@ -32,7 +32,7 @@ static int run_test(int cgroup_fd, int server_fd, bool classid) goto out; }
- fd = connect_to_fd_opts(server_fd, SOCK_STREAM, &opts); + fd = connect_to_fd_opts(server_fd, &opts); if (fd < 0) err = -1; else @@ -52,7 +52,7 @@ void test_cgroup_v1v2(void) server_fd = start_server(AF_INET, SOCK_STREAM, NULL, port, 0); if (!ASSERT_GE(server_fd, 0, "server_fd")) return; - client_fd = connect_to_fd_opts(server_fd, SOCK_STREAM, &opts); + client_fd = connect_to_fd_opts(server_fd, &opts); if (!ASSERT_GE(client_fd, 0, "client_fd")) { close(server_fd); return;
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 | 14 +++++---- 3 files changed, 10 insertions(+), 36 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..e91099c637b0 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,16 @@ 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) + errno = 0; + fd = connect_to_fd_opts(server_fd, NULL); + if (fd >= 0) { + log_err("Unexpected success to connect to server"); err = -1; - else close(fd); + } else if (errno != EPERM) { + log_err("Unexpected error from connect to server"); + err = -1; + } out: connect4_dropper__destroy(skel); return err;
On 7/17/24 11:22 PM, Geliang Tang wrote:
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");
There was log_err on connect() failure.
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))
log_err is gone now.
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))
Same here.
return -1;return 0;
From: Geliang Tang tanggeliang@kylinos.cn
Similar to connect_to_addr() helper for connecting to a server with the given sockaddr_storage type address, this patch adds a new helper named connect_to_addr_str() for connecting to a server with the given string type address "addr_str", together with its "family" and "port" as other parameters of connect_to_addr_str().
In connect_to_addr_str(), the parameters "family", "addr_str" and "port" are used to create a sockaddr_storage type address "addr" by invoking make_sockaddr(). Then pass this "addr" together with "addrlen", "type" and "opts" to connect_to_addr().
Suggested-by: Martin KaFai Lau martin.lau@kernel.org Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- tools/testing/selftests/bpf/network_helpers.c | 15 +++++++++++++++ tools/testing/selftests/bpf/network_helpers.h | 2 ++ 2 files changed, 17 insertions(+)
diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c index 48c27c810db7..a71af637bcbc 100644 --- a/tools/testing/selftests/bpf/network_helpers.c +++ b/tools/testing/selftests/bpf/network_helpers.c @@ -301,6 +301,21 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add return -1; }
+int connect_to_addr_str(int family, int type, const char *addr_str, __u16 port, + const struct network_helper_opts *opts) +{ + struct sockaddr_storage addr; + socklen_t addrlen; + + if (!opts) + opts = &default_opts; + + if (make_sockaddr(family, addr_str, port, &addr, &addrlen)) + return -1; + + return connect_to_addr(type, &addr, addrlen, opts); +} + int connect_to_fd_opts(int server_fd, const struct network_helper_opts *opts) { struct sockaddr_storage addr; diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h index f39eeb5a4594..cce56955371f 100644 --- a/tools/testing/selftests/bpf/network_helpers.h +++ b/tools/testing/selftests/bpf/network_helpers.h @@ -69,6 +69,8 @@ int client_socket(int family, int type, const struct network_helper_opts *opts); int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t len, const struct network_helper_opts *opts); +int connect_to_addr_str(int family, int type, const char *addr_str, __u16 port, + const struct network_helper_opts *opts); int connect_to_fd(int server_fd, int timeout_ms); int connect_to_fd_opts(int server_fd, const struct network_helper_opts *opts); int connect_fd_to_fd(int client_fd, int server_fd, int timeout_ms);
From: Geliang Tang tanggeliang@kylinos.cn
This patch uses the new helper connect_to_addr_str() in make_client() instead of using local defined function make_socket() + connect(). This local function can be dropped latter.
A new parameter "expect_errno" is added for make_client() too to allow different "expect_errno" is passed to make_client(). It is used to check with "errno" after invoking connect_to_addr_str().
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/sk_lookup.c | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c index ae87c00867ba..beea7866b37f 100644 --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c @@ -229,25 +229,19 @@ static int make_server(int sotype, const char *ip, int port, return -1; }
-static int make_client(int sotype, const char *ip, int port) +static int make_client(int sotype, const char *ip, int port, int expect_errno) { - struct sockaddr_storage addr = {0}; - int err, fd; - - fd = make_socket(sotype, ip, port, &addr); - if (fd < 0) - return -1; + int fd;
- err = connect(fd, (void *)&addr, inetaddr_len(&addr)); - if (CHECK(err, "make_client", "connect")) { + fd = connect_to_addr_str(is_ipv6(ip) ? AF_INET6 : AF_INET, + sotype, ip, port, NULL); + if (CHECK(fd < 0 && (!expect_errno || errno != expect_errno), + "make_client", "connect_to_addr_str")) { log_err("failed to connect client socket"); - goto fail; + return -1; }
return fd; -fail: - close(fd); - return -1; }
static __u64 socket_cookie(int fd) @@ -646,7 +640,7 @@ static void run_lookup_prog(const struct test *t) goto close; }
- client_fd = make_client(t->sotype, t->connect_to.ip, t->connect_to.port); + client_fd = make_client(t->sotype, t->connect_to.ip, t->connect_to.port, 0); if (client_fd < 0) goto close;
@@ -1152,7 +1146,7 @@ static void run_sk_assign_connected(struct test_sk_lookup *skel, if (server_fd < 0) return;
- connected_fd = make_client(sotype, EXT_IP4, EXT_PORT); + connected_fd = make_client(sotype, EXT_IP4, EXT_PORT, 0); if (connected_fd < 0) goto out_close_server;
@@ -1166,7 +1160,7 @@ static void run_sk_assign_connected(struct test_sk_lookup *skel, goto out_close_connected;
/* Try to redirect TCP SYN / UDP packet to a connected socket */ - client_fd = make_client(sotype, EXT_IP4, EXT_PORT); + client_fd = make_client(sotype, EXT_IP4, EXT_PORT, 0); if (client_fd < 0) goto out_unlink_prog; if (sotype == SOCK_DGRAM) {
On 7/17/24 11:22 PM, Geliang Tang wrote:
From: Geliang Tang tanggeliang@kylinos.cn
This patch uses the new helper connect_to_addr_str() in make_client() instead of using local defined function make_socket() + connect(). This local function can be dropped latter.
A new parameter "expect_errno" is added for make_client() too to allow different "expect_errno" is passed to make_client(). It is used to check with "errno" after invoking connect_to_addr_str().
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn
.../selftests/bpf/prog_tests/sk_lookup.c | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c index ae87c00867ba..beea7866b37f 100644 --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c @@ -229,25 +229,19 @@ static int make_server(int sotype, const char *ip, int port, return -1; } -static int make_client(int sotype, const char *ip, int port) +static int make_client(int sotype, const char *ip, int port, int expect_errno) {
- struct sockaddr_storage addr = {0};
- int err, fd;
- fd = make_socket(sotype, ip, port, &addr);
- if (fd < 0)
return -1;
- int fd;
- err = connect(fd, (void *)&addr, inetaddr_len(&addr));
- if (CHECK(err, "make_client", "connect")) {
- fd = connect_to_addr_str(is_ipv6(ip) ? AF_INET6 : AF_INET,
sotype, ip, port, NULL);- if (CHECK(fd < 0 && (!expect_errno || errno != expect_errno),
"make_client", "connect_to_addr_str")) {
goto fail;
return -1;return fd; -fail:
- close(fd);
- return -1; }
static __u64 socket_cookie(int fd) @@ -646,7 +640,7 @@ static void run_lookup_prog(const struct test *t) goto close; }
- client_fd = make_client(t->sotype, t->connect_to.ip, t->connect_to.port);
- client_fd = make_client(t->sotype, t->connect_to.ip, t->connect_to.port, 0); if (client_fd < 0) goto close;
@@ -1152,7 +1146,7 @@ static void run_sk_assign_connected(struct test_sk_lookup *skel, if (server_fd < 0) return;
- connected_fd = make_client(sotype, EXT_IP4, EXT_PORT);
- connected_fd = make_client(sotype, EXT_IP4, EXT_PORT, 0); if (connected_fd < 0) goto out_close_server;
@@ -1166,7 +1160,7 @@ static void run_sk_assign_connected(struct test_sk_lookup *skel, goto out_close_connected; /* Try to redirect TCP SYN / UDP packet to a connected socket */
- client_fd = make_client(sotype, EXT_IP4, EXT_PORT);
- client_fd = make_client(sotype, EXT_IP4, EXT_PORT, 0);
No errno is expected in all these cases. directly call connect_to_addr_str() instead of calling make_client.
if (client_fd < 0) goto out_unlink_prog; if (sotype == SOCK_DGRAM) {
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", "unexpected error %d\n", errno)) goto out_close_client;
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);
- 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);
- 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.
linux-kselftest-mirror@lists.linaro.org
-
Geliang Tang -
Martin KaFai Lau