* [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
* [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 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 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.