* [PATCH bpf-next 0/2] rely on ASSERT marcos in xdp_bpf2bpf.c/xdp_adjust_tail.c @ 2022-01-19 16:58 Lorenzo Bianconi 2022-01-19 16:58 ` [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c Lorenzo Bianconi 2022-01-19 16:58 ` [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c Lorenzo Bianconi 0 siblings, 2 replies; 7+ messages in thread From: Lorenzo Bianconi @ 2022-01-19 16:58 UTC (permalink / raw) To: bpf; +Cc: ast, andrii.nakryiko Rely on ASSERT* macros and get rid of deprecated CHECK ones in xdp_bpf2bpf and xdp_adjust_tail bpf selftests. This is a preliminary series for XDP multi-frags support. Lorenzo Bianconi (2): bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c .../bpf/prog_tests/xdp_adjust_tail.c | 62 +++++++------------ .../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 43 +++++-------- 2 files changed, 40 insertions(+), 65 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c 2022-01-19 16:58 [PATCH bpf-next 0/2] rely on ASSERT marcos in xdp_bpf2bpf.c/xdp_adjust_tail.c Lorenzo Bianconi @ 2022-01-19 16:58 ` Lorenzo Bianconi 2022-01-19 18:52 ` Andrii Nakryiko 2022-01-19 16:58 ` [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c Lorenzo Bianconi 1 sibling, 1 reply; 7+ messages in thread From: Lorenzo Bianconi @ 2022-01-19 16:58 UTC (permalink / raw) To: bpf; +Cc: ast, andrii.nakryiko Rely on ASSERT* macros and get rid of deprecated CHECK ones in xdp_adjust_tail bpf selftest. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- .../bpf/prog_tests/xdp_adjust_tail.c | 62 +++++++------------ 1 file changed, 23 insertions(+), 39 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c index 3f5a17c38be5..dffa21b35503 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c @@ -11,22 +11,19 @@ static void test_xdp_adjust_tail_shrink(void) char buf[128]; err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd); - if (CHECK_FAIL(err)) + if (ASSERT_OK(err, "test_xdp_adjust_tail_shrink")) return; err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4), buf, &size, &retval, &duration); - CHECK(err || retval != XDP_DROP, - "ipv4", "err %d errno %d retval %d size %d\n", - err, errno, retval, size); + ASSERT_OK(err || retval != XDP_DROP, "ipv4"); expect_sz = sizeof(pkt_v6) - 20; /* Test shrink with 20 bytes */ err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6), buf, &size, &retval, &duration); - CHECK(err || retval != XDP_TX || size != expect_sz, - "ipv6", "err %d errno %d retval %d size %d expect-size %d\n", - err, errno, retval, size, expect_sz); + ASSERT_OK(err || retval != XDP_TX || size != expect_sz, "ipv6"); + bpf_object__close(obj); } @@ -39,21 +36,17 @@ static void test_xdp_adjust_tail_grow(void) int err, prog_fd; err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd); - if (CHECK_FAIL(err)) + if (ASSERT_OK(err, "test_xdp_adjust_tail_grow")) return; err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4), buf, &size, &retval, &duration); - CHECK(err || retval != XDP_DROP, - "ipv4", "err %d errno %d retval %d size %d\n", - err, errno, retval, size); + ASSERT_OK(err || retval != XDP_DROP, "ipv4"); expect_sz = sizeof(pkt_v6) + 40; /* Test grow with 40 bytes */ err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6) /* 74 */, buf, &size, &retval, &duration); - CHECK(err || retval != XDP_TX || size != expect_sz, - "ipv6", "err %d errno %d retval %d size %d expect-size %d\n", - err, errno, retval, size, expect_sz); + ASSERT_OK(err || retval != XDP_TX || size != expect_sz, "ipv6"); bpf_object__close(obj); } @@ -76,7 +69,7 @@ static void test_xdp_adjust_tail_grow2(void) }; err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &tattr.prog_fd); - if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno)) + if (ASSERT_OK(err, "test_xdp_adjust_tail_grow")) return; /* Test case-64 */ @@ -86,21 +79,17 @@ static void test_xdp_adjust_tail_grow2(void) /* Kernel side alloc packet memory area that is zero init */ err = bpf_prog_test_run_xattr(&tattr); - CHECK_ATTR(errno != ENOSPC /* Due limit copy_size in bpf_test_finish */ - || tattr.retval != XDP_TX - || tattr.data_size_out != 192, /* Expected grow size */ - "case-64", - "err %d errno %d retval %d size %d\n", - err, errno, tattr.retval, tattr.data_size_out); + ASSERT_OK(errno != ENOSPC /* Due limit copy_size in bpf_test_finish */ + || tattr.retval != XDP_TX + || tattr.data_size_out != 192, /* Expected grow size */ + "case-64"); /* Extra checks for data contents */ - CHECK_ATTR(tattr.data_size_out != 192 - || buf[0] != 1 || buf[63] != 1 /* 0-63 memset to 1 */ - || buf[64] != 0 || buf[127] != 0 /* 64-127 memset to 0 */ - || buf[128] != 1 || buf[191] != 1, /*128-191 memset to 1 */ - "case-64-data", - "err %d errno %d retval %d size %d\n", - err, errno, tattr.retval, tattr.data_size_out); + ASSERT_OK(tattr.data_size_out != 192 + || buf[0] != 1 || buf[63] != 1 /* 0-63 memset to 1 */ + || buf[64] != 0 || buf[127] != 0 /* 64-127 memset to 0 */ + || buf[128] != 1 || buf[191] != 1, /*128-191 memset to 1 */ + "case-64-data"); /* Test case-128 */ memset(buf, 2, sizeof(buf)); @@ -109,23 +98,18 @@ static void test_xdp_adjust_tail_grow2(void) err = bpf_prog_test_run_xattr(&tattr); max_grow = 4096 - XDP_PACKET_HEADROOM - tailroom; /* 3520 */ - CHECK_ATTR(err - || tattr.retval != XDP_TX - || tattr.data_size_out != max_grow,/* Expect max grow size */ - "case-128", - "err %d errno %d retval %d size %d expect-size %d\n", - err, errno, tattr.retval, tattr.data_size_out, max_grow); + ASSERT_OK(err || tattr.retval != XDP_TX + || tattr.data_size_out != max_grow,/* Expect max grow size */ + "case-128"); /* Extra checks for data content: Count grow size, will contain zeros */ for (i = 0, cnt = 0; i < sizeof(buf); i++) { if (buf[i] == 0) cnt++; } - CHECK_ATTR((cnt != (max_grow - tattr.data_size_in)) /* Grow increase */ - || tattr.data_size_out != max_grow, /* Total grow size */ - "case-128-data", - "err %d errno %d retval %d size %d grow-size %d\n", - err, errno, tattr.retval, tattr.data_size_out, cnt); + ASSERT_OK((cnt != (max_grow - tattr.data_size_in)) /* Grow increase */ + || tattr.data_size_out != max_grow, /* Total grow size */ + "case-128-data"); bpf_object__close(obj); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c 2022-01-19 16:58 ` [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c Lorenzo Bianconi @ 2022-01-19 18:52 ` Andrii Nakryiko 2022-01-19 21:54 ` Lorenzo Bianconi 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2022-01-19 18:52 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: bpf, Alexei Starovoitov On Wed, Jan 19, 2022 at 8:58 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > Rely on ASSERT* macros and get rid of deprecated CHECK ones in > xdp_adjust_tail bpf selftest. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../bpf/prog_tests/xdp_adjust_tail.c | 62 +++++++------------ > 1 file changed, 23 insertions(+), 39 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c > index 3f5a17c38be5..dffa21b35503 100644 > --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c > @@ -11,22 +11,19 @@ static void test_xdp_adjust_tail_shrink(void) > char buf[128]; > > err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd); > - if (CHECK_FAIL(err)) > + if (ASSERT_OK(err, "test_xdp_adjust_tail_shrink")) > return; > > err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4), > buf, &size, &retval, &duration); > > - CHECK(err || retval != XDP_DROP, > - "ipv4", "err %d errno %d retval %d size %d\n", > - err, errno, retval, size); > + ASSERT_OK(err || retval != XDP_DROP, "ipv4"); it's better to do such checks as two ASSERTS: ASSERT_OK(err) and ASSERT_EQ(retval, XDP_DROP). It will give much more meaningful error messages and I think is easier to follow. > > expect_sz = sizeof(pkt_v6) - 20; /* Test shrink with 20 bytes */ > err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6), > buf, &size, &retval, &duration); > - CHECK(err || retval != XDP_TX || size != expect_sz, > - "ipv6", "err %d errno %d retval %d size %d expect-size %d\n", > - err, errno, retval, size, expect_sz); > + ASSERT_OK(err || retval != XDP_TX || size != expect_sz, "ipv6"); same as above, old CHECK printed all those values so it was ok-ish to combine checks. With ASSERT_XXX() let's do each error condition check separately. Same for all the rest below. > + > bpf_object__close(obj); > } > [...] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c 2022-01-19 18:52 ` Andrii Nakryiko @ 2022-01-19 21:54 ` Lorenzo Bianconi 0 siblings, 0 replies; 7+ messages in thread From: Lorenzo Bianconi @ 2022-01-19 21:54 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov [-- Attachment #1: Type: text/plain, Size: 2448 bytes --] > On Wed, Jan 19, 2022 at 8:58 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > Rely on ASSERT* macros and get rid of deprecated CHECK ones in > > xdp_adjust_tail bpf selftest. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > .../bpf/prog_tests/xdp_adjust_tail.c | 62 +++++++------------ > > 1 file changed, 23 insertions(+), 39 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c > > index 3f5a17c38be5..dffa21b35503 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c > > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c > > @@ -11,22 +11,19 @@ static void test_xdp_adjust_tail_shrink(void) > > char buf[128]; > > > > err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd); > > - if (CHECK_FAIL(err)) > > + if (ASSERT_OK(err, "test_xdp_adjust_tail_shrink")) > > return; > > > > err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4), > > buf, &size, &retval, &duration); > > > > - CHECK(err || retval != XDP_DROP, > > - "ipv4", "err %d errno %d retval %d size %d\n", > > - err, errno, retval, size); > > + ASSERT_OK(err || retval != XDP_DROP, "ipv4"); > > it's better to do such checks as two ASSERTS: ASSERT_OK(err) and > ASSERT_EQ(retval, XDP_DROP). It will give much more meaningful error > messages and I think is easier to follow. ack, I will fix it in v2. > > > > > expect_sz = sizeof(pkt_v6) - 20; /* Test shrink with 20 bytes */ > > err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6), > > buf, &size, &retval, &duration); > > - CHECK(err || retval != XDP_TX || size != expect_sz, > > - "ipv6", "err %d errno %d retval %d size %d expect-size %d\n", > > - err, errno, retval, size, expect_sz); > > + ASSERT_OK(err || retval != XDP_TX || size != expect_sz, "ipv6"); > > same as above, old CHECK printed all those values so it was ok-ish to > combine checks. With ASSERT_XXX() let's do each error condition check > separately. Same for all the rest below. ack, I will fix it in v2. Regards, Lorenzo > > > + > > bpf_object__close(obj); > > } > > > > [...] [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c 2022-01-19 16:58 [PATCH bpf-next 0/2] rely on ASSERT marcos in xdp_bpf2bpf.c/xdp_adjust_tail.c Lorenzo Bianconi 2022-01-19 16:58 ` [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c Lorenzo Bianconi @ 2022-01-19 16:58 ` Lorenzo Bianconi 2022-01-19 18:54 ` Andrii Nakryiko 1 sibling, 1 reply; 7+ messages in thread From: Lorenzo Bianconi @ 2022-01-19 16:58 UTC (permalink / raw) To: bpf; +Cc: ast, andrii.nakryiko Rely on ASSERT* macros and get rid of deprecated CHECK ones in xdp_bpf2bpf bpf selftest. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- .../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 43 ++++++++----------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c index c98a897ad692..951ce1fc535d 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c @@ -12,25 +12,21 @@ struct meta { static void on_sample(void *ctx, int cpu, void *data, __u32 size) { - int duration = 0; struct meta *meta = (struct meta *)data; struct ipv4_packet *trace_pkt_v4 = data + sizeof(*meta); - if (CHECK(size < sizeof(pkt_v4) + sizeof(*meta), - "check_size", "size %u < %zu\n", - size, sizeof(pkt_v4) + sizeof(*meta))) + if (!ASSERT_GE(size, sizeof(pkt_v4) + sizeof(*meta), "check_size")) return; - if (CHECK(meta->ifindex != if_nametoindex("lo"), "check_meta_ifindex", - "meta->ifindex = %d\n", meta->ifindex)) + if (!ASSERT_EQ(meta->ifindex, if_nametoindex("lo"), + "check_meta_ifindex")) return; - if (CHECK(meta->pkt_len != sizeof(pkt_v4), "check_meta_pkt_len", - "meta->pkt_len = %zd\n", sizeof(pkt_v4))) + if (!ASSERT_EQ(meta->pkt_len, sizeof(pkt_v4), "check_meta_pkt_len")) return; - if (CHECK(memcmp(trace_pkt_v4, &pkt_v4, sizeof(pkt_v4)), - "check_packet_content", "content not the same\n")) + if (!ASSERT_EQ(memcmp(trace_pkt_v4, &pkt_v4, sizeof(pkt_v4)), + 0, "check_packet_content")) return; *(bool *)ctx = true; @@ -52,7 +48,7 @@ void test_xdp_bpf2bpf(void) /* Load XDP program to introspect */ pkt_skel = test_xdp__open_and_load(); - if (CHECK(!pkt_skel, "pkt_skel_load", "test_xdp skeleton failed\n")) + if (!ASSERT_OK_PTR(pkt_skel, "test_xdp__open_and_load")) return; pkt_fd = bpf_program__fd(pkt_skel->progs._xdp_tx_iptunnel); @@ -62,7 +58,7 @@ void test_xdp_bpf2bpf(void) /* Load trace program */ ftrace_skel = test_xdp_bpf2bpf__open(); - if (CHECK(!ftrace_skel, "__open", "ftrace skeleton failed\n")) + if (!ASSERT_OK_PTR(ftrace_skel, "test_xdp_bpf2bpf__open")) goto out; /* Demonstrate the bpf_program__set_attach_target() API rather than @@ -77,11 +73,11 @@ void test_xdp_bpf2bpf(void) bpf_program__set_attach_target(prog, pkt_fd, "_xdp_tx_iptunnel"); err = test_xdp_bpf2bpf__load(ftrace_skel); - if (CHECK(err, "__load", "ftrace skeleton failed\n")) + if (!ASSERT_OK(err, "test_xdp_bpf2bpf__load")) goto out; err = test_xdp_bpf2bpf__attach(ftrace_skel); - if (CHECK(err, "ftrace_attach", "ftrace attach failed: %d\n", err)) + if (!ASSERT_OK(err, "test_xdp_bpf2bpf__attach")) goto out; /* Set up perf buffer */ @@ -94,30 +90,25 @@ void test_xdp_bpf2bpf(void) err = bpf_prog_test_run(pkt_fd, 1, &pkt_v4, sizeof(pkt_v4), buf, &size, &retval, &duration); memcpy(&iph, buf + sizeof(struct ethhdr), sizeof(iph)); - if (CHECK(err || retval != XDP_TX || size != 74 || - iph.protocol != IPPROTO_IPIP, "ipv4", - "err %d errno %d retval %d size %d\n", - err, errno, retval, size)) + if (!ASSERT_OK(err || retval != XDP_TX || size != 74 || + iph.protocol != IPPROTO_IPIP, "ipv4")) goto out; /* Make sure bpf_xdp_output() was triggered and it sent the expected * data to the perf ring buffer. */ err = perf_buffer__poll(pb, 100); - if (CHECK(err < 0, "perf_buffer__poll", "err %d\n", err)) + if (!ASSERT_GE(err, 0, "perf_buffer__poll")) goto out; - CHECK_FAIL(!passed); + ASSERT_TRUE(passed, "test passed"); /* Verify test results */ - if (CHECK(ftrace_skel->bss->test_result_fentry != if_nametoindex("lo"), - "result", "fentry failed err %llu\n", - ftrace_skel->bss->test_result_fentry)) + if (!ASSERT_EQ(ftrace_skel->bss->test_result_fentry, if_nametoindex("lo"), + "fentry result")) goto out; - CHECK(ftrace_skel->bss->test_result_fexit != XDP_TX, "result", - "fexit failed err %llu\n", ftrace_skel->bss->test_result_fexit); - + ASSERT_EQ(ftrace_skel->bss->test_result_fexit, XDP_TX, "fexit result"); out: if (pb) perf_buffer__free(pb); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c 2022-01-19 16:58 ` [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c Lorenzo Bianconi @ 2022-01-19 18:54 ` Andrii Nakryiko 2022-01-19 21:53 ` Lorenzo Bianconi 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2022-01-19 18:54 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: bpf, Alexei Starovoitov On Wed, Jan 19, 2022 at 8:58 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > Rely on ASSERT* macros and get rid of deprecated CHECK ones in > xdp_bpf2bpf bpf selftest. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 43 ++++++++----------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c > index c98a897ad692..951ce1fc535d 100644 > --- a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c > @@ -12,25 +12,21 @@ struct meta { > > static void on_sample(void *ctx, int cpu, void *data, __u32 size) > { > - int duration = 0; > struct meta *meta = (struct meta *)data; > struct ipv4_packet *trace_pkt_v4 = data + sizeof(*meta); > > - if (CHECK(size < sizeof(pkt_v4) + sizeof(*meta), > - "check_size", "size %u < %zu\n", > - size, sizeof(pkt_v4) + sizeof(*meta))) > + if (!ASSERT_GE(size, sizeof(pkt_v4) + sizeof(*meta), "check_size")) > return; > > - if (CHECK(meta->ifindex != if_nametoindex("lo"), "check_meta_ifindex", > - "meta->ifindex = %d\n", meta->ifindex)) > + if (!ASSERT_EQ(meta->ifindex, if_nametoindex("lo"), > + "check_meta_ifindex")) > return; > > - if (CHECK(meta->pkt_len != sizeof(pkt_v4), "check_meta_pkt_len", > - "meta->pkt_len = %zd\n", sizeof(pkt_v4))) > + if (!ASSERT_EQ(meta->pkt_len, sizeof(pkt_v4), "check_meta_pkt_len")) > return; > > - if (CHECK(memcmp(trace_pkt_v4, &pkt_v4, sizeof(pkt_v4)), > - "check_packet_content", "content not the same\n")) > + if (!ASSERT_EQ(memcmp(trace_pkt_v4, &pkt_v4, sizeof(pkt_v4)), > + 0, "check_packet_content")) > return; > we can simplify and make it easier to follow by not doing early returns. Just a sequence of ASSERTs would be compact and nice. > *(bool *)ctx = true; > @@ -52,7 +48,7 @@ void test_xdp_bpf2bpf(void) > > /* Load XDP program to introspect */ > pkt_skel = test_xdp__open_and_load(); > - if (CHECK(!pkt_skel, "pkt_skel_load", "test_xdp skeleton failed\n")) > + if (!ASSERT_OK_PTR(pkt_skel, "test_xdp__open_and_load")) > return; > > pkt_fd = bpf_program__fd(pkt_skel->progs._xdp_tx_iptunnel); > @@ -62,7 +58,7 @@ void test_xdp_bpf2bpf(void) > > /* Load trace program */ > ftrace_skel = test_xdp_bpf2bpf__open(); > - if (CHECK(!ftrace_skel, "__open", "ftrace skeleton failed\n")) > + if (!ASSERT_OK_PTR(ftrace_skel, "test_xdp_bpf2bpf__open")) > goto out; > > /* Demonstrate the bpf_program__set_attach_target() API rather than > @@ -77,11 +73,11 @@ void test_xdp_bpf2bpf(void) > bpf_program__set_attach_target(prog, pkt_fd, "_xdp_tx_iptunnel"); > > err = test_xdp_bpf2bpf__load(ftrace_skel); > - if (CHECK(err, "__load", "ftrace skeleton failed\n")) > + if (!ASSERT_OK(err, "test_xdp_bpf2bpf__load")) > goto out; > > err = test_xdp_bpf2bpf__attach(ftrace_skel); > - if (CHECK(err, "ftrace_attach", "ftrace attach failed: %d\n", err)) > + if (!ASSERT_OK(err, "test_xdp_bpf2bpf__attach")) > goto out; > > /* Set up perf buffer */ > @@ -94,30 +90,25 @@ void test_xdp_bpf2bpf(void) > err = bpf_prog_test_run(pkt_fd, 1, &pkt_v4, sizeof(pkt_v4), > buf, &size, &retval, &duration); > memcpy(&iph, buf + sizeof(struct ethhdr), sizeof(iph)); > - if (CHECK(err || retval != XDP_TX || size != 74 || > - iph.protocol != IPPROTO_IPIP, "ipv4", > - "err %d errno %d retval %d size %d\n", > - err, errno, retval, size)) > + if (!ASSERT_OK(err || retval != XDP_TX || size != 74 || > + iph.protocol != IPPROTO_IPIP, "ipv4")) > goto out; > > /* Make sure bpf_xdp_output() was triggered and it sent the expected > * data to the perf ring buffer. > */ > err = perf_buffer__poll(pb, 100); > - if (CHECK(err < 0, "perf_buffer__poll", "err %d\n", err)) > + if (!ASSERT_GE(err, 0, "perf_buffer__poll")) > goto out; > > - CHECK_FAIL(!passed); > + ASSERT_TRUE(passed, "test passed"); > > /* Verify test results */ > - if (CHECK(ftrace_skel->bss->test_result_fentry != if_nametoindex("lo"), > - "result", "fentry failed err %llu\n", > - ftrace_skel->bss->test_result_fentry)) > + if (!ASSERT_EQ(ftrace_skel->bss->test_result_fentry, if_nametoindex("lo"), > + "fentry result")) > goto out; > > - CHECK(ftrace_skel->bss->test_result_fexit != XDP_TX, "result", > - "fexit failed err %llu\n", ftrace_skel->bss->test_result_fexit); > - > + ASSERT_EQ(ftrace_skel->bss->test_result_fexit, XDP_TX, "fexit result"); > out: > if (pb) while at it, please drop this if. perf_buffer__free() handles NULLs and error pointers. > perf_buffer__free(pb); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c 2022-01-19 18:54 ` Andrii Nakryiko @ 2022-01-19 21:53 ` Lorenzo Bianconi 0 siblings, 0 replies; 7+ messages in thread From: Lorenzo Bianconi @ 2022-01-19 21:53 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov [-- Attachment #1: Type: text/plain, Size: 5857 bytes --] > On Wed, Jan 19, 2022 at 8:58 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > Rely on ASSERT* macros and get rid of deprecated CHECK ones in > > xdp_bpf2bpf bpf selftest. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > .../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 43 ++++++++----------- > > 1 file changed, 17 insertions(+), 26 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c > > index c98a897ad692..951ce1fc535d 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c > > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c > > @@ -12,25 +12,21 @@ struct meta { > > > > static void on_sample(void *ctx, int cpu, void *data, __u32 size) > > { > > - int duration = 0; > > struct meta *meta = (struct meta *)data; > > struct ipv4_packet *trace_pkt_v4 = data + sizeof(*meta); > > > > - if (CHECK(size < sizeof(pkt_v4) + sizeof(*meta), > > - "check_size", "size %u < %zu\n", > > - size, sizeof(pkt_v4) + sizeof(*meta))) > > + if (!ASSERT_GE(size, sizeof(pkt_v4) + sizeof(*meta), "check_size")) > > return; > > > > - if (CHECK(meta->ifindex != if_nametoindex("lo"), "check_meta_ifindex", > > - "meta->ifindex = %d\n", meta->ifindex)) > > + if (!ASSERT_EQ(meta->ifindex, if_nametoindex("lo"), > > + "check_meta_ifindex")) > > return; > > > > - if (CHECK(meta->pkt_len != sizeof(pkt_v4), "check_meta_pkt_len", > > - "meta->pkt_len = %zd\n", sizeof(pkt_v4))) > > + if (!ASSERT_EQ(meta->pkt_len, sizeof(pkt_v4), "check_meta_pkt_len")) > > return; > > > > - if (CHECK(memcmp(trace_pkt_v4, &pkt_v4, sizeof(pkt_v4)), > > - "check_packet_content", "content not the same\n")) > > + if (!ASSERT_EQ(memcmp(trace_pkt_v4, &pkt_v4, sizeof(pkt_v4)), > > + 0, "check_packet_content")) > > return; > > > > we can simplify and make it easier to follow by not doing early > returns. Just a sequence of ASSERTs would be compact and nice. ack, fine. I will fix it in v2. > > > *(bool *)ctx = true; > > @@ -52,7 +48,7 @@ void test_xdp_bpf2bpf(void) > > > > /* Load XDP program to introspect */ > > pkt_skel = test_xdp__open_and_load(); > > - if (CHECK(!pkt_skel, "pkt_skel_load", "test_xdp skeleton failed\n")) > > + if (!ASSERT_OK_PTR(pkt_skel, "test_xdp__open_and_load")) > > return; > > > > pkt_fd = bpf_program__fd(pkt_skel->progs._xdp_tx_iptunnel); > > @@ -62,7 +58,7 @@ void test_xdp_bpf2bpf(void) > > > > /* Load trace program */ > > ftrace_skel = test_xdp_bpf2bpf__open(); > > - if (CHECK(!ftrace_skel, "__open", "ftrace skeleton failed\n")) > > + if (!ASSERT_OK_PTR(ftrace_skel, "test_xdp_bpf2bpf__open")) > > goto out; > > > > /* Demonstrate the bpf_program__set_attach_target() API rather than > > @@ -77,11 +73,11 @@ void test_xdp_bpf2bpf(void) > > bpf_program__set_attach_target(prog, pkt_fd, "_xdp_tx_iptunnel"); > > > > err = test_xdp_bpf2bpf__load(ftrace_skel); > > - if (CHECK(err, "__load", "ftrace skeleton failed\n")) > > + if (!ASSERT_OK(err, "test_xdp_bpf2bpf__load")) > > goto out; > > > > err = test_xdp_bpf2bpf__attach(ftrace_skel); > > - if (CHECK(err, "ftrace_attach", "ftrace attach failed: %d\n", err)) > > + if (!ASSERT_OK(err, "test_xdp_bpf2bpf__attach")) > > goto out; > > > > /* Set up perf buffer */ > > @@ -94,30 +90,25 @@ void test_xdp_bpf2bpf(void) > > err = bpf_prog_test_run(pkt_fd, 1, &pkt_v4, sizeof(pkt_v4), > > buf, &size, &retval, &duration); > > memcpy(&iph, buf + sizeof(struct ethhdr), sizeof(iph)); > > - if (CHECK(err || retval != XDP_TX || size != 74 || > > - iph.protocol != IPPROTO_IPIP, "ipv4", > > - "err %d errno %d retval %d size %d\n", > > - err, errno, retval, size)) > > + if (!ASSERT_OK(err || retval != XDP_TX || size != 74 || > > + iph.protocol != IPPROTO_IPIP, "ipv4")) > > goto out; > > > > /* Make sure bpf_xdp_output() was triggered and it sent the expected > > * data to the perf ring buffer. > > */ > > err = perf_buffer__poll(pb, 100); > > - if (CHECK(err < 0, "perf_buffer__poll", "err %d\n", err)) > > + if (!ASSERT_GE(err, 0, "perf_buffer__poll")) > > goto out; > > > > - CHECK_FAIL(!passed); > > + ASSERT_TRUE(passed, "test passed"); > > > > /* Verify test results */ > > - if (CHECK(ftrace_skel->bss->test_result_fentry != if_nametoindex("lo"), > > - "result", "fentry failed err %llu\n", > > - ftrace_skel->bss->test_result_fentry)) > > + if (!ASSERT_EQ(ftrace_skel->bss->test_result_fentry, if_nametoindex("lo"), > > + "fentry result")) > > goto out; > > > > - CHECK(ftrace_skel->bss->test_result_fexit != XDP_TX, "result", > > - "fexit failed err %llu\n", ftrace_skel->bss->test_result_fexit); > > - > > + ASSERT_EQ(ftrace_skel->bss->test_result_fexit, XDP_TX, "fexit result"); > > out: > > if (pb) > > while at it, please drop this if. perf_buffer__free() handles NULLs > and error pointers. ack, fine. I will fix it in v2. Regards, Lorenzo > > > perf_buffer__free(pb); > > -- > > 2.34.1 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-19 21:55 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-19 16:58 [PATCH bpf-next 0/2] rely on ASSERT marcos in xdp_bpf2bpf.c/xdp_adjust_tail.c Lorenzo Bianconi 2022-01-19 16:58 ` [PATCH bpf-next 1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c Lorenzo Bianconi 2022-01-19 18:52 ` Andrii Nakryiko 2022-01-19 21:54 ` Lorenzo Bianconi 2022-01-19 16:58 ` [PATCH bpf-next 2/2] bpf: selftests: get rid of CHECK macro in xdp_bpf2bpf.c Lorenzo Bianconi 2022-01-19 18:54 ` Andrii Nakryiko 2022-01-19 21:53 ` Lorenzo Bianconi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).