diff mbox series

[bpf-next,1/2] bpf: selftests: get rid of CHECK macro in xdp_adjust_tail.c

Message ID bd3608faf2e9162cc93d54ee93d2d6737750bb30.1642611050.git.lorenzo@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series rely on ASSERT marcos in xdp_bpf2bpf.c/xdp_adjust_tail.c | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 13 maintainers not CCed: andrii@kernel.org linux-kselftest@vger.kernel.org hawk@kernel.org kuba@kernel.org kpsingh@kernel.org daniel@iogearbox.net john.fastabend@gmail.com kafai@fb.com shuah@kernel.org songliubraving@fb.com yhs@fb.com netdev@vger.kernel.org davem@davemloft.net
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning CHECK: Logical continuations should be on the previous line
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next success VM_Test
bpf/vmtest-bpf-next-PR success PR summary

Commit Message

Lorenzo Bianconi Jan. 19, 2022, 4:58 p.m. UTC 
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(-)

Comments

Andrii Nakryiko Jan. 19, 2022, 6:52 p.m. UTC | #1 
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);
>  }
>

[...]
Lorenzo Bianconi Jan. 19, 2022, 9:54 p.m. UTC | #2 
> 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);
> >  }
> >
> 
> [...]
diff mbox series

Patch

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);
 }