Message-ID: <CAEf4BzZGYdTVWf3dp6FvBu+ogd491CXky5v708OzQG8oyYoCOQ@mail.gmail.com>
Date:   Sat, 13 Mar 2021 10:37:21 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform
 BPF static linking

On Fri, Mar 12, 2021 at 10:07 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2021-03-11 10:45 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> > On Thu, Mar 11, 2021 at 3:31 AM Quentin Monnet <quentin@...valent.com> wrote:
> >>
> >> 2021-03-09 20:04 UTC-0800 ~ Andrii Nakryiko <andrii@...nel.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@...nel.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.
>
> Thanks. Yes, --verbose should do it, but it's worth a double-check.
>
> >>> +                     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?
>
> "bpftool linker" would have been nice, but having "bpftool link", I
> think it would be even more confusing. We can pass commands by their
> prefixes, so is "bpftool link" the command "link" or a prefix for
> "linker"? (I know it would be easy to sort out from our point of view,
> but for regular users I'm sure that would be confusing).

right

>
> I don't mind leaving it under "bpftool gen", it's probably the most
> relevant command we have. As for replacing the "bpfo" keyword, I've
> thought of "combined", "static_linked", "archive", "concat". I write
> them in case it's any inspiration, but I find none of them ideal :/.

How about "bpftool gen object", which can be shortened in typing to
just `bpftool gen obj`. It seems complementary to `gen skeleton`. You
first generate object (from other objects generated by compiler, which
might be a bit confusing at first), then you generate skeleton from
the object. WDYT?

>
> Quentin

Powered by blists - more mailing lists