Re: [PATCH bpf-next v3 3/3] Add documentation to API functions

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Apr 19, 2022 at 9:04 AM grantseltzer <grantseltzer@gmail.com> wrote:
>
> From: Grant Seltzer <grantseltzer@gmail.com>
>
> This adds documentation for the following API functions:
> - bpf_program__set_expected_attach_type()
> - bpf_program__set_type()
> - bpf_program__set_attach_target()
> - bpf_program__attach()
> - bpf_program__pin()
> - bpf_program__unpin()
>
> Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
> ---
>  tools/lib/bpf/libbpf.h | 100 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 2 deletions(-)
>

[...]

>  LIBBPF_API enum bpf_attach_type
>  bpf_program__expected_attach_type(const struct bpf_program *prog);
> -LIBBPF_API void
> +/**
> + * @brief **bpf_program__set_expected_attach_type()** sets the
> + * attach type of the passed BPF program. This is used for
> + * auto-detection of attachment when programs are loaded.
> + * @param prog BPF program to set the attach type for
> + * @param type attach type to set the BPF map to have
> + * @return error code; or 0 if no error. An error occurs
> + * if the object is already loaded.
> + *
> + * An example workflow:
> + *
> + * ...
> + *   xdp_fd = bpf_prog_get_fd_by_id(id);
> + *   trace_obj = bpf_object__open_file("func.o", NULL);
> + *   prog = bpf_object__find_program_by_title(trace_obj, "fentry/myfunc");


bpf_object__find_program_by_title() is deprecated, we shouldn't use it
in documentation. There is a lot going on in this example workflow
(and workflow itself is using generic API instead of recommended
skeleton). Let's drop it, it might just be adding to confusion.

Just mention that expected attach type has to be set before BPF object
is loaded (same for program type). And that should be enough. In most
cases user won't ever have to use this anyways, IMO.

> + *   int err = bpf_program__set_expected_attach_type(prog, BPF_TRACE_FENTRY);
> + *   if (err != 0) {
> + *     // Object already loaded
> + *   }
> + *   bpf_program__set_attach_target(prog, xdp_fd, "xdpfilt_blk_all");
> + *   bpf_object__load(trace_obj);
> + * ...
> + */
> +LIBBPF_API int
>  bpf_program__set_expected_attach_type(struct bpf_program *prog,
>                                       enum bpf_attach_type type);
>
> @@ -707,6 +780,29 @@ LIBBPF_API int bpf_program__set_log_level(struct bpf_program *prog, __u32 log_le
>  LIBBPF_API const char *bpf_program__log_buf(const struct bpf_program *prog, size_t *log_size);
>  LIBBPF_API int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log_size);
>
> +/**
> + * @brief **bpf_program__set_attach_target()** sets the
> + * specified BPF program to attach to a specific tracepoint
> + * or kernel function. This can be used to supplement
> + * the BPF program name/section not matching the tracepoint
> + * or function semanics.

Not sure what you wanted to say with the last sentence?

How about something along the lines:

"... sets BTF-based attach target for supported BPF program types:
BTF-aware raw tracepoints, fentry/fexit/fmod_ret, lsm, freplace" ?


> + * @param prog BPF program to set the attach type for
> + * @param type attach type to set the BPF map to have
> + * @return error code; or 0 if no error occurred.
> + * An example workflow:
> + *
> + * ...
> + *   xdp_fd = bpf_prog_get_fd_by_id(id);
> + *   trace_obj = bpf_object__open_file("func.o", NULL);
> + *   prog = bpf_object__find_program_by_title(trace_obj, "fentry/myfunc");

same about find_program_by_title, please don't use it; and same about
the overall example, it's not succinct enough to fit in a doc comment,
let's just drop it?

> + *   bpf_program__set_expected_attach_type(prog, BPF_TRACE_FENTRY);
> + *   int err = bpf_program__set_attach_target(prog, xdp_fd, "xdpfilt_blk_all");
> + *   if (err != 0) {
> + *     // Object already loaded
> + *   }
> + *   bpf_object__load(trace_obj);

like here, we don't check error, setting a bad example :(

> + * ...
> + */
>  LIBBPF_API int
>  bpf_program__set_attach_target(struct bpf_program *prog, int attach_prog_fd,
>                                const char *attach_func_name);
> --
> 2.34.1
>