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"); ``` 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). [1] - https://lore.kernel.org/lkml/gv1pr10mb6563a0be91080e6e8ec2651de8...@gv1pr10mb6563.eurprd10.prod.outlook.com/ As always, thank you for your feedback. Yuran Pereira
