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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/20/23 12:15 PM, Yuran Pereira wrote:
Hello Yonghong,
On Mon, Nov 20, 2023 at 07:22:59AM -0800, Yonghong Song wrote:
-		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(..).

Sure, that makes sense. I'll change it in v3.
-	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.

Yes that is true, but the v1 [1] broke the tests because the
ASSERT_OK_PTR(thread_ret, "pthread_join") kept failing, even
though all the asserts within the `server()` function itself
passed.

Also, isn't asserting `thread_ret` technically checking the
`server()` function instead of `pthread_join`? So should we
have two asserts here? One for `server` and one for `pthread_join`
or is it not necessary?
i.e:
```
ASSERT_OK_PTR(thread_ret, "server");
ASSERT_OK(err, "pthread_join");
```

As I mentioned, checking return value of pthread_join()
is not critical as in general pthread_join() not fail.
The test is not to test pthread_join() and if pthread_join()
fails it would be an even bigger problem affecting many other
tests.


Upon taking a second look, I now think that the reason why
`ASSERT_OK_PTR(thread_ret, "pthread_join");` failed in v1 might
have been because it calls `libbpf_get_error` which returns
`-errno` when the pointer is `NULL`.

Since `server`'s return value is not a bpf address, which
`ASSERT_OK_PTR` expects it to be, do you that think we should
explicitly set `errno = 0` prior to returning NULL on server?
That way that assert would pass even when the pointer is NULL
(which is the case when `server` returns successfuly).

Let us just do

  ASSERT_OK(IS_ERR(thread_ret), "thread_ret")



[1] - https://lore.kernel.org/lkml/GV1PR10MB6563A0BE91080E6E8EC2651DE8B0A@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

As always, thank you for your feedback.

Yuran Pereira





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux