On Fri, Mar 12, 2021 at 10:07 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> 2021-03-11 10:45 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> > On Thu, Mar 11, 2021 at 3:31 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
> >>
> >> 2021-03-09 20:04 UTC-0800 ~ Andrii Nakryiko <andrii@xxxxxxxxxx>
> >>> 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@xxxxxxxxxx>
> >>> ---
> >>> 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
