Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair()
From: Jakub Sitnicki <jakub@cloudflare.com>
Date: 2024-07-26 17:23:45
Also in:
bpf, linux-kselftest
Subsystem:
bpf [general] (safe dynamic programs and tools), bpf [selftests] (test runners & infrastructure), kernel selftest framework, the rest · Maintainers:
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Shuah Khan, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2024-07-31 · Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair() · Michal Luczaj <hidden>
- 2024-07-30 · Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair() · Jakub Sitnicki <jakub@cloudflare.com>
- 2024-07-24 · [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair() · Michal Luczaj <hidden>
On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
Extend the function to allow creating socket pairs of SOCK_STREAM, SOCK_DGRAM and SOCK_SEQPACKET. Adapt direct callers and leave further cleanups for the following patch. Suggested-by: Jakub Sitnicki <jakub@cloudflare.com> Signed-off-by: Michal Luczaj <redacted> --- .../selftests/bpf/prog_tests/sockmap_basic.c | 19 +-- .../selftests/bpf/prog_tests/sockmap_helpers.h | 138 ++++++++++++++------- 2 files changed, 96 insertions(+), 61 deletions(-)
[...]
quoted hunk
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h index e880f97bc44d..77b73333f091 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
[...]
+static inline int create_pair(int family, int sotype, int *p0, int *p1)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int s, c, p, err;
+
+ s = socket_loopback(family, sotype);
+ if (s < 0)
+ return s;
+
+ err = xgetsockname(s, sockaddr(&addr), &len);
+ if (err)
+ goto close_s;
+
+ c = xsocket(family, sotype, 0);
+ if (c < 0) {
+ err = c;
+ goto close_s;
+ }
+
+ err = connect(c, sockaddr(&addr), len);
+ if (err) {
+ if (errno != EINPROGRESS) {
+ FAIL_ERRNO("connect");
+ goto close_c;
+ }
+
+ err = poll_connect(c, IO_TIMEOUT_SEC);
+ if (err) {
+ FAIL_ERRNO("poll_connect");
+ goto close_c;
+ }
+ }
+
+ switch (sotype & SOCK_TYPE_MASK) {
+ case SOCK_DGRAM:
+ err = xgetsockname(c, sockaddr(&addr), &len);
+ if (err)
+ goto close_c;
+
+ err = xconnect(s, sockaddr(&addr), len);
+ if (!err) {
+ *p0 = s;
+ *p1 = c;
+ return err;
+ }
+ break;
+ case SOCK_STREAM:
+ case SOCK_SEQPACKET:
+ p = xaccept_nonblock(s, NULL, NULL);
+ if (p >= 0) {
+ *p0 = p;
+ *p1 = c;
+ goto close_s;
+ }
+
+ err = p;
+ break;
+ default:
+ FAIL("Unsupported socket type %#x", sotype);
+ err = -EOPNOTSUPP;
+ }
+
+close_c:
+ close(c);
+close_s:
+ close(s);
+ return err;
+}I was going to suggest that a single return path for success is better than two (diff below), but I see that this is what you ended up with after patch 6. So I think we can leave it as is. ---
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index 77b73333f091..ed266c6c0117 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h@@ -408,28 +408,31 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1) goto close_c; err = xconnect(s, sockaddr(&addr), len); - if (!err) { - *p0 = s; - *p1 = c; - return err; - } + if (err) + goto close_c; + + p = s; break; case SOCK_STREAM: case SOCK_SEQPACKET: p = xaccept_nonblock(s, NULL, NULL); - if (p >= 0) { - *p0 = p; - *p1 = c; - goto close_s; + if (p < 0) { + err = p; + goto close_c; } - err = p; + xclose(s); break; default: FAIL("Unsupported socket type %#x", sotype); err = -EOPNOTSUPP; + goto close_c; } + *p0 = p; + *p1 = c; + return 0; + close_c: close(c); close_s: