On Sat, Sep 19, 2020 at 4:51 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>
> This adds support for supplying a target btf ID for the bpf_link_create()
> operation, and adds a new bpf_program__attach_freplace() high-level API for
> attaching freplace functions with a target.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
> ---
> tools/lib/bpf/bpf.c | 18 +++++++++++++++---
> tools/lib/bpf/bpf.h | 3 ++-
> tools/lib/bpf/libbpf.c | 36 +++++++++++++++++++++++++++++++-----
> tools/lib/bpf/libbpf.h | 3 +++
> tools/lib/bpf/libbpf.map | 1 +
> 5 files changed, 52 insertions(+), 9 deletions(-)
>
Looks good, but let's add meaningful error messages, especially that
it's so trivial to do and obvious what exactly is wrong.
> +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
> + int target_fd,
> + const char *attach_func_name)
> +{
> + int btf_id;
> +
> + if (!!target_fd != !!attach_func_name || prog->type != BPF_PROG_TYPE_EXT)
> + return ERR_PTR(-EINVAL);
Can you please split those two and have separate error messages for
both, following libbpf's format, e.g.,:
prog '%s': should be freplace program type
and
prog '%s': either both or none of target FD and target function name
should be provided
> +
> + if (target_fd) {
> +
nit: why empty line?
> + btf_id = libbpf_find_prog_btf_id(attach_func_name, target_fd);
> + if (btf_id < 0)
> + return ERR_PTR(btf_id);
> +
> + return bpf_program__attach_fd(prog, target_fd, btf_id, "freplace");
> + } else {
> + /* no target, so use raw_tracepoint_open for compatibility
> + * with old kernels
> + */
> + return bpf_program__attach_trace(prog);
> + }
> }
>
[...]
