| Message ID | 20210310040431.916483-8-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | BPF static linking | expand |
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? > + 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? > +} > + > 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). Great work! Quentin
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
2021-03-11 10:45 UTC-0800 ~ 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. 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
On Fri, Mar 12, 2021 at 10:07 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2021-03-11 10:45 UTC-0800 ~ 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. > > 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
On Sat, 13 Mar 2021 at 18:37, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Mar 12, 2021 at 10:07 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > 2021-03-11 10:45 UTC-0800 ~ 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 > > >>> @@ -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? Sounds good, better than "bpfo" I think. Quentin
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 @@ -54,7 +54,9 @@ static void get_obj_name(char *name, const char *file) strncpy(name, basename(file), MAX_OBJ_NAME_LEN - 1); name[MAX_OBJ_NAME_LEN - 1] = '\0'; if (str_has_suffix(name, ".o")) - name[strlen(name) - 2] = '\0'; + name[strlen(name) - sizeof(".o") + 1] = '\0'; + else if (str_has_suffix(name, ".bpfo")) + name[strlen(name) - sizeof(".bpfo") + 1] = '\0'; sanitize_identifier(name); } @@ -591,6 +593,47 @@ static int do_skeleton(int argc, char **argv) return err; } +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); + 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; +} + 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 } };
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(-)