Re: [PATCH bpf-next v2 1/4] selftests/bpf: Replaces the usage of CHECK calls for ASSERTs in bpf_tcp_ca

On 11/18/23 1:42 PM, Yuran Pereira wrote:

bpf_tcp_ca uses the `CHECK` calls even though the use of ASSERT_ series of macros is preferred in the bpf selftests.

This patch replaces all `CHECK` calls for equivalent `ASSERT_` macro calls.

Signed-off-by: Yuran Pereira yuran.pereira@hotmail.com

.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 50 +++++++++---------- 1 file changed, 23 insertions(+), 27 deletions(-)

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 4aabeaa525d4..6d610b66ec38 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -20,15 +20,14 @@ static const unsigned int total_bytes = 10 * 1024 * 1024; static int expected_stg = 0xeB9F; -static int stop, duration; +static int stop; [...] @@ -108,26 +107,27 @@ static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map) sa6.sin6_family = AF_INET6; sa6.sin6_addr = in6addr_loopback; err = bind(lfd, (struct sockaddr *)&sa6, addrlen);

• if (CHECK(err == -1, "bind", "errno:%d\n", errno))

• if (!ASSERT_NEQ(err, -1, "bind")) goto done;
• err = getsockname(lfd, (struct sockaddr *)&sa6, &addrlen);

• if (CHECK(err == -1, "getsockname", "errno:%d\n", errno))

• if (!ASSERT_NEQ(err, -1, "getsockname")) goto done;
• err = listen(lfd, 1);

• if (CHECK(err == -1, "listen", "errno:%d\n", errno))

• if (!ASSERT_NEQ(err, -1, "listen")) goto done;

if (sk_stg_map) { err = bpf_map_update_elem(bpf_map__fd(sk_stg_map), &fd, &expected_stg, BPF_NOEXIST);

• if (CHECK(err, "bpf_map_update_elem(sk_stg_map)",
  

• 	  "err:%d errno:%d\n", err, errno))
  

• if (!ASSERT_OK(err, "bpf_map_update_elem(sk_stg_map)"))
  goto done;
  

  }

/* connect to server */ err = connect(fd, (struct sockaddr *)&sa6, addrlen);

• if (CHECK(err == -1, "connect", "errno:%d\n", errno))

• if (!ASSERT_NEQ(err, -1, "connect")) goto done;

if (sk_stg_map) { @@ -135,14 +135,13 @@ static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map) err = bpf_map_lookup_elem(bpf_map__fd(sk_stg_map), &fd, &tmp_stg);

• if (CHECK(!err || errno != ENOENT,
  

• 	  "bpf_map_lookup_elem(sk_stg_map)",
  

• 	  "err:%d errno:%d\n", err, errno))
  

• if (!ASSERT_NEQ(err, 0, "bpf_map_lookup_elem(sk_stg_map)") ||
  

!ASSERT_ERR(err, "bpf_map_lookup_elem(sk_stg_map)") might be simpler than !ASSERT_NEQ(..).

• 		!ASSERT_EQ(errno, ENOENT, "bpf_map_lookup_elem(sk_stg_map)"))
  goto done;
  

  }

err = pthread_create(&srv_thread, NULL, server, (void *)(long)lfd);

• if (CHECK(err != 0, "pthread_create", "err:%d errno:%d\n", err, errno))

• if (!ASSERT_OK(err, "pthread_create")) goto done;

/* recv total_bytes */ @@ -156,13 +155,12 @@ static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map) bytes += nr_recv; }

• CHECK(bytes != total_bytes, "recv", "%zd != %u nr_recv:%zd errno:%d\n",

•      bytes, total_bytes, nr_recv, errno);
  

• ASSERT_EQ(bytes, total_bytes, "recv");

WRITE_ONCE(stop, 1);

• pthread_join(srv_thread, &thread_ret);
• CHECK(IS_ERR(thread_ret), "pthread_join", "thread_ret:%ld",

•      PTR_ERR(thread_ret));
  

• err = pthread_join(srv_thread, &thread_ret);
• ASSERT_OK(err, "pthread_join");

The above is not equivalent to the original code. The original didn't check pthread_join() return as it is very very unlikely to fail. And check 'thread_ret' is still needed.

• done: close(lfd); close(fd);

@@ -174,7 +172,7 @@ static void test_cubic(void) struct bpf_link *link; cubic_skel = bpf_cubic__open_and_load();

• if (CHECK(!cubic_skel, "bpf_cubic__open_and_load", "failed\n"))

• if (!ASSERT_OK_PTR(cubic_skel, "bpf_cubic__open_and_load")) return;

[...]