| Message ID | 20210408195740.153029-2-toke@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] bpf: return target info when a tracing bpf_link is queried | 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 | warning | 4 maintainers not CCed: linux-kselftest@vger.kernel.org netdev@vger.kernel.org udippant@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 | WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, Apr 8, 2021 at 12:57 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Extend the fexit_bpf2bpf test to check that the info for the bpf_link > returned by the kernel matches the expected values. > > While we're updating the test, change existing uses of CHEC() to use the > much easier to read ASSERT_*() macros. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- Just a minor nit below. Looks good, thanks. Acked-by: Andrii Nakryiko <andrii@kernel.org> > .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 50 +++++++++++++++---- > 1 file changed, 39 insertions(+), 11 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c > index 5c0448910426..019a46d8e98e 100644 > --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c > +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c > @@ -57,11 +57,13 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, > bool run_prog, > test_cb cb) > { > + __u32 duration = 0, retval, tgt_prog_id, info_len; if not CHECK() is used, duration shouldn't be needed anymore > struct bpf_object *obj = NULL, *tgt_obj; > + struct bpf_prog_info prog_info = {}; > struct bpf_program **prog = NULL; > struct bpf_link **link = NULL; > - __u32 duration = 0, retval; > int err, tgt_fd, i; > + struct btf *btf; > > err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC, > &tgt_obj, &tgt_fd); > @@ -72,28 +74,55 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, > .attach_prog_fd = tgt_fd, > ); > > + info_len = sizeof(prog_info); > + err = bpf_obj_get_info_by_fd(tgt_fd, &prog_info, &info_len); > + if (!ASSERT_OK(err, "tgt_fd_get_info")) > + goto close_prog; > + > + tgt_prog_id = prog_info.id; > + btf = bpf_object__btf(tgt_obj); > + > link = calloc(sizeof(struct bpf_link *), prog_cnt); > prog = calloc(sizeof(struct bpf_program *), prog_cnt); > - if (CHECK(!link || !prog, "alloc_memory", "failed to alloc memory")) > + if (!ASSERT_OK_PTR(link, "link_ptr") || !ASSERT_OK_PTR(prog, "prog_ptr")) nit: can you split them into two independent ifs now? Just one extra `goto close_prog` is no big deal, but reads nicer > goto close_prog; > > obj = bpf_object__open_file(obj_file, &opts); > - if (CHECK(IS_ERR_OR_NULL(obj), "obj_open", > - "failed to open %s: %ld\n", obj_file, > - PTR_ERR(obj))) > + if (!ASSERT_OK_PTR(obj, "obj_open")) > goto close_prog; > > err = bpf_object__load(obj); > - if (CHECK(err, "obj_load", "err %d\n", err)) > + if (!ASSERT_OK(err, "obj_load")) > goto close_prog; > [...]
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Thu, Apr 8, 2021 at 12:57 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Extend the fexit_bpf2bpf test to check that the info for the bpf_link >> returned by the kernel matches the expected values. >> >> While we're updating the test, change existing uses of CHEC() to use the >> much easier to read ASSERT_*() macros. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- > > Just a minor nit below. Looks good, thanks. Right, will fix those and respin - thanks! :) -Toke
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Thu, Apr 8, 2021 at 12:57 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Extend the fexit_bpf2bpf test to check that the info for the bpf_link >> returned by the kernel matches the expected values. >> >> While we're updating the test, change existing uses of CHEC() to use the >> much easier to read ASSERT_*() macros. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- > > Just a minor nit below. Looks good, thanks. > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > >> .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 50 +++++++++++++++---- >> 1 file changed, 39 insertions(+), 11 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c >> index 5c0448910426..019a46d8e98e 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c >> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c >> @@ -57,11 +57,13 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, >> bool run_prog, >> test_cb cb) >> { >> + __u32 duration = 0, retval, tgt_prog_id, info_len; > > if not CHECK() is used, duration shouldn't be needed anymore Oh, and duration is still needed for bpf_prog_test_run(), so I'll keep that; but removing it did make the compiler point out that I missed one CHECK() at the beginning of the function when converting, so will fix that instead :) -Toke
Toke Høiland-Jørgensen <toke@redhat.com> writes: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> On Thu, Apr 8, 2021 at 12:57 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>> >>> Extend the fexit_bpf2bpf test to check that the info for the bpf_link >>> returned by the kernel matches the expected values. >>> >>> While we're updating the test, change existing uses of CHEC() to use the >>> much easier to read ASSERT_*() macros. >>> >>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >>> --- >> >> Just a minor nit below. Looks good, thanks. >> >> Acked-by: Andrii Nakryiko <andrii@kernel.org> >> >>> .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 50 +++++++++++++++---- >>> 1 file changed, 39 insertions(+), 11 deletions(-) >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c >>> index 5c0448910426..019a46d8e98e 100644 >>> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c >>> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c >>> @@ -57,11 +57,13 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, >>> bool run_prog, >>> test_cb cb) >>> { >>> + __u32 duration = 0, retval, tgt_prog_id, info_len; >> >> if not CHECK() is used, duration shouldn't be needed anymore > > Oh, and duration is still needed for bpf_prog_test_run(), so I'll keep > that; but removing it did make the compiler point out that I missed one > CHECK() at the beginning of the function when converting, so will fix > that instead :) Argh, no, bpf_prog_test_run() will accept a NULL pointer for duration; sorry for the noise! -Toke
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c index 5c0448910426..019a46d8e98e 100644 --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c @@ -57,11 +57,13 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, bool run_prog, test_cb cb) { + __u32 duration = 0, retval, tgt_prog_id, info_len; struct bpf_object *obj = NULL, *tgt_obj; + struct bpf_prog_info prog_info = {}; struct bpf_program **prog = NULL; struct bpf_link **link = NULL; - __u32 duration = 0, retval; int err, tgt_fd, i; + struct btf *btf; err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC, &tgt_obj, &tgt_fd); @@ -72,28 +74,55 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, .attach_prog_fd = tgt_fd, ); + info_len = sizeof(prog_info); + err = bpf_obj_get_info_by_fd(tgt_fd, &prog_info, &info_len); + if (!ASSERT_OK(err, "tgt_fd_get_info")) + goto close_prog; + + tgt_prog_id = prog_info.id; + btf = bpf_object__btf(tgt_obj); + link = calloc(sizeof(struct bpf_link *), prog_cnt); prog = calloc(sizeof(struct bpf_program *), prog_cnt); - if (CHECK(!link || !prog, "alloc_memory", "failed to alloc memory")) + if (!ASSERT_OK_PTR(link, "link_ptr") || !ASSERT_OK_PTR(prog, "prog_ptr")) goto close_prog; obj = bpf_object__open_file(obj_file, &opts); - if (CHECK(IS_ERR_OR_NULL(obj), "obj_open", - "failed to open %s: %ld\n", obj_file, - PTR_ERR(obj))) + if (!ASSERT_OK_PTR(obj, "obj_open")) goto close_prog; err = bpf_object__load(obj); - if (CHECK(err, "obj_load", "err %d\n", err)) + if (!ASSERT_OK(err, "obj_load")) goto close_prog; for (i = 0; i < prog_cnt; i++) { + struct bpf_link_info link_info; + char *tgt_name; + __s32 btf_id; + + tgt_name = strstr(prog_name[i], "/"); + if (!ASSERT_OK_PTR(tgt_name, "tgt_name")) + goto close_prog; + btf_id = btf__find_by_name_kind(btf, tgt_name + 1, BTF_KIND_FUNC); + prog[i] = bpf_object__find_program_by_title(obj, prog_name[i]); - if (CHECK(!prog[i], "find_prog", "prog %s not found\n", prog_name[i])) + if (!ASSERT_OK_PTR(prog[i], prog_name[i])) goto close_prog; + link[i] = bpf_program__attach_trace(prog[i]); - if (CHECK(IS_ERR(link[i]), "attach_trace", "failed to link\n")) + if (!ASSERT_OK_PTR(link[i], "attach_trace")) goto close_prog; + + info_len = sizeof(link_info); + memset(&link_info, 0, sizeof(link_info)); + err = bpf_obj_get_info_by_fd(bpf_link__fd(link[i]), + &link_info, &info_len); + ASSERT_OK(err, "link_fd_get_info"); + ASSERT_EQ(link_info.tracing.attach_type, + bpf_program__get_expected_attach_type(prog[i]), + "link_attach_type"); + ASSERT_EQ(link_info.tracing.target_obj_id, tgt_prog_id, "link_tgt_obj_id"); + ASSERT_EQ(link_info.tracing.target_btf_id, btf_id, "link_tgt_btf_id"); } if (cb) { @@ -107,9 +136,8 @@ static void test_fexit_bpf2bpf_common(const char *obj_file, err = bpf_prog_test_run(tgt_fd, 1, &pkt_v6, sizeof(pkt_v6), NULL, NULL, &retval, &duration); - CHECK(err || retval, "ipv6", - "err %d errno %d retval %d duration %d\n", - err, errno, retval, duration); + ASSERT_OK(err, "prog_run"); + ASSERT_EQ(retval, 0, "prog_run_ret"); if (check_data_map(obj, prog_cnt, false)) goto close_prog;
Extend the fexit_bpf2bpf test to check that the info for the bpf_link returned by the kernel matches the expected values. While we're updating the test, change existing uses of CHEC() to use the much easier to read ASSERT_*() macros. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 50 +++++++++++++++---- 1 file changed, 39 insertions(+), 11 deletions(-)