Re: [PATCH bpf-next] selftests/bpf: Add missing bpf_iter_vma_offset__destroy call

[Jiri Olsa <olsajiri@gmail.com>]

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