diff mbox series

[bpf,1/6] selftest/bpf: Support more socket types in create_pair()

Message ID 20240724-sockmap-selftest-fixes-v1-1-46165d224712@rbox.co (mailing list archive)
State Accepted
Commit 190de5449973056f416c855b11fdfee2fc18f550
Headers show
Series selftest/bpf: Various sockmap-related fixes | expand

Commit Message

Michal Luczaj July 24, 2024, 11:32 a.m. UTC 
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 <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c       |  19 +--
 .../selftests/bpf/prog_tests/sockmap_helpers.h     | 138 ++++++++++++++-------
 2 files changed, 96 insertions(+), 61 deletions(-)

Comments

Jakub Sitnicki July 26, 2024, 5:23 p.m. UTC | #1 
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 <mhal@rbox.co>
> ---
>  .../selftests/bpf/prog_tests/sockmap_basic.c       |  19 +--
>  .../selftests/bpf/prog_tests/sockmap_helpers.h     | 138 ++++++++++++++-------
>  2 files changed, 96 insertions(+), 61 deletions(-)
>

[...]

> 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:
Michal Luczaj July 26, 2024, 8:29 p.m. UTC | #2 
On 7/26/24 19:23, Jakub Sitnicki wrote:
> 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.
> [...]

And speaking of which, would you rather have patch 1 and 6 squashed?
Jakub Sitnicki July 30, 2024, 5:13 p.m. UTC | #3 
On Fri, Jul 26, 2024 at 10:29 PM +02, Michal Luczaj wrote:
> On 7/26/24 19:23, Jakub Sitnicki wrote:
>> 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.
>> [...]
>
> And speaking of which, would you rather have patch 1 and 6 squashed?

Don't have a straight answer, sorry . Would have to see if the diff is
clear enough after squashing it. Use your best judgement.

It's certainly fine with me to review the steps that were taken to
massage the code.
Michal Luczaj July 31, 2024, 10:05 a.m. UTC | #4 
On 7/30/24 19:13, Jakub Sitnicki wrote:
> On Fri, Jul 26, 2024 at 10:29 PM +02, Michal Luczaj wrote:
>> On 7/26/24 19:23, Jakub Sitnicki wrote:
>>> 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.
>>> [...]
>>
>> And speaking of which, would you rather have patch 1 and 6 squashed?
> 
> Don't have a straight answer, sorry . Would have to see if the diff is
> clear enough after squashing it. Use your best judgement.
> 
> It's certainly fine with me to review the steps that were taken to
> massage the code.

That's what I've assumed, thanks. So here's the bpf-next based respin:
https://lore.kernel.org/bpf/20240731-selftest-sockmap-fixes-v2-0-08a0c73abed2@rbox.co
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 1337153eb0ad..5b17d69c9ee6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -451,11 +451,11 @@  static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
 #define MAX_EVENTS 10
 static void test_sockmap_skb_verdict_shutdown(void)
 {
+	int n, err, map, verdict, c1 = -1, p1 = -1;
 	struct epoll_event ev, events[MAX_EVENTS];
-	int n, err, map, verdict, s, c1 = -1, p1 = -1;
 	struct test_sockmap_pass_prog *skel;
-	int epollfd;
 	int zero = 0;
+	int epollfd;
 	char b;
 
 	skel = test_sockmap_pass_prog__open_and_load();
@@ -469,10 +469,7 @@  static void test_sockmap_skb_verdict_shutdown(void)
 	if (!ASSERT_OK(err, "bpf_prog_attach"))
 		goto out;
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (s < 0)
-		goto out;
-	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
+	err = create_pair(AF_INET, SOCK_STREAM, &c1, &p1);
 	if (err < 0)
 		goto out;
 
@@ -570,16 +567,12 @@  static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 
 static void test_sockmap_skb_verdict_peek_helper(int map)
 {
-	int err, s, c1, p1, zero = 0, sent, recvd, avail;
+	int err, c1, p1, zero = 0, sent, recvd, avail;
 	char snd[256] = "0123456789";
 	char rcv[256] = "0";
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (!ASSERT_GT(s, -1, "socket_loopback(s)"))
-		return;
-
-	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
-	if (!ASSERT_OK(err, "create_pairs(s)"))
+	err = create_pair(AF_INET, SOCK_STREAM, &c1, &p1);
+	if (!ASSERT_OK(err, "create_pair()"))
 		return;
 
 	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);
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
@@ -3,6 +3,9 @@ 
 
 #include <linux/vm_sockets.h>
 
+/* include/linux/net.h */
+#define SOCK_TYPE_MASK 0xf
+
 #define IO_TIMEOUT_SEC 30
 #define MAX_STRERR_LEN 256
 #define MAX_TEST_NAME 80
@@ -312,54 +315,6 @@  static inline int add_to_sockmap(int sock_mapfd, int fd1, int fd2)
 	return xbpf_map_update_elem(sock_mapfd, &key, &value, BPF_NOEXIST);
 }
 
-static inline int create_pair(int s, int family, int sotype, int *c, int *p)
-{
-	struct sockaddr_storage addr;
-	socklen_t len;
-	int err = 0;
-
-	len = sizeof(addr);
-	err = xgetsockname(s, sockaddr(&addr), &len);
-	if (err)
-		return err;
-
-	*c = xsocket(family, sotype, 0);
-	if (*c < 0)
-		return errno;
-	err = xconnect(*c, sockaddr(&addr), len);
-	if (err) {
-		err = errno;
-		goto close_cli0;
-	}
-
-	*p = xaccept_nonblock(s, NULL, NULL);
-	if (*p < 0) {
-		err = errno;
-		goto close_cli0;
-	}
-	return err;
-close_cli0:
-	close(*c);
-	return err;
-}
-
-static inline int create_socket_pairs(int s, int family, int sotype,
-				      int *c0, int *c1, int *p0, int *p1)
-{
-	int err;
-
-	err = create_pair(s, family, sotype, c0, p0);
-	if (err)
-		return err;
-
-	err = create_pair(s, family, sotype, c1, p1);
-	if (err) {
-		close(*c0);
-		close(*p0);
-	}
-	return err;
-}
-
 static inline int enable_reuseport(int s, int progfd)
 {
 	int err, one = 1;
@@ -412,5 +367,92 @@  static inline int socket_loopback(int family, int sotype)
 	return socket_loopback_reuseport(family, sotype, -1);
 }
 
+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;
+}
+
+static inline int create_socket_pairs(int s, int family, int sotype,
+				      int *c0, int *c1, int *p0, int *p1)
+{
+	int err;
+
+	err = create_pair(family, sotype, c0, p0);
+	if (err)
+		return err;
+
+	err = create_pair(family, sotype, c1, p1);
+	if (err) {
+		close(*c0);
+		close(*p0);
+	}
+
+	return err;
+}
 
 #endif // __SOCKMAP_HELPERS__