From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
kernel-team@fb.com
Subject: Re: [PATCH bpf-next 07/10] libbpf: refactor CO-RE relo human description formatting routine
Date: Tue, 26 Apr 2022 11:52:48 -0700 [thread overview]
Message-ID: <20220426185248.ogbjc7f2rwfzhxqs@MacBook-Pro.local> (raw)
In-Reply-To: <20220426004511.2691730-8-andrii@kernel.org>
On Mon, Apr 25, 2022 at 05:45:08PM -0700, Andrii Nakryiko wrote:
> Refactor how CO-RE relocation is formatted. Now it dumps human-readable
> representation, currently used by libbpf in either debug or error
> message output during CO-RE relocation resolution process, into provided
> buffer. This approach allows for better reuse of this functionality
> outside of CO-RE relocation resolution, which we'll use in next patch
> for providing better error message for BPF verifier rejecting BPF
> program due to unguarded failed CO-RE relocation.
>
> It also gets rid of annoying "stitching" of libbpf_print() calls, which
> was the only place where we did this.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> tools/lib/bpf/relo_core.c | 64 +++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> index adaa22160692..13d36a705464 100644
> --- a/tools/lib/bpf/relo_core.c
> +++ b/tools/lib/bpf/relo_core.c
> @@ -1055,51 +1055,66 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
> * [<type-id>] (<type-name>) + <raw-spec> => <offset>@<spec>,
> * where <spec> is a C-syntax view of recorded field access, e.g.: x.a[3].b
> */
> -static void bpf_core_dump_spec(const char *prog_name, int level, const struct bpf_core_spec *spec)
> +static int bpf_core_format_spec(char *buf, size_t buf_sz, const struct bpf_core_spec *spec)
> {
> const struct btf_type *t;
> const struct btf_enum *e;
> const char *s;
> __u32 type_id;
> - int i;
> + int i, len = 0;
> +
> +#define append_buf(fmt, args...) \
> + ({ \
> + int r; \
> + r = snprintf(buf, buf_sz, fmt, ##args); \
> + len += r; \
> + if (r >= buf_sz) \
Do we need to check for r<0 here too or it's highly unlikely?
> + r = buf_sz; \
> + buf += r; \
> + buf_sz -= r; \
> + })
>
> type_id = spec->root_type_id;
> t = btf_type_by_id(spec->btf, type_id);
> s = btf__name_by_offset(spec->btf, t->name_off);
>
> - libbpf_print(level, "[%u] %s %s", type_id, btf_kind_str(t), str_is_empty(s) ? "<anon>" : s);
> + append_buf("<%s> [%u] %s %s",
> + core_relo_kind_str(spec->relo_kind),
> + type_id, btf_kind_str(t), str_is_empty(s) ? "<anon>" : s);
>
> if (core_relo_is_type_based(spec->relo_kind))
> - return;
> + return len;
>
> if (core_relo_is_enumval_based(spec->relo_kind)) {
> t = skip_mods_and_typedefs(spec->btf, type_id, NULL);
> e = btf_enum(t) + spec->raw_spec[0];
> s = btf__name_by_offset(spec->btf, e->name_off);
>
> - libbpf_print(level, "::%s = %u", s, e->val);
> - return;
> + append_buf("::%s = %u", s, e->val);
> + return len;
> }
>
> if (core_relo_is_field_based(spec->relo_kind)) {
> for (i = 0; i < spec->len; i++) {
> if (spec->spec[i].name)
> - libbpf_print(level, ".%s", spec->spec[i].name);
> + append_buf(".%s", spec->spec[i].name);
> else if (i > 0 || spec->spec[i].idx > 0)
> - libbpf_print(level, "[%u]", spec->spec[i].idx);
> + append_buf("[%u]", spec->spec[i].idx);
> }
>
> - libbpf_print(level, " (");
> + append_buf(" (");
> for (i = 0; i < spec->raw_len; i++)
> - libbpf_print(level, "%s%d", i == 0 ? "" : ":", spec->raw_spec[i]);
> + append_buf("%s%d", i == 0 ? "" : ":", spec->raw_spec[i]);
>
> if (spec->bit_offset % 8)
> - libbpf_print(level, " @ offset %u.%u)",
> - spec->bit_offset / 8, spec->bit_offset % 8);
> + append_buf(" @ offset %u.%u)", spec->bit_offset / 8, spec->bit_offset % 8);
> else
> - libbpf_print(level, " @ offset %u)", spec->bit_offset / 8);
> - return;
> + append_buf(" @ offset %u)", spec->bit_offset / 8);
> + return len;
> }
> +
> + return len;
> +#undef append_buf
> }
>
> /*
> @@ -1168,6 +1183,7 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> const char *local_name;
> __u32 local_id;
> const char *spec_str;
> + char spec_buf[256];
> int i, j, err;
>
> local_id = relo->type_id;
> @@ -1190,10 +1206,8 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> return -EINVAL;
> }
>
> - pr_debug("prog '%s': relo #%d: kind <%s> (%d), spec is ", prog_name,
> - relo_idx, core_relo_kind_str(relo->kind), relo->kind);
> - bpf_core_dump_spec(prog_name, LIBBPF_DEBUG, local_spec);
> - libbpf_print(LIBBPF_DEBUG, "\n");
> + bpf_core_format_spec(spec_buf, sizeof(spec_buf), local_spec);
> + pr_debug("prog '%s': relo #%d: %s\n", prog_name, relo_idx, spec_buf);
Looks great, but return value 'len' doesn't seem to be used in this
patch or in the following patch.
What was the intent ?
next prev parent reply other threads:[~2022-04-26 18:52 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-26 0:45 [PATCH bpf-next 00/10] Teach libbpf to "fix up" BPF verifier log Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 01/10] libbpf: fix anonymous type check in CO-RE logic Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 02/10] libbpf: drop unhelpful "program too large" guess Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 03/10] libbpf: fix logic for finding matching program for CO-RE relocation Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 04/10] libbpf: avoid joining .BTF.ext data with BPF programs by section name Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 05/10] selftests/bpf: add CO-RE relos and SEC("?...") to linked_funcs selftests Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 06/10] libbpf: record subprog-resolved CO-RE relocations unconditionally Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 07/10] libbpf: refactor CO-RE relo human description formatting routine Andrii Nakryiko
2022-04-26 18:52 ` Alexei Starovoitov [this message]
2022-04-26 22:20 ` Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 08/10] libbpf: simplify bpf_core_parse_spec() signature Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 09/10] libbpf: fix up verifier log for unguarded failed CO-RE relos Andrii Nakryiko
2022-04-26 18:59 ` Alexei Starovoitov
2022-04-26 22:16 ` Andrii Nakryiko
2022-04-26 0:45 ` [PATCH bpf-next 10/10] selftests/bpf: add libbpf's log fixup logic selftests Andrii Nakryiko
2022-04-26 22:50 ` [PATCH bpf-next 00/10] Teach libbpf to "fix up" BPF verifier log patchwork-bot+netdevbpf
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=20220426185248.ogbjc7f2rwfzhxqs@MacBook-Pro.local \
--to=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.