On Fri, Jun 23, 2023 at 01:40:08PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 20, 2023 at 1:37 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > Adding bpf_program__attach_uprobe_multi_opts function that
> > allows to attach multiple uprobes with uprobe_multi link.
> >
> > The user can specify uprobes with direct arguments:
> >
> > binary_path/func_pattern
> >
> > or with struct bpf_uprobe_multi_opts opts argument fields:
> >
> > const char *path;
> > const char **syms;
> > const unsigned long *offsets;
> > const unsigned long *ref_ctr_offsets;
> >
> > User can specify 3 mutually exclusive set of incputs:
>
> typo: inputs
>
> >
> > 1) use only binary_path/func_pattern aruments
>
> typo: arguments
ok
>
> >
> > 2) use only opts argument with allowed combinations of:
> > path/offsets/ref_ctr_offsets/cookies/cnt
> >
> > 3) use binary_path with allowed combinations of:
> > syms/offsets/ref_ctr_offsets/cookies/cnt
> >
>
> why do we need this duplication of binary_path and opts.path? same for
> pid and opts.pid?
right, will remove opts.path|pid
>
> > Any other usage results in error.
> >
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/libbpf.c | 131 +++++++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 31 +++++++++
> > tools/lib/bpf/libbpf.map | 1 +
> > 3 files changed, 163 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 3e5c88caf5d5..d972cea4c658 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -11402,6 +11402,137 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
> > return -ENOENT;
> > }
> >
> > +struct bpf_link *
> > +bpf_program__attach_uprobe_multi_opts(const struct bpf_program *prog,
>
> let's call it just bpf_program__attach_uprobe_multi()
>
> we should have done that with bpf_program__attach_kprobe_multi_opts()
> as well. Generally, if the only variant of API is the one with opts,
> then there is no point in adding "_opts" suffix to the API name, IMO
ok
>
>
> > + pid_t pid,
> > + const char *binary_path,
> > + const char *func_pattern,
> > + const struct bpf_uprobe_multi_opts *opts)
> > +{
> > + const unsigned long *ref_ctr_offsets = NULL, *offsets = NULL;
> > + LIBBPF_OPTS(bpf_link_create_opts, lopts);
> > + unsigned long *resolved_offsets = NULL;
> > + const char **resolved_symbols = NULL;
> > + int err = 0, link_fd, prog_fd;
> > + struct bpf_link *link = NULL;
> > + char errmsg[STRERR_BUFSIZE];
> > + const char *path, **syms;
> > + char full_path[PATH_MAX];
> > + const __u64 *cookies;
> > + bool has_pattern;
> > + bool retprobe;
> > + size_t cnt;
> > +
> > + if (!OPTS_VALID(opts, bpf_uprobe_multi_opts))
> > + return libbpf_err_ptr(-EINVAL);
> > +
> > + path = OPTS_GET(opts, path, NULL);
> > + syms = OPTS_GET(opts, syms, NULL);
> > + offsets = OPTS_GET(opts, offsets, NULL);
> > + ref_ctr_offsets = OPTS_GET(opts, ref_ctr_offsets, NULL);
> > + cookies = OPTS_GET(opts, cookies, NULL);
> > + cnt = OPTS_GET(opts, cnt, 0);
> > +
> > + /*
> > + * User can specify 3 mutually exclusive set of incputs:
> > + *
> > + * 1) use only binary_path/func_pattern aruments
>
> same typos
>
> > + *
> > + * 2) use only opts argument with allowed combinations of:
> > + * path/offsets/ref_ctr_offsets/cookies/cnt
> > + *
> > + * 3) use binary_path with allowed combinations of:
> > + * syms/offsets/ref_ctr_offsets/cookies/cnt
> > + *
> > + * Any other usage results in error.
> > + */
> > +
>
>
> Looking at this part (sorry, I already trimmed reply before realizing
> I want to comment on this):
>
>
> + err = elf_find_pattern_func_offset(binary_path, func_pattern,
> + &resolved_symbols,
> &resolved_offsets,
> + &cnt);
> + if (err < 0)
> + return libbpf_err_ptr(err);
> + offsets = resolved_offsets;
> + } else if (syms) {
> + err = elf_find_multi_func_offset(binary_path, cnt,
> syms, &resolved_offsets);
>
> Few thoughts.
>
> First, we are not using resolved_symbols returned from
> elf_find_pattern_func_offset(), do we? If not, let's drop it.
I think I used that in bpftrace with the RFC version,
where I wanted to display found symbols.. but there's
no use for it in libbpf for now.. will remove it
>
> Second, I'm not a big fan of chosen naming. Maybe something like
> elf_resolve_{pattern,syms}_offsets?
sure
SNIP
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_uprobe_multi_opts(const struct bpf_program *prog,
> > + pid_t pid,
> > + const char *binary_path,
> > + const char *func_pattern,
> > + const struct bpf_uprobe_multi_opts *opts);
> > +
> > struct bpf_ksyscall_opts {
> > /* size of this struct, for forward/backward compatibility */
> > size_t sz;
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 7521a2fb7626..81558ef1bc38 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -390,6 +390,7 @@ LIBBPF_1.2.0 {
> > bpf_link_get_info_by_fd;
> > bpf_map_get_info_by_fd;
> > bpf_prog_get_info_by_fd;
> > + bpf_program__attach_uprobe_multi_opts;
>
> should be in 1.3.0?
yes, will fix
thanks,
jirka
