From: Quentin Monnet <quentin@isovalent.com>
To: Andrii Nakryiko <andrii@kernel.org>,
bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
daniel@iogearbox.net
Cc: kernel-team@fb.com
Subject: Re: [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform BPF static linking
Date: Thu, 11 Mar 2021 11:31:31 +0000 [thread overview]
Message-ID: <9f44eedf-79a3-0025-0f31-ee70f2f7d98b@isovalent.com> (raw)
In-Reply-To: <20210310040431.916483-8-andrii@kernel.org>
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
next prev parent reply other threads:[~2021-03-11 11:32 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-10 4:04 [PATCH bpf-next 00/10] BPF static linking Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 01/10] libbpf: expose btf_type_by_id() internally Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 02/10] libbpf: add internal helper to get raw BTF strings section Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 03/10] libbpf: generalize BTF and BTF.ext type ID and strings iteration Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 04/10] libbpf: add generic BTF type shallow copy API Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 05/10] libbpf: add BPF static linker APIs Andrii Nakryiko
2021-03-11 2:34 ` Alexei Starovoitov
2021-03-11 3:29 ` Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 06/10] libbpf: add BPF static linker BTF and BTF.ext support Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform BPF static linking Andrii Nakryiko
2021-03-11 11:31 ` Quentin Monnet [this message]
2021-03-11 18:45 ` Andrii Nakryiko
2021-03-12 18:07 ` Quentin Monnet
2021-03-13 18:37 ` Andrii Nakryiko
2021-03-13 21:27 ` Quentin Monnet
2021-03-10 4:04 ` [PATCH bpf-next 08/10] selftests/bpf: re-generate vmlinux.h and BPF skeletons if bpftool changed Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 09/10] selftests/bpf: pass all BPF .o's through BPF static linker Andrii Nakryiko
2021-03-10 4:04 ` [PATCH bpf-next 10/10] selftests/bpf: add multi-file statically linked BPF object file test Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9f44eedf-79a3-0025-0f31-ee70f2f7d98b@isovalent.com \
--to=quentin@isovalent.com \
--cc=andrii@kernel.org \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).