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).
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 :/.
Quentin
