Re: [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Lorenz Bauer <lmb@cloudflare.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner
Date: Mon, 26 Apr 2021 08:55:10 -0700	[thread overview]
Message-ID: <CAEf4BzYTg+eawA9gbBM30QZpwS=wTNCpG4SsFNiLctKjChyFNg@mail.gmail.com> (raw)
In-Reply-To: <CACAyw98cvRe6rE8XOBZfd7v=_5X45U=Qb0AtWJi5Kw2hWccpFQ@mail.gmail.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