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

On Thu, Mar 11, 2021 at 3:31 AM Quentin Monnet [off-list ref] wrote:

2021-03-09 20:04 UTC-0800 ~ Andrii Nakryiko [off-list ref]

quoted

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)

quoted

+{
+     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.

quoted

+                     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;

quoted

+}
+
 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