On 3/22/24 9:48 AM, Andrii Nakryiko wrote:
On Fri, Mar 22, 2024 at 9:41 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:On 3/22/24 9:13 AM, Andrii Nakryiko wrote:On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:I am going to modify ksyms test later so take this opportunity to replace old CHECK macros with new ASSERT macros. No functionality change. Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx> --- .../testing/selftests/bpf/prog_tests/ksyms.c | 30 ++++++------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c index b295969b263b..6a86d1f07800 100644 --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c @@ -5,8 +5,6 @@ #include "test_ksyms.skel.h" #include <sys/stat.h> -static int duration; - void test_ksyms(void) { const char *btf_path = "/sys/kernel/btf/vmlinux"; @@ -18,43 +16,33 @@ void test_ksyms(void) int err; err = kallsyms_find("bpf_link_fops", &link_fops_addr); - if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno)) - return; - if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n")) + if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "bpf_link_fops")) return;it's best to keep each individual equality test as ASSERT_EQ(), that way we get more specific and detailed error (with expected vs actual value), which here we'll just get "wanted true but got false", and then we'd have to debug some more which of the conditions it is. So please keep the original granularity of checks everywhere.Do you mean if (ASSERT_EQ(err, -EINVAL, "...")) return; if (ASSERT_EQ(err, -ENOENT, "...")) return; This does not really work, for example if err = 0, it will declare test failure.if (!ASSERT_NEQ(err, -EINVAL, "...")) return; if (!ASSERT_NEQ(err, -NOENT, "...")) return; It's mirroring original logic. We had two independent checks there? In this case the condition is "not equal" (ASSERT_NEQ).
I actually tried this as well when I am experimenting ASSERT_EQ and somehow I concluded that !ASSERT_NEQ not working either. Now, I actully tried with adding err = 0, -EINVAL and -ENOENT and verified it indeed works. I don't know how I concluded it was not working before... Sad.
err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr); - if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno)) - return; - if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n")) + if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__per_cpu_start")) return; - if (CHECK(stat(btf_path, &st), "stat_btf", "err %d\n", errno)) + if (!ASSERT_OK(stat(btf_path, &st), "stat_btf")) return; btf_size = st.st_size; skel = test_ksyms__open_and_load(); - if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n")) + if (!ASSERT_OK_PTR(skel, "test_ksyms__open_and_load")) return; err = test_ksyms__attach(skel); - if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) + if (!ASSERT_OK(err, "test_ksyms__attach")) goto cleanup; /* trigger tracepoint */ usleep(1); data = skel->data; - CHECK(data->out__bpf_link_fops != link_fops_addr, "bpf_link_fops", - "got 0x%llx, exp 0x%llx\n", - data->out__bpf_link_fops, link_fops_addr); - CHECK(data->out__bpf_link_fops1 != 0, "bpf_link_fops1", - "got %llu, exp %llu\n", data->out__bpf_link_fops1, (__u64)0); - CHECK(data->out__btf_size != btf_size, "btf_size", - "got %llu, exp %llu\n", data->out__btf_size, btf_size); - CHECK(data->out__per_cpu_start != per_cpu_start_addr, "__per_cpu_start", - "got %llu, exp %llu\n", data->out__per_cpu_start, - per_cpu_start_addr); + ASSERT_EQ(data->out__bpf_link_fops, link_fops_addr, "bpf_link_fops"); + ASSERT_EQ(data->out__bpf_link_fops1, 0, "bpf_link_fops1"); + ASSERT_EQ(data->out__btf_size, btf_size, "btf_size"); + ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start"); cleanup: test_ksyms__destroy(skel); -- 2.43.0
