On Wed, Oct 30, 2019 at 12:36 PM Alexei Starovoitov <ast@xxxxxxxxxx> wrote:
>
> Cleanup libbpf from expected_attach_type == attach_btf_id hack
> and introduce BPF_PROG_TYPE_TRACING.
>
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
Looks good overall, but please replace all u32 to __u32 and add proper
LIBBPF_APIs for all the new exposed functions from libbpf.h, with
those changes:
Acked-by: Andrii Nakryiko <andriin@xxxxxx>
> tools/include/uapi/linux/bpf.h | 2 +
> tools/lib/bpf/bpf.c | 8 ++--
> tools/lib/bpf/bpf.h | 5 +-
> tools/lib/bpf/libbpf.c | 88 +++++++++++++++++++++++++---------
> tools/lib/bpf/libbpf.h | 4 ++
> tools/lib/bpf/libbpf_probes.c | 1 +
> 6 files changed, 80 insertions(+), 28 deletions(-)
>
[...]
>
> +static int libbpf_attach_btf_id_by_name(const char *name, u32 *btf_id);
here and in few places below, u32 will break libbpf on Github, please use __u32
> +
> static struct bpf_object *
> __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> struct bpf_object_open_opts *opts)
> @@ -3656,6 +3660,7 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> bpf_object__for_each_program(prog, obj) {
> enum bpf_prog_type prog_type;
> enum bpf_attach_type attach_type;
> + u32 btf_id;
>
> err = libbpf_prog_type_by_name(prog->section_name, &prog_type,
> &attach_type);
> @@ -3667,6 +3672,12 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>
> bpf_program__set_type(prog, prog_type);
> bpf_program__set_expected_attach_type(prog, attach_type);
> + if (prog_type == BPF_PROG_TYPE_TRACING) {
> + err = libbpf_attach_btf_id_by_name(prog->section_name, &btf_id);
> + if (err)
> + goto out;
> + bpf_program__set_attach_btf_id(prog, btf_id);
> + }
> }
>
> return obj;
[...]
>
> +#define BTF_PREFIX "btf_trace_"
> +static int libbpf_attach_btf_id_by_name(const char *name, u32 *btf_id)
> +{
> + struct btf *btf = bpf_core_find_kernel_btf();
> + char raw_tp_btf_name[128] = BTF_PREFIX;
> + char *dst = raw_tp_btf_name + sizeof(BTF_PREFIX) - 1;
> + int ret, i, err;
> +
> + if (IS_ERR(btf)) {
> + pr_warn("vmlinux BTF is not found\n");
> + return -EINVAL;
> + }
> +
> + if (!name) {
> + err = -EINVAL;
> + goto err;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> + if (!section_names[i].is_attach_btf)
> + continue;
> + if (strncmp(name, section_names[i].sec, section_names[i].len))
> + continue;
> + /* prepend "btf_trace_" prefix per kernel convention */
> + strncat(dst, name + section_names[i].len,
> + sizeof(raw_tp_btf_name) - sizeof(BTF_PREFIX));
> + ret = btf__find_by_name(btf, raw_tp_btf_name);
> + if (ret <= 0) {
> + pr_warn("%s is not found in vmlinux BTF\n", dst);
> + err = -EINVAL;
> + goto err;
> + }
> + *btf_id = ret;
> + err = 0;
nit: I'd just initialize err to zero, it will be easy to miss this if
we need to extend this function a bit.
> + goto err;
> + }
> + pr_warn("failed to identify btf_id based on ELF section name '%s'\n", name);
> + err = -ESRCH;
> +err:
err is misleading, it's just exit, really
> + btf__free(btf);
> + return err;
> +}
> +
> int libbpf_attach_type_by_name(const char *name,
> enum bpf_attach_type *attach_type)
> {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index c63e2ff84abc..a3f5b8d3398d 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -307,6 +307,7 @@ LIBBPF_API int bpf_program__set_sched_cls(struct bpf_program *prog);
> LIBBPF_API int bpf_program__set_sched_act(struct bpf_program *prog);
> LIBBPF_API int bpf_program__set_xdp(struct bpf_program *prog);
> LIBBPF_API int bpf_program__set_perf_event(struct bpf_program *prog);
> +int bpf_program__set_tracing(struct bpf_program *prog);
LIBBPF_API? same few places below
>
> LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
> LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> @@ -317,6 +318,8 @@ bpf_program__get_expected_attach_type(struct bpf_program *prog);
> LIBBPF_API void
> bpf_program__set_expected_attach_type(struct bpf_program *prog,
> enum bpf_attach_type type);
> +void
> +bpf_program__set_attach_btf_id(struct bpf_program *prog, u32 btf_id);
>
[...]
