Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 20 Nov 2023 11:00:17 -0800 | Subject | Re: [PATCH bpf-next v2 1/4] selftests/bpf: Replaces the usage of CHECK calls for ASSERTs in bpf_tcp_ca | From | Yonghong Song <> |
| |
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@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM/ > > As always, thank you for your feedback. > > Yuran Pereira >
| ![\](/images/icornerr.gif) |