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

From: Lorenz Bauer <lmb@cloudflare.com>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: 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 09:16:51 +0100	[thread overview]
Message-ID: <CACAyw98cvRe6rE8XOBZfd7v=_5X45U=Qb0AtWJi5Kw2hWccpFQ@mail.gmail.com> (raw)
In-Reply-To: <20210423233058.3386115-6-andrii@kernel.org>

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