* [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants
2021-04-23 23:30 [PATCH bpf-next 0/5] CO-RE relocation selftests fixes Andrii Nakryiko
@ 2021-04-23 23:30 ` Andrii Nakryiko
2021-04-26 8:05 ` Lorenz Bauer
2021-04-23 23:30 ` [PATCH bpf-next 2/5] libbpf: support BTF_KIND_FLOAT during type compatibility checks in CO-RE Andrii Nakryiko
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-23 23:30 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team, Lorenz Bauer
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")) {
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;
-
void test_btf_endian() {
#if __BYTE_ORDER == __LITTLE_ENDIAN
enum btf_endianness endian = BTF_LITTLE_ENDIAN;
@@ -71,7 +69,7 @@ void test_btf_endian() {
/* now modify original BTF */
var_id = btf__add_var(btf, "some_var", BTF_VAR_GLOBAL_ALLOCATED, 1);
- CHECK(var_id <= 0, "var_id", "failed %d\n", var_id);
+ ASSERT_GT(var_id, 0, "var_id");
btf__free(swap_btf);
swap_btf = NULL;
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_link.c b/tools/testing/selftests/bpf/prog_tests/cgroup_link.c
index 4d9b514b3fd9..736796e56ed1 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_link.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_link.c
@@ -54,7 +54,7 @@ void test_cgroup_link(void)
for (i = 0; i < cg_nr; i++) {
cgs[i].fd = create_and_get_cgroup(cgs[i].path);
- if (CHECK(cgs[i].fd < 0, "cg_create", "fail: %d\n", cgs[i].fd))
+ if (!ASSERT_GE(cgs[i].fd, 0, "cg_create"))
goto cleanup;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
index 42c3a3103c26..d65107919998 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
@@ -134,7 +134,7 @@ void test_kfree_skb(void)
/* make sure kfree_skb program was triggered
* and it sent expected skb into ring buffer
*/
- CHECK_FAIL(!passed);
+ ASSERT_TRUE(passed, "passed");
err = bpf_map_lookup_elem(bpf_map__fd(global_data), &zero, test_ok);
if (CHECK(err, "get_result",
diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
index 6ace5e9efec1..d3c2de2c24d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
+++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
@@ -160,11 +160,8 @@ int test_resolve_btfids(void)
break;
if (i > 0) {
- ret = CHECK(test_set.ids[i - 1] > test_set.ids[i],
- "sort_check",
- "test_set is not sorted\n");
- if (ret)
- break;
+ if (!ASSERT_LE(test_set.ids[i - 1], test_set.ids[i], "sort_check"))
+ return -1;
}
}
diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c b/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c
index 686b40f11a45..76e1f5fe18fa 100644
--- a/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c
@@ -42,9 +42,7 @@ void test_snprintf_btf(void)
* and it set expected return values from bpf_trace_printk()s
* and all tests ran.
*/
- if (CHECK(bss->ret <= 0,
- "bpf_snprintf_btf: got return value",
- "ret <= 0 %ld test %d\n", bss->ret, bss->ran_subtests))
+ if (!ASSERT_GT(bss->ret, 0, "bpf_snprintf_ret"))
goto cleanup;
if (CHECK(bss->ran_subtests == 0, "check if subtests ran",
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index ee7e3b45182a..dda52cb649dc 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -130,6 +130,20 @@ extern int test__join_cgroup(const char *path);
#define CHECK_ATTR(condition, tag, format...) \
_CHECK(condition, tag, tattr.duration, format)
+#define ASSERT_TRUE(actual, name) ({ \
+ static int duration = 0; \
+ bool ___ok = (actual); \
+ CHECK(!___ok, (name), "unexpected %s: got FALSE\n", (name)); \
+ ___ok; \
+})
+
+#define ASSERT_FALSE(actual, name) ({ \
+ static int duration = 0; \
+ bool ___ok = !(actual); \
+ CHECK(!___ok, (name), "unexpected %s: got TRUE\n", (name)); \
+ ___ok; \
+})
+
#define ASSERT_EQ(actual, expected, name) ({ \
static int duration = 0; \
typeof(actual) ___act = (actual); \
@@ -163,6 +177,39 @@ extern int test__join_cgroup(const char *path);
___ok; \
})
+#define ASSERT_LE(actual, expected, name) ({ \
+ static int duration = 0; \
+ typeof(actual) ___act = (actual); \
+ typeof(expected) ___exp = (expected); \
+ bool ___ok = ___act <= ___exp; \
+ CHECK(!___ok, (name), \
+ "unexpected %s: actual %lld > expected %lld\n", \
+ (name), (long long)(___act), (long long)(___exp)); \
+ ___ok; \
+})
+
+#define ASSERT_GT(actual, expected, name) ({ \
+ static int duration = 0; \
+ typeof(actual) ___act = (actual); \
+ typeof(expected) ___exp = (expected); \
+ bool ___ok = ___act > ___exp; \
+ CHECK(!___ok, (name), \
+ "unexpected %s: actual %lld <= expected %lld\n", \
+ (name), (long long)(___act), (long long)(___exp)); \
+ ___ok; \
+})
+
+#define ASSERT_GE(actual, expected, name) ({ \
+ static int duration = 0; \
+ typeof(actual) ___act = (actual); \
+ typeof(expected) ___exp = (expected); \
+ bool ___ok = ___act >= ___exp; \
+ CHECK(!___ok, (name), \
+ "unexpected %s: actual %lld < expected %lld\n", \
+ (name), (long long)(___act), (long long)(___exp)); \
+ ___ok; \
+})
+
#define ASSERT_STREQ(actual, expected, name) ({ \
static int duration = 0; \
const char *___act = actual; \
@@ -178,7 +225,8 @@ extern int test__join_cgroup(const char *path);
static int duration = 0; \
long long ___res = (res); \
bool ___ok = ___res == 0; \
- CHECK(!___ok, (name), "unexpected error: %lld\n", ___res); \
+ CHECK(!___ok, (name), "unexpected error: %lld (errno %d)\n", \
+ ___res, errno); \
___ok; \
})
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants
2021-04-23 23:30 ` [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants Andrii Nakryiko
@ 2021-04-26 8:05 ` Lorenz Bauer
2021-04-26 15:50 ` Andrii Nakryiko
0 siblings, 1 reply; 16+ messages in thread
From: Lorenz Bauer @ 2021-04-26 8:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants
2021-04-26 8:05 ` Lorenz Bauer
@ 2021-04-26 15:50 ` Andrii Nakryiko
2021-04-26 15:59 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-26 15:50 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants
2021-04-26 15:50 ` Andrii Nakryiko
@ 2021-04-26 15:59 ` Toke Høiland-Jørgensen
2021-04-26 16:15 ` Andrii Nakryiko
0 siblings, 1 reply; 16+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-04-26 15:59 UTC (permalink / raw)
To: Andrii Nakryiko, Lorenz Bauer
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants
2021-04-26 15:59 ` Toke Høiland-Jørgensen
@ 2021-04-26 16:15 ` Andrii Nakryiko
2021-04-26 16:44 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-26 16:15 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Lorenz Bauer, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants
2021-04-26 16:15 ` Andrii Nakryiko
@ 2021-04-26 16:44 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 16+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-04-26 16:44 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Lorenz Bauer, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next 2/5] libbpf: support BTF_KIND_FLOAT during type compatibility checks in CO-RE
2021-04-23 23:30 [PATCH bpf-next 0/5] CO-RE relocation selftests fixes Andrii Nakryiko
2021-04-23 23:30 ` [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants Andrii Nakryiko
@ 2021-04-23 23:30 ` Andrii Nakryiko
2021-04-26 8:07 ` Lorenz Bauer
2021-04-23 23:30 ` [PATCH bpf-next 3/5] selftests/bpf: fix BPF_CORE_READ_BITFIELD() macro Andrii Nakryiko
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-23 23:30 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team, Lorenz Bauer
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:
return 1;
case BTF_KIND_FWD:
case BTF_KIND_ENUM: {
@@ -6245,8 +6246,8 @@ static int bpf_core_apply_relo(struct bpf_program *prog,
/* bpf_core_patch_insn() should know how to handle missing targ_spec */
err = bpf_core_patch_insn(prog, relo, relo_idx, &targ_res);
if (err) {
- pr_warn("prog '%s': relo #%d: failed to patch insn at offset %d: %d\n",
- prog->name, relo_idx, relo->insn_off, err);
+ pr_warn("prog '%s': relo #%d: failed to patch insn #%zu: %d\n",
+ prog->name, relo_idx, relo->insn_off / BPF_INSN_SZ, err);
return -EINVAL;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 2/5] libbpf: support BTF_KIND_FLOAT during type compatibility checks in CO-RE
2021-04-23 23:30 ` [PATCH bpf-next 2/5] libbpf: support BTF_KIND_FLOAT during type compatibility checks in CO-RE Andrii Nakryiko
@ 2021-04-26 8:07 ` Lorenz Bauer
0 siblings, 0 replies; 16+ messages in thread
From: Lorenz Bauer @ 2021-04-26 8:07 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next 3/5] selftests/bpf: fix BPF_CORE_READ_BITFIELD() macro
2021-04-23 23:30 [PATCH bpf-next 0/5] CO-RE relocation selftests fixes Andrii Nakryiko
2021-04-23 23:30 ` [PATCH bpf-next 1/5] selftests/bpf: add remaining ASSERT_xxx() variants Andrii Nakryiko
2021-04-23 23:30 ` [PATCH bpf-next 2/5] libbpf: support BTF_KIND_FLOAT during type compatibility checks in CO-RE Andrii Nakryiko
@ 2021-04-23 23:30 ` Andrii Nakryiko
2021-04-26 8:10 ` Lorenz Bauer
2021-04-23 23:30 ` [PATCH bpf-next 4/5] selftests/bpf: fix field existence CO-RE reloc tests Andrii Nakryiko
2021-04-23 23:30 ` [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner Andrii Nakryiko
4 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-23 23:30 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team, Lorenz Bauer
Fix BPF_CORE_READ_BITFIELD() macro used for reading CO-RE-relocatable
bitfields. Missing breaks in a switch caused 8-byte reads always. This can
confuse libbpf because it does strict checks that memory load size corresponds
to the original size of the field, which in this case quite often would be
wrong.
After fixing that, we run into another problem, which quite subtle, so worth
documenting here. The issue is in Clang optimization and CO-RE relocation
interactions. Without that asm volatile construct (also known as
barrier_var()), Clang will re-order BYTE_OFFSET and BYTE_SIZE relocations and
will apply BYTE_OFFSET 4 times for each switch case arm. This will result in
the same error from libbpf about mismatch of memory load size and original
field size. I.e., if we were reading u32, we'd still have *(u8 *), *(u16 *),
*(u32 *), and *(u64 *) memory loads, three of which will fail. Using
barrier_var() forces Clang to apply BYTE_OFFSET relocation first (and once) to
calculate p, after which value of p is used without relocation in each of
switch case arms, doing appropiately-sized memory load.
Here's the list of relevant relocations and pieces of generated BPF code
before and after this patch for test_core_reloc_bitfields_direct selftests.
BEFORE
=====
#45: core_reloc: insn #160 --> [5] + 0:5: byte_sz --> struct core_reloc_bitfields.u32
#46: core_reloc: insn #167 --> [5] + 0:5: byte_off --> struct core_reloc_bitfields.u32
#47: core_reloc: insn #174 --> [5] + 0:5: byte_off --> struct core_reloc_bitfields.u32
#48: core_reloc: insn #178 --> [5] + 0:5: byte_off --> struct core_reloc_bitfields.u32
#49: core_reloc: insn #182 --> [5] + 0:5: byte_off --> struct core_reloc_bitfields.u32
157: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0 ll
159: 7b 12 20 01 00 00 00 00 *(u64 *)(r2 + 288) = r1
160: b7 02 00 00 04 00 00 00 r2 = 4
; BYTE_SIZE relocation here ^^^
161: 66 02 07 00 03 00 00 00 if w2 s> 3 goto +7 <LBB0_63>
162: 16 02 0d 00 01 00 00 00 if w2 == 1 goto +13 <LBB0_65>
163: 16 02 01 00 02 00 00 00 if w2 == 2 goto +1 <LBB0_66>
164: 05 00 12 00 00 00 00 00 goto +18 <LBB0_69>
0000000000000528 <LBB0_66>:
165: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
167: 69 11 08 00 00 00 00 00 r1 = *(u16 *)(r1 + 8)
; BYTE_OFFSET relo here w/ WRONG size ^^^^^^^^^^^^^^^^
168: 05 00 0e 00 00 00 00 00 goto +14 <LBB0_69>
0000000000000548 <LBB0_63>:
169: 16 02 0a 00 04 00 00 00 if w2 == 4 goto +10 <LBB0_67>
170: 16 02 01 00 08 00 00 00 if w2 == 8 goto +1 <LBB0_68>
171: 05 00 0b 00 00 00 00 00 goto +11 <LBB0_69>
0000000000000560 <LBB0_68>:
172: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
174: 79 11 08 00 00 00 00 00 r1 = *(u64 *)(r1 + 8)
; BYTE_OFFSET relo here w/ WRONG size ^^^^^^^^^^^^^^^^
175: 05 00 07 00 00 00 00 00 goto +7 <LBB0_69>
0000000000000580 <LBB0_65>:
176: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
178: 71 11 08 00 00 00 00 00 r1 = *(u8 *)(r1 + 8)
; BYTE_OFFSET relo here w/ WRONG size ^^^^^^^^^^^^^^^^
179: 05 00 03 00 00 00 00 00 goto +3 <LBB0_69>
00000000000005a0 <LBB0_67>:
180: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
182: 61 11 08 00 00 00 00 00 r1 = *(u32 *)(r1 + 8)
; BYTE_OFFSET relo here w/ RIGHT size ^^^^^^^^^^^^^^^^
00000000000005b8 <LBB0_69>:
183: 67 01 00 00 20 00 00 00 r1 <<= 32
184: b7 02 00 00 00 00 00 00 r2 = 0
185: 16 02 02 00 00 00 00 00 if w2 == 0 goto +2 <LBB0_71>
186: c7 01 00 00 20 00 00 00 r1 s>>= 32
187: 05 00 01 00 00 00 00 00 goto +1 <LBB0_72>
00000000000005e0 <LBB0_71>:
188: 77 01 00 00 20 00 00 00 r1 >>= 32
AFTER
=====
#30: core_reloc: insn #132 --> [5] + 0:5: byte_off --> struct core_reloc_bitfields.u32
#31: core_reloc: insn #134 --> [5] + 0:5: byte_sz --> struct core_reloc_bitfields.u32
129: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0 ll
131: 7b 12 20 01 00 00 00 00 *(u64 *)(r2 + 288) = r1
132: b7 01 00 00 08 00 00 00 r1 = 8
; BYTE_OFFSET relo here ^^^
; no size check for non-memory dereferencing instructions
133: 0f 12 00 00 00 00 00 00 r2 += r1
134: b7 03 00 00 04 00 00 00 r3 = 4
; BYTE_SIZE relocation here ^^^
135: 66 03 05 00 03 00 00 00 if w3 s> 3 goto +5 <LBB0_63>
136: 16 03 09 00 01 00 00 00 if w3 == 1 goto +9 <LBB0_65>
137: 16 03 01 00 02 00 00 00 if w3 == 2 goto +1 <LBB0_66>
138: 05 00 0a 00 00 00 00 00 goto +10 <LBB0_69>
0000000000000458 <LBB0_66>:
139: 69 21 00 00 00 00 00 00 r1 = *(u16 *)(r2 + 0)
; NO CO-RE relocation here ^^^^^^^^^^^^^^^^
140: 05 00 08 00 00 00 00 00 goto +8 <LBB0_69>
0000000000000468 <LBB0_63>:
141: 16 03 06 00 04 00 00 00 if w3 == 4 goto +6 <LBB0_67>
142: 16 03 01 00 08 00 00 00 if w3 == 8 goto +1 <LBB0_68>
143: 05 00 05 00 00 00 00 00 goto +5 <LBB0_69>
0000000000000480 <LBB0_68>:
144: 79 21 00 00 00 00 00 00 r1 = *(u64 *)(r2 + 0)
; NO CO-RE relocation here ^^^^^^^^^^^^^^^^
145: 05 00 03 00 00 00 00 00 goto +3 <LBB0_69>
0000000000000490 <LBB0_65>:
146: 71 21 00 00 00 00 00 00 r1 = *(u8 *)(r2 + 0)
; NO CO-RE relocation here ^^^^^^^^^^^^^^^^
147: 05 00 01 00 00 00 00 00 goto +1 <LBB0_69>
00000000000004a0 <LBB0_67>:
148: 61 21 00 00 00 00 00 00 r1 = *(u32 *)(r2 + 0)
; NO CO-RE relocation here ^^^^^^^^^^^^^^^^
00000000000004a8 <LBB0_69>:
149: 67 01 00 00 20 00 00 00 r1 <<= 32
150: b7 02 00 00 00 00 00 00 r2 = 0
151: 16 02 02 00 00 00 00 00 if w2 == 0 goto +2 <LBB0_71>
152: c7 01 00 00 20 00 00 00 r1 s>>= 32
153: 05 00 01 00 00 00 00 00 goto +1 <LBB0_72>
00000000000004d0 <LBB0_71>:
154: 77 01 00 00 20 00 00 00 r1 >>= 323
Fixes: ee26dade0e3b ("libbpf: Add support for relocatable bitfields")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf_core_read.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index 53b3e199fb25..09ebe3db5f2f 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -88,11 +88,19 @@ enum bpf_enum_value_kind {
const void *p = (const void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \
unsigned long long val; \
\
+ /* This is a so-called barrier_var() operation that makes specified \
+ * variable "a black box" for optimizing compiler. \
+ * It forces compiler to perform BYTE_OFFSET relocation on p and use \
+ * its calculated value in the switch below, instead of applying \
+ * the same relocation 4 times for each individual memory load. \
+ */ \
+ asm volatile("" : "=r"(p) : "0"(p)); \
+ \
switch (__CORE_RELO(s, field, BYTE_SIZE)) { \
- case 1: val = *(const unsigned char *)p; \
- case 2: val = *(const unsigned short *)p; \
- case 4: val = *(const unsigned int *)p; \
- case 8: val = *(const unsigned long long *)p; \
+ case 1: val = *(const unsigned char *)p; break; \
+ case 2: val = *(const unsigned short *)p; break; \
+ case 4: val = *(const unsigned int *)p; break; \
+ case 8: val = *(const unsigned long long *)p; break; \
} \
val <<= __CORE_RELO(s, field, LSHIFT_U64); \
if (__CORE_RELO(s, field, SIGNED)) \
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 3/5] selftests/bpf: fix BPF_CORE_READ_BITFIELD() macro
2021-04-23 23:30 ` [PATCH bpf-next 3/5] selftests/bpf: fix BPF_CORE_READ_BITFIELD() macro Andrii Nakryiko
@ 2021-04-26 8:10 ` Lorenz Bauer
0 siblings, 0 replies; 16+ messages in thread
From: Lorenz Bauer @ 2021-04-26 8:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Fix BPF_CORE_READ_BITFIELD() macro used for reading CO-RE-relocatable
> bitfields. Missing breaks in a switch caused 8-byte reads always. This can
> confuse libbpf because it does strict checks that memory load size corresponds
> to the original size of the field, which in this case quite often would be
> wrong.
Acked-by: Lorenz Bauer <lmb@cloudflare.com>
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next 4/5] selftests/bpf: fix field existence CO-RE reloc tests
2021-04-23 23:30 [PATCH bpf-next 0/5] CO-RE relocation selftests fixes Andrii Nakryiko
` (2 preceding siblings ...)
2021-04-23 23:30 ` [PATCH bpf-next 3/5] selftests/bpf: fix BPF_CORE_READ_BITFIELD() macro Andrii Nakryiko
@ 2021-04-23 23:30 ` Andrii Nakryiko
2021-04-26 8:12 ` Lorenz Bauer
2021-04-23 23:30 ` [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner Andrii Nakryiko
4 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-23 23:30 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team, Lorenz Bauer
Negative field existence cases for have a broken assumption that FIELD_EXISTS
CO-RE relo will fail for fields that match the name but have incompatible type
signature. That's not how CO-RE relocations generally behave. Types and fields
that match by name but not by expected type are treated as non-matching
candidates and are skipped. Error later is reported if no matching candidate
was found. That's what happens for most relocations, but existence relocations
(FIELD_EXISTS and TYPE_EXISTS) are more permissive and they are designed to
return 0 or 1, depending if a match is found. This allows to handle
name-conflicting but incompatible types in BPF code easily. Combined with
___flavor suffixes, it's possible to handle pretty much any structural type
changes in kernel within the compiled once BPF source code.
So, long story short, negative field existence test cases are invalid in their
assumptions, so this patch reworks them into a single consolidated positive
case that doesn't match any of the fields.
Fixes: c7566a69695c ("selftests/bpf: Add field existence CO-RE relocs tests")
Reported-by: Lorenz Bauer <lmb@cloudflare.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/prog_tests/core_reloc.c | 31 ++++++++++++-------
...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 ++----------
9 files changed, 24 insertions(+), 48 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
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index d94dcead72e6..385fd7696a2e 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -210,11 +210,6 @@ static int duration = 0;
.bpf_obj_file = "test_core_reloc_existence.o", \
.btf_src_file = "btf__core_reloc_" #name ".o" \
-#define FIELD_EXISTS_ERR_CASE(name) { \
- FIELD_EXISTS_CASE_COMMON(name), \
- .fails = true, \
-}
-
#define BITFIELDS_CASE_COMMON(objfile, test_name_prefix, name) \
.case_name = test_name_prefix#name, \
.bpf_obj_file = objfile, \
@@ -643,13 +638,25 @@ static struct core_reloc_test_case test_cases[] = {
},
.output_len = sizeof(struct core_reloc_existence_output),
},
-
- FIELD_EXISTS_ERR_CASE(existence__err_int_sz),
- FIELD_EXISTS_ERR_CASE(existence__err_int_type),
- FIELD_EXISTS_ERR_CASE(existence__err_int_kind),
- FIELD_EXISTS_ERR_CASE(existence__err_arr_kind),
- FIELD_EXISTS_ERR_CASE(existence__err_arr_value_type),
- FIELD_EXISTS_ERR_CASE(existence__err_struct_type),
+ {
+ FIELD_EXISTS_CASE_COMMON(existence___wrong_field_defs),
+ .input = STRUCT_TO_CHAR_PTR(core_reloc_existence___wrong_field_defs) {
+ },
+ .input_len = sizeof(struct core_reloc_existence___wrong_field_defs),
+ .output = STRUCT_TO_CHAR_PTR(core_reloc_existence_output) {
+ .a_exists = 0,
+ .b_exists = 0,
+ .c_exists = 0,
+ .arr_exists = 0,
+ .s_exists = 0,
+ .a_value = 0xff000001u,
+ .b_value = 0xff000002u,
+ .c_value = 0xff000003u,
+ .arr_value = 0xff000004u,
+ .s_value = 0xff000005u,
+ },
+ .output_len = sizeof(struct core_reloc_existence_output),
+ },
/* bitfield relocation checks */
BITFIELDS_CASE(bitfields, {
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_kind.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_kind.c
deleted file mode 100644
index dd0ffa518f36..000000000000
--- a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_kind.c
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "core_reloc_types.h"
-
-void f(struct core_reloc_existence___err_wrong_arr_kind x) {}
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_value_type.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_value_type.c
deleted file mode 100644
index bc83372088ad..000000000000
--- a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_arr_value_type.c
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "core_reloc_types.h"
-
-void f(struct core_reloc_existence___err_wrong_arr_value_type x) {}
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_kind.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_kind.c
deleted file mode 100644
index 917bec41be08..000000000000
--- a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_kind.c
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "core_reloc_types.h"
-
-void f(struct core_reloc_existence___err_wrong_int_kind x) {}
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_sz.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_sz.c
deleted file mode 100644
index 6ec7e6ec1c91..000000000000
--- a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_sz.c
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "core_reloc_types.h"
-
-void f(struct core_reloc_existence___err_wrong_int_sz x) {}
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_type.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_type.c
deleted file mode 100644
index 7bbcacf2b0d1..000000000000
--- a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_int_type.c
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "core_reloc_types.h"
-
-void f(struct core_reloc_existence___err_wrong_int_type x) {}
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_struct_type.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_struct_type.c
deleted file mode 100644
index f384dd38ec70..000000000000
--- a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___err_wrong_struct_type.c
+++ /dev/null
@@ -1,3 +0,0 @@
-#include "core_reloc_types.h"
-
-void f(struct core_reloc_existence___err_wrong_struct_type x) {}
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___wrong_field_defs.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___wrong_field_defs.c
new file mode 100644
index 000000000000..d14b496190c3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btf__core_reloc_existence___wrong_field_defs.c
@@ -0,0 +1,3 @@
+#include "core_reloc_types.h"
+
+void f(struct core_reloc_existence___wrong_field_defs x) {}
diff --git a/tools/testing/selftests/bpf/progs/core_reloc_types.h b/tools/testing/selftests/bpf/progs/core_reloc_types.h
index 9982eb969048..c95c0cabe951 100644
--- a/tools/testing/selftests/bpf/progs/core_reloc_types.h
+++ b/tools/testing/selftests/bpf/progs/core_reloc_types.h
@@ -700,27 +700,11 @@ struct core_reloc_existence___minimal {
int a;
};
-struct core_reloc_existence___err_wrong_int_sz {
- short a;
-};
-
-struct core_reloc_existence___err_wrong_int_type {
+struct core_reloc_existence___wrong_field_defs {
+ void *a;
int b[1];
-};
-
-struct core_reloc_existence___err_wrong_int_kind {
struct{ int x; } c;
-};
-
-struct core_reloc_existence___err_wrong_arr_kind {
int arr;
-};
-
-struct core_reloc_existence___err_wrong_arr_value_type {
- short arr[1];
-};
-
-struct core_reloc_existence___err_wrong_struct_type {
int s;
};
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 4/5] selftests/bpf: fix field existence CO-RE reloc tests
2021-04-23 23:30 ` [PATCH bpf-next 4/5] selftests/bpf: fix field existence CO-RE reloc tests Andrii Nakryiko
@ 2021-04-26 8:12 ` Lorenz Bauer
0 siblings, 0 replies; 16+ messages in thread
From: Lorenz Bauer @ 2021-04-26 8:12 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Negative field existence cases for have a broken assumption that FIELD_EXISTS
> CO-RE relo will fail for fields that match the name but have incompatible type
> signature. That's not how CO-RE relocations generally behave. Types and fields
> that match by name but not by expected type are treated as non-matching
> candidates and are skipped. Error later is reported if no matching candidate
> was found. That's what happens for most relocations, but existence relocations
> (FIELD_EXISTS and TYPE_EXISTS) are more permissive and they are designed to
> return 0 or 1, depending if a match is found. This allows to handle
> name-conflicting but incompatible types in BPF code easily. Combined with
> ___flavor suffixes, it's possible to handle pretty much any structural type
> changes in kernel within the compiled once BPF source code.
>
> So, long story short, negative field existence test cases are invalid in their
> assumptions, so this patch reworks them into a single consolidated positive
> case that doesn't match any of the fields.
>
> Fixes: c7566a69695c ("selftests/bpf: Add field existence CO-RE relocs tests")
> Reported-by: Lorenz Bauer <lmb@cloudflare.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Lorenz Bauer <lmb@cloudflare.com>
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner
2021-04-23 23:30 [PATCH bpf-next 0/5] CO-RE relocation selftests fixes Andrii Nakryiko
` (3 preceding siblings ...)
2021-04-23 23:30 ` [PATCH bpf-next 4/5] selftests/bpf: fix field existence CO-RE reloc tests Andrii Nakryiko
@ 2021-04-23 23:30 ` Andrii Nakryiko
2021-04-26 8:16 ` Lorenz Bauer
4 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-23 23:30 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team, Lorenz Bauer
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"))
goto cleanup;
- }
equal = memcmp(data->out, test_case->output,
test_case->output_len) == 0;
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner
2021-04-23 23:30 ` [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner Andrii Nakryiko
@ 2021-04-26 8:16 ` Lorenz Bauer
2021-04-26 15:55 ` Andrii Nakryiko
0 siblings, 1 reply; 16+ messages in thread
From: Lorenz Bauer @ 2021-04-26 8:16 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner
2021-04-26 8:16 ` Lorenz Bauer
@ 2021-04-26 15:55 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-04-26 15:55 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 16+ messages in thread