| Message ID | 20221002151141.1074196-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Add missing bpf_iter_vma_offset__destroy call | expand |
On 10/2/22 8:11 AM, Jiri Olsa wrote: > Adding missing bpf_iter_vma_offset__destroy call to > test_task_vma_offset_common function and related goto jumps. > > Fixes: b3e1331eb925 ("selftests/bpf: Test parameterized task BPF iterators.") > Cc: Kui-Feng Lee <kuifeng@fb.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > index 3369c5ec3a17..462fe92e0736 100644 > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > @@ -1515,11 +1515,11 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > link = bpf_program__attach_iter(skel->progs.get_vma_offset, opts); Thanks for the fix. A nit. Instead of adding a new goto label. How about doing skel->links.get_vma_offset = bpf_program_attach_iter(...) and bpf_iter_vma_offset__destroy(skel) will take care of the link destroy. The earlier test_task_vma_common() is doing that also. Kui-Feng, please also take a look. > if (!ASSERT_OK_PTR(link, "attach_iter")) > - return; > + goto exit_skel; > > iter_fd = bpf_iter_create(bpf_link__fd(link)); > if (!ASSERT_GT(iter_fd, 0, "create_iter")) > - goto exit; > + goto exit_link; > > while ((len = read(iter_fd, buf, sizeof(buf))) > 0) > ; > @@ -1534,8 +1534,10 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > close(iter_fd); > > -exit: > +exit_link: > bpf_link__destroy(link); > +exit_skel: > + bpf_iter_vma_offset__destroy(skel); > } > > static void test_task_vma_offset(void)
On Mon, Oct 03, 2022 at 05:12:44PM -0700, Martin KaFai Lau wrote: > On 10/2/22 8:11 AM, Jiri Olsa wrote: > > Adding missing bpf_iter_vma_offset__destroy call to > > test_task_vma_offset_common function and related goto jumps. > > > > Fixes: b3e1331eb925 ("selftests/bpf: Test parameterized task BPF iterators.") > > Cc: Kui-Feng Lee <kuifeng@fb.com> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > index 3369c5ec3a17..462fe92e0736 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > @@ -1515,11 +1515,11 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > link = bpf_program__attach_iter(skel->progs.get_vma_offset, opts); > > Thanks for the fix. > > A nit. Instead of adding a new goto label. How about doing > > skel->links.get_vma_offset = bpf_program_attach_iter(...) > > and bpf_iter_vma_offset__destroy(skel) will take care of the link destroy. > The earlier test_task_vma_common() is doing that also. right, I forgot destroy would do that.. it'll be simpler change thanks, jirka > > Kui-Feng, please also take a look. > > > if (!ASSERT_OK_PTR(link, "attach_iter")) > > - return; > > + goto exit_skel; > > iter_fd = bpf_iter_create(bpf_link__fd(link)); > > if (!ASSERT_GT(iter_fd, 0, "create_iter")) > > - goto exit; > > + goto exit_link; > > while ((len = read(iter_fd, buf, sizeof(buf))) > 0) > > ; > > @@ -1534,8 +1534,10 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > close(iter_fd); > > -exit: > > +exit_link: > > bpf_link__destroy(link); > > +exit_skel: > > + bpf_iter_vma_offset__destroy(skel); > > } > > static void test_task_vma_offset(void) >
On Tue, Oct 04, 2022 at 08:38:23AM +0200, Jiri Olsa wrote: > On Mon, Oct 03, 2022 at 05:12:44PM -0700, Martin KaFai Lau wrote: > > On 10/2/22 8:11 AM, Jiri Olsa wrote: > > > Adding missing bpf_iter_vma_offset__destroy call to > > > test_task_vma_offset_common function and related goto jumps. > > > > > > Fixes: b3e1331eb925 ("selftests/bpf: Test parameterized task BPF iterators.") > > > Cc: Kui-Feng Lee <kuifeng@fb.com> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > index 3369c5ec3a17..462fe92e0736 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > @@ -1515,11 +1515,11 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > > link = bpf_program__attach_iter(skel->progs.get_vma_offset, opts); > > > > Thanks for the fix. > > > > A nit. Instead of adding a new goto label. How about doing > > > > skel->links.get_vma_offset = bpf_program_attach_iter(...) > > > > and bpf_iter_vma_offset__destroy(skel) will take care of the link destroy. > > The earlier test_task_vma_common() is doing that also. > > right, I forgot destroy would do that.. it'll be simpler change ugh actually no ;-) it's outside (of skeleton) link, so it won't get closed in bpf_iter_vma_offset__destroy the earlier test_task_vma_common does not create such link jirka > > thanks, > jirka > > > > > Kui-Feng, please also take a look. > > > > > if (!ASSERT_OK_PTR(link, "attach_iter")) > > > - return; > > > + goto exit_skel; > > > iter_fd = bpf_iter_create(bpf_link__fd(link)); > > > if (!ASSERT_GT(iter_fd, 0, "create_iter")) > > > - goto exit; > > > + goto exit_link; > > > while ((len = read(iter_fd, buf, sizeof(buf))) > 0) > > > ; > > > @@ -1534,8 +1534,10 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > > close(iter_fd); > > > -exit: > > > +exit_link: > > > bpf_link__destroy(link); > > > +exit_skel: > > > + bpf_iter_vma_offset__destroy(skel); > > > } > > > static void test_task_vma_offset(void) > >
On Tue, Oct 4, 2022 at 5:16 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Tue, Oct 04, 2022 at 08:38:23AM +0200, Jiri Olsa wrote: > > On Mon, Oct 03, 2022 at 05:12:44PM -0700, Martin KaFai Lau wrote: > > > On 10/2/22 8:11 AM, Jiri Olsa wrote: > > > > Adding missing bpf_iter_vma_offset__destroy call to > > > > test_task_vma_offset_common function and related goto jumps. > > > > > > > > Fixes: b3e1331eb925 ("selftests/bpf: Test parameterized task BPF iterators.") > > > > Cc: Kui-Feng Lee <kuifeng@fb.com> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > > tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > > index 3369c5ec3a17..462fe92e0736 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > > @@ -1515,11 +1515,11 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > > > link = bpf_program__attach_iter(skel->progs.get_vma_offset, opts); > > > > > > Thanks for the fix. > > > > > > A nit. Instead of adding a new goto label. How about doing > > > > > > skel->links.get_vma_offset = bpf_program_attach_iter(...) > > > > > > and bpf_iter_vma_offset__destroy(skel) will take care of the link destroy. > > > The earlier test_task_vma_common() is doing that also. > > > > right, I forgot destroy would do that.. it'll be simpler change > > ugh actually no ;-) it's outside (of skeleton) link, > so it won't get closed in bpf_iter_vma_offset__destroy Martin's point was that if you assign it to skel->links.get_vma_offset it will be closed by skeleton's destroy method. So let's do that? > > the earlier test_task_vma_common does not create such link > > jirka > > > > > thanks, > > jirka > > > > > > > > Kui-Feng, please also take a look. > > > > > > > if (!ASSERT_OK_PTR(link, "attach_iter")) > > > > - return; > > > > + goto exit_skel; > > > > iter_fd = bpf_iter_create(bpf_link__fd(link)); > > > > if (!ASSERT_GT(iter_fd, 0, "create_iter")) > > > > - goto exit; > > > > + goto exit_link; > > > > while ((len = read(iter_fd, buf, sizeof(buf))) > 0) > > > > ; > > > > @@ -1534,8 +1534,10 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > > > close(iter_fd); > > > > -exit: > > > > +exit_link: > > > > bpf_link__destroy(link); > > > > +exit_skel: > > > > + bpf_iter_vma_offset__destroy(skel); > > > > } > > > > static void test_task_vma_offset(void) > > >
On Wed, Oct 05, 2022 at 03:58:45PM -0700, Andrii Nakryiko wrote: > On Tue, Oct 4, 2022 at 5:16 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Tue, Oct 04, 2022 at 08:38:23AM +0200, Jiri Olsa wrote: > > > On Mon, Oct 03, 2022 at 05:12:44PM -0700, Martin KaFai Lau wrote: > > > > On 10/2/22 8:11 AM, Jiri Olsa wrote: > > > > > Adding missing bpf_iter_vma_offset__destroy call to > > > > > test_task_vma_offset_common function and related goto jumps. > > > > > > > > > > Fixes: b3e1331eb925 ("selftests/bpf: Test parameterized task BPF iterators.") > > > > > Cc: Kui-Feng Lee <kuifeng@fb.com> > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > > --- > > > > > tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 8 +++++--- > > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > > > index 3369c5ec3a17..462fe92e0736 100644 > > > > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c > > > > > @@ -1515,11 +1515,11 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool > > > > > link = bpf_program__attach_iter(skel->progs.get_vma_offset, opts); > > > > > > > > Thanks for the fix. > > > > > > > > A nit. Instead of adding a new goto label. How about doing > > > > > > > > skel->links.get_vma_offset = bpf_program_attach_iter(...) > > > > > > > > and bpf_iter_vma_offset__destroy(skel) will take care of the link destroy. > > > > The earlier test_task_vma_common() is doing that also. > > > > > > right, I forgot destroy would do that.. it'll be simpler change > > > > ugh actually no ;-) it's outside (of skeleton) link, > > so it won't get closed in bpf_iter_vma_offset__destroy > > Martin's point was that if you assign it to skel->links.get_vma_offset > it will be closed by skeleton's destroy method. So let's do that? ah ok, will send new version thanks, jirka
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c index 3369c5ec3a17..462fe92e0736 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c @@ -1515,11 +1515,11 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool link = bpf_program__attach_iter(skel->progs.get_vma_offset, opts); if (!ASSERT_OK_PTR(link, "attach_iter")) - return; + goto exit_skel; iter_fd = bpf_iter_create(bpf_link__fd(link)); if (!ASSERT_GT(iter_fd, 0, "create_iter")) - goto exit; + goto exit_link; while ((len = read(iter_fd, buf, sizeof(buf))) > 0) ; @@ -1534,8 +1534,10 @@ static void test_task_vma_offset_common(struct bpf_iter_attach_opts *opts, bool close(iter_fd); -exit: +exit_link: bpf_link__destroy(link); +exit_skel: + bpf_iter_vma_offset__destroy(skel); } static void test_task_vma_offset(void)
Adding missing bpf_iter_vma_offset__destroy call to test_task_vma_offset_common function and related goto jumps. Fixes: b3e1331eb925 ("selftests/bpf: Test parameterized task BPF iterators.") Cc: Kui-Feng Lee <kuifeng@fb.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)