Sun Sep 25 2022 17:18:32 GMT+0100 ~ Tianyi Liu <i.pear@xxxxxxxxxxx>
> strerror() excepts a posivite errno, however libbpf_get_error()
s/excepts/expects/
> or variable err will never be positive when an error occurs.
> This causes bpftool to output too many "unknown error", even a simple
> "file not exist" error can't get an accurate prompt.
“prompt” (here and in commit title) sounds a bit odd to me in that
context, I'd say error string/message.
> This patch fixed all "strerror(err)" patten in bpftool.
s/patten/patterns/
>
> Signed-off-by: Tianyi Liu <i.pear@xxxxxxxxxxx>
> ---
> tools/bpf/bpftool/btf.c | 6 +++---
> tools/bpf/bpftool/gen.c | 4 ++--
> tools/bpf/bpftool/map_perf_ring.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 0744bd115..ac586c0e5 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -643,7 +643,7 @@ static int do_dump(int argc, char **argv)
> if (err) {
> btf = NULL;
> p_err("failed to load BTF from %s: %s",
> - *argv, strerror(err));
> + *argv, strerror(errno));
> goto done;
> }
> NEXT_ARG();
> @@ -689,7 +689,7 @@ static int do_dump(int argc, char **argv)
> btf = btf__load_from_kernel_by_id_split(btf_id, base_btf);
> err = libbpf_get_error(btf);
> if (err) {
> - p_err("get btf by id (%u): %s", btf_id, strerror(err));
> + p_err("get btf by id (%u): %s", btf_id, strerror(errno));
> goto done;
> }
> }
> @@ -825,7 +825,7 @@ build_btf_type_table(struct hashmap *tab, enum bpf_obj_type type,
> u32_as_hash_field(id));
> if (err) {
> p_err("failed to append entry to hashmap for BTF ID %u, object ID %u: %s",
> - btf_id, id, strerror(errno));
> + btf_id, id, strerror(-err));
> goto err_free;
> }
> }
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 7070dcffa..0783069f6 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1594,14 +1594,14 @@ static int do_object(int argc, char **argv)
>
> err = bpf_linker__add_file(linker, file, NULL);
> if (err) {
> - p_err("failed to link '%s': %s (%d)", file, strerror(err), err);
> + p_err("failed to link '%s': %s (%d)", file, strerror(errno), err);
Here and below, it would look more consistent to me to pass "errno" as
the last argument as well.
> goto out;
> }
> }
>
> err = bpf_linker__finalize(linker);
> if (err) {
> - p_err("failed to finalize ELF file: %s (%d)", strerror(err), err);
> + p_err("failed to finalize ELF file: %s (%d)", strerror(errno), err);
> goto out;
> }
>
> diff --git a/tools/bpf/bpftool/map_perf_ring.c b/tools/bpf/bpftool/map_perf_ring.c
> index 6b0c41015..1650b7127 100644
> --- a/tools/bpf/bpftool/map_perf_ring.c
> +++ b/tools/bpf/bpftool/map_perf_ring.c
> @@ -198,7 +198,7 @@ int do_event_pipe(int argc, char **argv)
> err = libbpf_get_error(pb);
> if (err) {
> p_err("failed to create perf buffer: %s (%d)",
> - strerror(err), err);
> + strerror(errno), err);
> goto err_close_map;
> }
>
> @@ -213,7 +213,7 @@ int do_event_pipe(int argc, char **argv)
> err = perf_buffer__poll(pb, 200);
> if (err < 0 && err != -EINTR) {
> p_err("perf buffer polling failed: %s (%d)",
> - strerror(err), err);
> + strerror(errno), err);
> goto err_close_pb;
> }
> }
Thanks a lot for the fix, but please see Andrii's comment on the other
thread [0]: now that bpftool relies on libbpf 1.0, the correct update
for these error checks would be to remove the calls to
libbpf_get_error() and to check directly for NULL values/errno, so we
won't need to check "err" and won't get the confusion about what
variable to use for the error messages.
[0]:
https://lore.kernel.org/bpf/CAEf4Bzaskw74UeufRgKSbGtk5eybD9J+4keAPYb-u=jb5myLjw@xxxxxxxxxxxxxx/
