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@xxxxxxxxxxxxxx>
> Signed-off-by: Michal Luczaj <mhal@xxxxxxx>
> ---
> .../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:
