> On Wed, Jan 19, 2022 at 8:58 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
> >
> > Rely on ASSERT* macros and get rid of deprecated CHECK ones in
> > xdp_bpf2bpf bpf selftest.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > ---
> > .../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:
signature.asc
Description: PGP signature
