| Message ID | 20210423233058.3386115-6-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | CO-RE relocation selftests fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 2 blamed authors not CCed: ast@kernel.org yhs@fb.com; 9 maintainers not CCed: linux-kselftest@vger.kernel.org yhs@fb.com kpsingh@kernel.org kafai@fb.com iii@linux.ibm.com ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com shuah@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Please don't use multiple blank lines |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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>
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
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")) goto cleanup; - } equal = memcmp(data->out, test_case->output, test_case->output_len) == 0;
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(-)