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@xxxxxxxxxx>
> ---
> 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 ?
