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

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: