Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Sep 10, 2020 at 03:34:26PM -0700, Andrii Nakryiko wrote:

SNIP

> > +
> > +void test_trace_ext(void)
> > +{
> > +       struct test_trace_ext_tracing *skel_trace = NULL;
> > +       struct test_trace_ext_tracing__bss *bss_trace;
> > +       const char *file = "./test_pkt_md_access.o";
> > +       struct test_trace_ext *skel_ext = NULL;
> > +       struct test_trace_ext__bss *bss_ext;
> > +       int err, prog_fd, ext_fd;
> > +       struct bpf_object *obj;
> > +       char buf[100];
> > +       __u32 retval;
> > +       __u64 len;
> > +
> > +       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > +       if (CHECK_FAIL(err))
> > +               return;
> 
> We should avoid using bpf_prog_load() for new code. Can you please
> just skeleton instead? Or at least bpf_object__open_file()?

ok

> 
> > +
> > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > +                           .attach_prog_fd = prog_fd,
> > +       );
> 
> DECLARE_LIBBPF_OPTS does declare a variable, so should be together
> with all the other variables above, otherwise some overly strict C89
> mode compiler will start complaining. You can assign
> `opts.attach_prog_fd = prog_fd;` outside of declaration. But I also
> don't think you need this one. Having .attach_prog_fd in open_opts is
> not great, because it's a per-program setting specified at bpf_object
> level. Would bpf_program__set_attach_target() work here?

right, I'll try it, it should be enough

SNIP

> > +
> > +cleanup:
> > +       test_trace_ext__destroy(skel_ext);
> > +       bpf_object__close(obj);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> > new file mode 100644
> > index 000000000000..a6318f6b52ee
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> > @@ -0,0 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019 Facebook
> > +#include <linux/bpf.h>
> > +#include <stdbool.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_endian.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +volatile __u64 ext_called = 0;
> 
> nit: no need for volatile, global variables are not going anywhere;
> same below in two places

ok, thanks

jirka