| Message ID | 20210423233058.3386115-1-andrii@kernel.org |
|---|---|
| Headers | show |
| Series | CO-RE relocation selftests fixes | expand |
On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: > > Add ASSERT_TRUE/ASSERT_FALSE for conditions calculated with custom logic to > true/false. Also add remaining arithmetical assertions: > - ASSERT_LE -- less than or equal; > - ASSERT_GT -- greater than; > - ASSERT_GE -- greater than or equal. > This should cover most scenarios where people fall back to error-prone > CHECK()s. > > Also extend ASSERT_ERR() to print out errno, in addition to direct error. > > Also convert few CHECK() instances to ensure new ASSERT_xxx() variants work as > expected. Subsequent patch will also use ASSERT_TRUE/ASSERT_FALSE more > extensively. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > .../selftests/bpf/prog_tests/btf_dump.c | 2 +- > .../selftests/bpf/prog_tests/btf_endian.c | 4 +- > .../selftests/bpf/prog_tests/cgroup_link.c | 2 +- > .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- > .../selftests/bpf/prog_tests/resolve_btfids.c | 7 +-- > .../selftests/bpf/prog_tests/snprintf_btf.c | 4 +- > tools/testing/selftests/bpf/test_progs.h | 50 ++++++++++++++++++- > 7 files changed, 56 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c > index c60091ee8a21..5e129dc2073c 100644 > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c > @@ -77,7 +77,7 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t) > > snprintf(out_file, sizeof(out_file), "/tmp/%s.output.XXXXXX", t->file); > fd = mkstemp(out_file); > - if (CHECK(fd < 0, "create_tmp", "failed to create file: %d\n", fd)) { > + if (!ASSERT_GE(fd, 0, "create_tmp")) { Nit: I would find ASSERT_LE easier to read here. Inverting boolean conditions is easy to get wrong. > err = fd; > goto done; > } > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_endian.c b/tools/testing/selftests/bpf/prog_tests/btf_endian.c > index 8c52d72c876e..8ab5d3e358dd 100644 > --- a/tools/testing/selftests/bpf/prog_tests/btf_endian.c > +++ b/tools/testing/selftests/bpf/prog_tests/btf_endian.c > @@ -6,8 +6,6 @@ > #include <test_progs.h> > #include <bpf/btf.h> > > -static int duration = 0; Good to see this go. Acked-by: Lorenz Bauer <lmb@cloudflare.com> -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: > > Add BTF_KIND_FLOAT support when doing CO-RE field type compatibility check. > Without this, relocations against float/double fields will fail. > > Also adjust one error message to emit instruction index instead of less > convenient instruction byte offset. > > Fixes: 22541a9eeb0d ("libbpf: Add BTF_KIND_FLOAT support") > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/libbpf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index a1cddd17af7d..ff54ffef433a 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -5141,6 +5141,7 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf, > > switch (btf_kind(local_type)) { > case BTF_KIND_PTR: > + case BTF_KIND_FLOAT: Nit: update the function comment as well? Acked-by: Lorenz Bauer <lmb@cloudflare.com> -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: > > Fix failed tests checks in core_reloc test runner, which allowed failing tests > to pass quietly. Also add extra check to make sure that expected to fail test cases with > invalid names are caught as test failure anyway, as this is not an expected > failure mode. Also fix mislabeled probed vs direct bitfield test cases. > > Fixes: 124a892d1c41 ("selftests/bpf: Test TYPE_EXISTS and TYPE_SIZE CO-RE relocations") > Reported-by: Lorenz Bauer <lmb@cloudflare.com> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > .../selftests/bpf/prog_tests/core_reloc.c | 20 +++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > index 385fd7696a2e..607710826dca 100644 > --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c > +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > @@ -217,7 +217,7 @@ static int duration = 0; > > #define BITFIELDS_CASE(name, ...) { \ > BITFIELDS_CASE_COMMON("test_core_reloc_bitfields_probed.o", \ > - "direct:", name), \ > + "probed:", name), \ > .input = STRUCT_TO_CHAR_PTR(core_reloc_##name) __VA_ARGS__, \ > .input_len = sizeof(struct core_reloc_##name), \ > .output = STRUCT_TO_CHAR_PTR(core_reloc_bitfields_output) \ > @@ -225,7 +225,7 @@ static int duration = 0; > .output_len = sizeof(struct core_reloc_bitfields_output), \ > }, { \ > BITFIELDS_CASE_COMMON("test_core_reloc_bitfields_direct.o", \ > - "probed:", name), \ > + "direct:", name), \ > .input = STRUCT_TO_CHAR_PTR(core_reloc_##name) __VA_ARGS__, \ > .input_len = sizeof(struct core_reloc_##name), \ > .output = STRUCT_TO_CHAR_PTR(core_reloc_bitfields_output) \ > @@ -546,8 +546,7 @@ static struct core_reloc_test_case test_cases[] = { > ARRAYS_ERR_CASE(arrays___err_too_small), > ARRAYS_ERR_CASE(arrays___err_too_shallow), > ARRAYS_ERR_CASE(arrays___err_non_array), > - ARRAYS_ERR_CASE(arrays___err_wrong_val_type1), > - ARRAYS_ERR_CASE(arrays___err_wrong_val_type2), > + ARRAYS_ERR_CASE(arrays___err_wrong_val_type), > ARRAYS_ERR_CASE(arrays___err_bad_zero_sz_arr), > > /* enum/ptr/int handling scenarios */ > @@ -865,13 +864,20 @@ void test_core_reloc(void) > "prog '%s' not found\n", probe_name)) > goto cleanup; > > + > + if (test_case->btf_src_file) { > + err = access(test_case->btf_src_file, R_OK); > + if (!ASSERT_OK(err, "btf_src_file")) > + goto cleanup; > + } > + > load_attr.obj = obj; > load_attr.log_level = 0; > load_attr.target_btf_path = test_case->btf_src_file; > err = bpf_object__load_xattr(&load_attr); > if (err) { > if (!test_case->fails) > - CHECK(false, "obj_load", "failed to load prog '%s': %d\n", probe_name, err); > + ASSERT_OK(err, "obj_load"); > goto cleanup; > } > > @@ -910,10 +916,8 @@ void test_core_reloc(void) > goto cleanup; > } > > - if (test_case->fails) { > - CHECK(false, "obj_load_fail", "should fail to load prog '%s'\n", probe_name); > + if (!ASSERT_FALSE(test_case->fails, "obj_load_should_fail")) Similar to my other comment, I find it difficult to tell when this triggers. Maybe it makes sense to return the status of the assertion (not the original value)? So if (assertion()) will be executed when the assertion fails? Not sure. Acked-by: Lorenz Bauer <lmb@cloudflare.com> -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Mon, Apr 26, 2021 at 1:06 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: > > > > Add ASSERT_TRUE/ASSERT_FALSE for conditions calculated with custom logic to > > true/false. Also add remaining arithmetical assertions: > > - ASSERT_LE -- less than or equal; > > - ASSERT_GT -- greater than; > > - ASSERT_GE -- greater than or equal. > > This should cover most scenarios where people fall back to error-prone > > CHECK()s. > > > > Also extend ASSERT_ERR() to print out errno, in addition to direct error. > > > > Also convert few CHECK() instances to ensure new ASSERT_xxx() variants work as > > expected. Subsequent patch will also use ASSERT_TRUE/ASSERT_FALSE more > > extensively. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > .../selftests/bpf/prog_tests/btf_dump.c | 2 +- > > .../selftests/bpf/prog_tests/btf_endian.c | 4 +- > > .../selftests/bpf/prog_tests/cgroup_link.c | 2 +- > > .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- > > .../selftests/bpf/prog_tests/resolve_btfids.c | 7 +-- > > .../selftests/bpf/prog_tests/snprintf_btf.c | 4 +- > > tools/testing/selftests/bpf/test_progs.h | 50 ++++++++++++++++++- > > 7 files changed, 56 insertions(+), 15 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c > > index c60091ee8a21..5e129dc2073c 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c > > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c > > @@ -77,7 +77,7 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t) > > > > snprintf(out_file, sizeof(out_file), "/tmp/%s.output.XXXXXX", t->file); > > fd = mkstemp(out_file); > > - if (CHECK(fd < 0, "create_tmp", "failed to create file: %d\n", fd)) { > > + if (!ASSERT_GE(fd, 0, "create_tmp")) { > > Nit: I would find ASSERT_LE easier to read here. Inverting boolean > conditions is easy to get wrong. You mean if (ASSERT_LE(fd, -1, "create_tmp")) { err = fd; goto done; } ? That will mark the test failing if fd >= 0, which is exactly opposite to what we wan't. It's confusing because CHECK() checks invalid conditions and returns "true" if it holds. But ASSERT_xxx() checks *valid* condition and returns whether valid condition holds. So the pattern is always if (CHECK(expr)) --> if (!ASSERT_xxx(!expr)) And it might feel awkward only when converting original inverted condition. > > > err = fd; > > goto done; > > } > > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_endian.c b/tools/testing/selftests/bpf/prog_tests/btf_endian.c > > index 8c52d72c876e..8ab5d3e358dd 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/btf_endian.c > > +++ b/tools/testing/selftests/bpf/prog_tests/btf_endian.c > > @@ -6,8 +6,6 @@ > > #include <test_progs.h> > > #include <bpf/btf.h> > > > > -static int duration = 0; > > Good to see this go. > > Acked-by: Lorenz Bauer <lmb@cloudflare.com> > > -- > Lorenz Bauer | Systems Engineer > 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK > > www.cloudflare.com
On Mon, Apr 26, 2021 at 1:17 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: > > > > Fix failed tests checks in core_reloc test runner, which allowed failing tests > > to pass quietly. Also add extra check to make sure that expected to fail test cases with > > invalid names are caught as test failure anyway, as this is not an expected > > failure mode. Also fix mislabeled probed vs direct bitfield test cases. > > > > Fixes: 124a892d1c41 ("selftests/bpf: Test TYPE_EXISTS and TYPE_SIZE CO-RE relocations") > > Reported-by: Lorenz Bauer <lmb@cloudflare.com> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > .../selftests/bpf/prog_tests/core_reloc.c | 20 +++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > > index 385fd7696a2e..607710826dca 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c > > +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c > > @@ -217,7 +217,7 @@ static int duration = 0; > > > > #define BITFIELDS_CASE(name, ...) { \ > > BITFIELDS_CASE_COMMON("test_core_reloc_bitfields_probed.o", \ > > - "direct:", name), \ > > + "probed:", name), \ > > .input = STRUCT_TO_CHAR_PTR(core_reloc_##name) __VA_ARGS__, \ > > .input_len = sizeof(struct core_reloc_##name), \ > > .output = STRUCT_TO_CHAR_PTR(core_reloc_bitfields_output) \ > > @@ -225,7 +225,7 @@ static int duration = 0; > > .output_len = sizeof(struct core_reloc_bitfields_output), \ > > }, { \ > > BITFIELDS_CASE_COMMON("test_core_reloc_bitfields_direct.o", \ > > - "probed:", name), \ > > + "direct:", name), \ > > .input = STRUCT_TO_CHAR_PTR(core_reloc_##name) __VA_ARGS__, \ > > .input_len = sizeof(struct core_reloc_##name), \ > > .output = STRUCT_TO_CHAR_PTR(core_reloc_bitfields_output) \ > > @@ -546,8 +546,7 @@ static struct core_reloc_test_case test_cases[] = { > > ARRAYS_ERR_CASE(arrays___err_too_small), > > ARRAYS_ERR_CASE(arrays___err_too_shallow), > > ARRAYS_ERR_CASE(arrays___err_non_array), > > - ARRAYS_ERR_CASE(arrays___err_wrong_val_type1), > > - ARRAYS_ERR_CASE(arrays___err_wrong_val_type2), > > + ARRAYS_ERR_CASE(arrays___err_wrong_val_type), > > ARRAYS_ERR_CASE(arrays___err_bad_zero_sz_arr), > > > > /* enum/ptr/int handling scenarios */ > > @@ -865,13 +864,20 @@ void test_core_reloc(void) > > "prog '%s' not found\n", probe_name)) > > goto cleanup; > > > > + > > + if (test_case->btf_src_file) { > > + err = access(test_case->btf_src_file, R_OK); > > + if (!ASSERT_OK(err, "btf_src_file")) > > + goto cleanup; > > + } > > + > > load_attr.obj = obj; > > load_attr.log_level = 0; > > load_attr.target_btf_path = test_case->btf_src_file; > > err = bpf_object__load_xattr(&load_attr); > > if (err) { > > if (!test_case->fails) > > - CHECK(false, "obj_load", "failed to load prog '%s': %d\n", probe_name, err); > > + ASSERT_OK(err, "obj_load"); > > goto cleanup; > > } > > > > @@ -910,10 +916,8 @@ void test_core_reloc(void) > > goto cleanup; > > } > > > > - if (test_case->fails) { > > - CHECK(false, "obj_load_fail", "should fail to load prog '%s'\n", probe_name); > > + if (!ASSERT_FALSE(test_case->fails, "obj_load_should_fail")) > > Similar to my other comment, I find it difficult to tell when this > triggers. Maybe it makes sense to return the status of the > assertion (not the original value)? So if (assertion()) will be > executed when the assertion fails? Not sure. > ASSERT_XXX() does return the status of assertion -- true if it holds, false if it's violated. So false from ASSERT_xxx() means the test already is marked failed. Mechanically, in this case, it reads as "if we couldn't assert that test_case->fails == false, do something about it". It's the part why test_case->fails should be false is a bit obscure (because we successfully loaded, but test_case is marked as should-be-failed, so test_case->fails has to be false). I hope it helps at least a bit. > Acked-by: Lorenz Bauer <lmb@cloudflare.com> > > -- > Lorenz Bauer | Systems Engineer > 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK > > www.cloudflare.com
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Mon, Apr 26, 2021 at 1:06 AM Lorenz Bauer <lmb@cloudflare.com> wrote: >> >> On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: >> > >> > Add ASSERT_TRUE/ASSERT_FALSE for conditions calculated with custom logic to >> > true/false. Also add remaining arithmetical assertions: >> > - ASSERT_LE -- less than or equal; >> > - ASSERT_GT -- greater than; >> > - ASSERT_GE -- greater than or equal. >> > This should cover most scenarios where people fall back to error-prone >> > CHECK()s. >> > >> > Also extend ASSERT_ERR() to print out errno, in addition to direct error. >> > >> > Also convert few CHECK() instances to ensure new ASSERT_xxx() variants work as >> > expected. Subsequent patch will also use ASSERT_TRUE/ASSERT_FALSE more >> > extensively. >> > >> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >> > --- >> > .../selftests/bpf/prog_tests/btf_dump.c | 2 +- >> > .../selftests/bpf/prog_tests/btf_endian.c | 4 +- >> > .../selftests/bpf/prog_tests/cgroup_link.c | 2 +- >> > .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- >> > .../selftests/bpf/prog_tests/resolve_btfids.c | 7 +-- >> > .../selftests/bpf/prog_tests/snprintf_btf.c | 4 +- >> > tools/testing/selftests/bpf/test_progs.h | 50 ++++++++++++++++++- >> > 7 files changed, 56 insertions(+), 15 deletions(-) >> > >> > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c >> > index c60091ee8a21..5e129dc2073c 100644 >> > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c >> > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c >> > @@ -77,7 +77,7 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t) >> > >> > snprintf(out_file, sizeof(out_file), "/tmp/%s.output.XXXXXX", t->file); >> > fd = mkstemp(out_file); >> > - if (CHECK(fd < 0, "create_tmp", "failed to create file: %d\n", fd)) { >> > + if (!ASSERT_GE(fd, 0, "create_tmp")) { >> >> Nit: I would find ASSERT_LE easier to read here. Inverting boolean >> conditions is easy to get wrong. > > You mean if (ASSERT_LE(fd, -1, "create_tmp")) { err = fd; goto done; } ? > > That will mark the test failing if fd >= 0, which is exactly opposite > to what we wan't. It's confusing because CHECK() checks invalid > conditions and returns "true" if it holds. But ASSERT_xxx() checks > *valid* condition and returns whether valid condition holds. So the > pattern is always There's already an ASSERT_OK_PTR(), so maybe a corresponding ASSERT_OK_FD() would be handy? -Toke
On Mon, Apr 26, 2021 at 8:59 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Mon, Apr 26, 2021 at 1:06 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > >> > >> On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: > >> > > >> > Add ASSERT_TRUE/ASSERT_FALSE for conditions calculated with custom logic to > >> > true/false. Also add remaining arithmetical assertions: > >> > - ASSERT_LE -- less than or equal; > >> > - ASSERT_GT -- greater than; > >> > - ASSERT_GE -- greater than or equal. > >> > This should cover most scenarios where people fall back to error-prone > >> > CHECK()s. > >> > > >> > Also extend ASSERT_ERR() to print out errno, in addition to direct error. > >> > > >> > Also convert few CHECK() instances to ensure new ASSERT_xxx() variants work as > >> > expected. Subsequent patch will also use ASSERT_TRUE/ASSERT_FALSE more > >> > extensively. > >> > > >> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > >> > --- > >> > .../selftests/bpf/prog_tests/btf_dump.c | 2 +- > >> > .../selftests/bpf/prog_tests/btf_endian.c | 4 +- > >> > .../selftests/bpf/prog_tests/cgroup_link.c | 2 +- > >> > .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- > >> > .../selftests/bpf/prog_tests/resolve_btfids.c | 7 +-- > >> > .../selftests/bpf/prog_tests/snprintf_btf.c | 4 +- > >> > tools/testing/selftests/bpf/test_progs.h | 50 ++++++++++++++++++- > >> > 7 files changed, 56 insertions(+), 15 deletions(-) > >> > > >> > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c > >> > index c60091ee8a21..5e129dc2073c 100644 > >> > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c > >> > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c > >> > @@ -77,7 +77,7 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t) > >> > > >> > snprintf(out_file, sizeof(out_file), "/tmp/%s.output.XXXXXX", t->file); > >> > fd = mkstemp(out_file); > >> > - if (CHECK(fd < 0, "create_tmp", "failed to create file: %d\n", fd)) { > >> > + if (!ASSERT_GE(fd, 0, "create_tmp")) { > >> > >> Nit: I would find ASSERT_LE easier to read here. Inverting boolean > >> conditions is easy to get wrong. > > > > You mean if (ASSERT_LE(fd, -1, "create_tmp")) { err = fd; goto done; } ? > > > > That will mark the test failing if fd >= 0, which is exactly opposite > > to what we wan't. It's confusing because CHECK() checks invalid > > conditions and returns "true" if it holds. But ASSERT_xxx() checks > > *valid* condition and returns whether valid condition holds. So the > > pattern is always > > There's already an ASSERT_OK_PTR(), so maybe a corresponding > ASSERT_OK_FD() would be handy? I honestly don't see the point. OK_PTR is special, it checks NULL and the special ERR_PTR() variants, which is a lot of hassle to check manually. While for FD doing ASSERT_GE(fd, 0) seems to be fine and just mostly natural. Also for some APIs valid FD is > 0 and for other cases valid FD is plain >= 0, so that just adds to the confusion. > > -Toke >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Mon, Apr 26, 2021 at 8:59 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Mon, Apr 26, 2021 at 1:06 AM Lorenz Bauer <lmb@cloudflare.com> wrote: >> >> >> >> On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote: >> >> > >> >> > Add ASSERT_TRUE/ASSERT_FALSE for conditions calculated with custom logic to >> >> > true/false. Also add remaining arithmetical assertions: >> >> > - ASSERT_LE -- less than or equal; >> >> > - ASSERT_GT -- greater than; >> >> > - ASSERT_GE -- greater than or equal. >> >> > This should cover most scenarios where people fall back to error-prone >> >> > CHECK()s. >> >> > >> >> > Also extend ASSERT_ERR() to print out errno, in addition to direct error. >> >> > >> >> > Also convert few CHECK() instances to ensure new ASSERT_xxx() variants work as >> >> > expected. Subsequent patch will also use ASSERT_TRUE/ASSERT_FALSE more >> >> > extensively. >> >> > >> >> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >> >> > --- >> >> > .../selftests/bpf/prog_tests/btf_dump.c | 2 +- >> >> > .../selftests/bpf/prog_tests/btf_endian.c | 4 +- >> >> > .../selftests/bpf/prog_tests/cgroup_link.c | 2 +- >> >> > .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- >> >> > .../selftests/bpf/prog_tests/resolve_btfids.c | 7 +-- >> >> > .../selftests/bpf/prog_tests/snprintf_btf.c | 4 +- >> >> > tools/testing/selftests/bpf/test_progs.h | 50 ++++++++++++++++++- >> >> > 7 files changed, 56 insertions(+), 15 deletions(-) >> >> > >> >> > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c >> >> > index c60091ee8a21..5e129dc2073c 100644 >> >> > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c >> >> > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c >> >> > @@ -77,7 +77,7 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t) >> >> > >> >> > snprintf(out_file, sizeof(out_file), "/tmp/%s.output.XXXXXX", t->file); >> >> > fd = mkstemp(out_file); >> >> > - if (CHECK(fd < 0, "create_tmp", "failed to create file: %d\n", fd)) { >> >> > + if (!ASSERT_GE(fd, 0, "create_tmp")) { >> >> >> >> Nit: I would find ASSERT_LE easier to read here. Inverting boolean >> >> conditions is easy to get wrong. >> > >> > You mean if (ASSERT_LE(fd, -1, "create_tmp")) { err = fd; goto done; } ? >> > >> > That will mark the test failing if fd >= 0, which is exactly opposite >> > to what we wan't. It's confusing because CHECK() checks invalid >> > conditions and returns "true" if it holds. But ASSERT_xxx() checks >> > *valid* condition and returns whether valid condition holds. So the >> > pattern is always >> >> There's already an ASSERT_OK_PTR(), so maybe a corresponding >> ASSERT_OK_FD() would be handy? > > I honestly don't see the point. OK_PTR is special, it checks NULL and > the special ERR_PTR() variants, which is a lot of hassle to check > manually. While for FD doing ASSERT_GE(fd, 0) seems to be fine and > just mostly natural. Also for some APIs valid FD is > 0 and for other > cases valid FD is plain >= 0, so that just adds to the confusion. Alright, fair enough. I just wondered because I had the same feeling of slight awkwardness when I was writing an fd check the other day, so thought I'd air the thought; but as you say not *really* a big deal, so I'm also OK with just using LE or GE for this... -Toke
Lorenz Bauer noticed that core_reloc selftest has two inverted CHECK() conditions, allowing failing tests to pass unnoticed. Fixing that opened up few long-standing (field existence and direct memory bitfields) and one recent failures (BTF_KIND_FLOAT relos). This patch set fixes core_reloc selftest to capture such failures reliably in the future. It also fixes all the newly failing tests. See individual patches for details. This patch set also completes a set of ASSERT_xxx() macros, so now there should be a very little reason to use verbose and error-prone generic CHECK() macro. Cc: Lorenz Bauer <lmb@cloudflare.com> Andrii Nakryiko (5): selftests/bpf: add remaining ASSERT_xxx() variants libbpf: support BTF_KIND_FLOAT during type compatibility checks in CO-RE selftests/bpf: fix BPF_CORE_READ_BITFIELD() macro selftests/bpf: fix field existence CO-RE reloc tests selftests/bpf: fix core_reloc test runner tools/lib/bpf/bpf_core_read.h | 16 ++++-- tools/lib/bpf/libbpf.c | 5 +- .../selftests/bpf/prog_tests/btf_dump.c | 2 +- .../selftests/bpf/prog_tests/btf_endian.c | 4 +- .../selftests/bpf/prog_tests/cgroup_link.c | 2 +- .../selftests/bpf/prog_tests/core_reloc.c | 51 +++++++++++-------- .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- .../selftests/bpf/prog_tests/resolve_btfids.c | 7 +-- .../selftests/bpf/prog_tests/snprintf_btf.c | 4 +- ...ore_reloc_existence___err_wrong_arr_kind.c | 3 -- ...loc_existence___err_wrong_arr_value_type.c | 3 -- ...ore_reloc_existence___err_wrong_int_kind.c | 3 -- ..._core_reloc_existence___err_wrong_int_sz.c | 3 -- ...ore_reloc_existence___err_wrong_int_type.c | 3 -- ..._reloc_existence___err_wrong_struct_type.c | 3 -- ..._core_reloc_existence___wrong_field_defs.c | 3 ++ .../selftests/bpf/progs/core_reloc_types.h | 20 +------- tools/testing/selftests/bpf/test_progs.h | 50 +++++++++++++++++- 18 files changed, 107 insertions(+), 77 deletions(-) delete mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_kind.c delete mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_value_type.c delete mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_kind.c delete mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_sz.c delete mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_type.c delete mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_struct_type.c create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_existence___wrong_field_defs.c