Re: [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform BPF static linking

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

On Thu, Mar 11, 2021 at 3:31 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-03-09 20:04 UTC-0800 ~ Andrii Nakryiko <andrii@kernel.org>
> > Add `bpftool gen bpfo <output-file> <input_file>...` command to statically
> > link multiple BPF object files into a single output BPF object file.
> >
> > Similarly to existing '*.o' convention, bpftool is establishing a '*.bpfo'
> > convention for statically-linked BPF object files. Both .o and .bpfo suffixes
> > will be stripped out during BPF skeleton generation to infer BPF object name.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/bpf/bpftool/gen.c | 46 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 4033c46d83e7..8b1ed6c0a62f 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > +static int do_bpfo(int argc, char **argv)
>
> > +{
> > +     struct bpf_linker *linker;
> > +     const char *output_file, *file;
> > +     int err;
> > +
> > +     if (!REQ_ARGS(2)) {
> > +             usage();
> > +             return -1;
> > +     }
> > +
> > +     output_file = GET_ARG();
> > +
> > +     linker = bpf_linker__new(output_file, NULL);
> > +     if (!linker) {
> > +             p_err("failed to create BPF linker instance");
> > +             return -1;
> > +     }
> > +
> > +     while (argc) {
> > +             file = GET_ARG();
> > +
> > +             err = bpf_linker__add_file(linker, file);
> > +             if (err) {
> > +                     p_err("failed to link '%s': %d", file, err);
>
> I think you mentioned before that your preference was for having just
> the error code instead of using strerror(), but I think it would be more
> user-friendly for the majority of users who don't know the error codes
> if we had something more verbose? How about having both strerror()
> output and the error code?

Sure, I'll add strerror(). My earlier point was that those messages
are more often misleading (e.g., "file not found" for ENOENT or
something similar) than helpful. I should check if bpftool is passing
through warn-level messages from libbpf. Those are going to be very
helpful, if anything goes wrong. --verbose should pass through all of
libbpf messages, if it's not already the case.

>
> > +                     goto err_out;
> > +             }
> > +     }
> > +
> > +     err = bpf_linker__finalize(linker);
> > +     if (err) {
> > +             p_err("failed to finalize ELF file: %d", err);
> > +             goto err_out;
> > +     }
> > +
> > +     return 0;
> > +err_out:
> > +     bpf_linker__free(linker);
> > +     return -1;
>
> Should you call bpf_linker__free() even on success? I see that
> bpf_linker__finalize() frees some of the resources, but it seems that
> bpf_linker__free() does a more thorough job?

yep, it should really be just

err_out:
    bpf_linker__free(linker);
    return err;


>
> > +}
> > +
> >  static int do_help(int argc, char **argv)
> >  {
> >       if (json_output) {
> > @@ -611,6 +654,7 @@ static int do_help(int argc, char **argv)
> >
> >  static const struct cmd cmds[] = {
> >       { "skeleton",   do_skeleton },
> > +     { "bpfo",       do_bpfo },
> >       { "help",       do_help },
> >       { 0 }
> >  };
> >
>
> Please update the usage help message, man page, and bash completion,
> thanks. Especially because what "bpftool gen bpfo" does is not intuitive
> (but I don't have a better name suggestion at the moment).

Yeah, forgot about manpage and bash completions, as usual.

re: "gen bpfo". I don't have much better naming as well. `bpftool
link` is already taken for bpf_link-related commands. It felt like
keeping this under "gen" command makes sense. But maybe `bpftool
linker link <out> <in1> <in2> ...` would be a bit less confusing
convention?

>
> Great work!
>
> Quentin