Re: [PATCHv4 bpf-next 4/4] selftests/bpf: Add bpf_vma_build_id_parse task vma iterator test

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Nov 28, 2022 at 11:21:30AM -0800, Hao Luo wrote:
> On Mon, Nov 28, 2022 at 5:30 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding tests for using new bpf_vma_build_id_parse kfunc in task_vma
> > iterator program.
> >
> > On bpf program side the iterator filters test proccess and proper
> > vma by provided function pointer and reads its build id with the
> > new kfunc.
> >
> > On user side the test uses readelf to get test_progs build id and
> > compares it with the one read from iterator.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/bpf_iter.c       | 44 +++++++++++++++++++
> >  .../selftests/bpf/progs/bpf_iter_build_id.c   | 41 +++++++++++++++++
> >  2 files changed, 85 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_build_id.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > index 6f8ed61fc4b4..b2cad9f70b32 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > @@ -33,6 +33,9 @@
> <...>
> >
> > +static void test_task_vma_build_id(void)
> > +{
> > +       struct bpf_iter_build_id *skel;
> > +       char buf[BUILDID_STR_SIZE] = {};
> 
> Jiri, do you mean buf[BUILDID_STR_SIZE + 1]? I see you have
> buf[BUILDID_STR_SIZE] = 0; below.

ugh, that plus one ended up in the BUILDID_STR_SIZE define, will fix

> 
> > +       int iter_fd, len;
> > +       char *build_id;
> <...>
> > +
> > +       while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> > +               ;
> 
> I think you need to pass 'buf + len' to read(), otherwise the last
> iteration will overwrite the content read from the previous
> iterations.

ok

> 
> > +       buf[BUILDID_STR_SIZE] = 0;
> > +
> > +       /* Read build_id via readelf to compare with iterator buf. */
> > +       if (!ASSERT_OK(read_self_buildid(&build_id), "read_buildid"))
> > +               goto exit;
> 
> We need to close iter_fd before going to exit.

right, will fix

thanks,
jirka

> 
> > +
> > +       ASSERT_STREQ(buf, build_id, "build_id_match");
> > +       ASSERT_GT(skel->data->size, 0, "size");
> > +
> > +       free(build_id);
> > +       close(iter_fd);
> > +exit:
> > +       bpf_iter_build_id__destroy(skel);
> > +}
> > +
> <...>
> > --
> > 2.38.1
> >