| Message ID | SY4P282MB1084AD9CD84A920F08DF83E29D549@SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3ca2fb497440a3c8294f9df0ce7b2c3c9a1c5875 |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] bpftool: Fix error message of strerror | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
Wed Sep 28 2022 09:09:32 GMT+0100 ~ Tianyi Liu <i.pear@outlook.com> > strerror() expects a positive errno, however 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 not get an > accurate message. > > This patch fixed all "strerror(err)" patterns in bpftool. > Specially in btf.c#L823, hashmap__append() is an internal function of > libbpf and will not change errno, so there's a little difference. > Some libbpf_get_error() calls are kept for return values. > > Changes since v1: https://lore.kernel.org/bpf/SY4P282MB1084B61CD8671DFA395AA8579D539@SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM/ > Check directly for NULL values instead of calling libbpf_get_error(). > > Signed-off-by: Tianyi Liu <i.pear@outlook.com> > --- > tools/bpf/bpftool/btf.c | 11 +++++------ > tools/bpf/bpftool/gen.c | 4 ++-- > tools/bpf/bpftool/map_perf_ring.c | 7 +++---- > 3 files changed, 10 insertions(+), 12 deletions(-) > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > index 0744bd115..933177bdd 100644 > --- a/tools/bpf/bpftool/btf.c > +++ b/tools/bpf/bpftool/btf.c > @@ -640,10 +640,9 @@ static int do_dump(int argc, char **argv) > > btf = btf__parse_split(*argv, base ?: base_btf); > err = libbpf_get_error(btf); > - if (err) { > - btf = NULL; > + if (!btf) { > p_err("failed to load BTF from %s: %s", > - *argv, strerror(err)); > + *argv, strerror(errno)); > goto done; > } I thought we could get rid of libbpf_get_error() entirely, but I hadn't realised "err" is later returned by the function. We'll need a pass to clean these up eventually, but this is beyond the scope of your patch. Thanks! And good catch for hashmap__append(). Reviewed-by: Quentin Monnet <quentin@isovalent.com>
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Wed, 28 Sep 2022 16:09:32 +0800 you wrote: > strerror() expects a positive errno, however 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 not get an > accurate message. > > This patch fixed all "strerror(err)" patterns in bpftool. > Specially in btf.c#L823, hashmap__append() is an internal function of > libbpf and will not change errno, so there's a little difference. > Some libbpf_get_error() calls are kept for return values. > > [...] Here is the summary with links: - [v2] bpftool: Fix error message of strerror https://git.kernel.org/bpf/bpf-next/c/3ca2fb497440 You are awesome, thank you!
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index 0744bd115..933177bdd 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -640,10 +640,9 @@ static int do_dump(int argc, char **argv) btf = btf__parse_split(*argv, base ?: base_btf); err = libbpf_get_error(btf); - if (err) { - btf = NULL; + if (!btf) { p_err("failed to load BTF from %s: %s", - *argv, strerror(err)); + *argv, strerror(errno)); goto done; } NEXT_ARG(); @@ -688,8 +687,8 @@ 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)); + if (!btf) { + p_err("get btf by id (%u): %s", btf_id, strerror(errno)); goto done; } } @@ -825,7 +824,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..cf8b4e525 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), errno); 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), errno); goto out; } diff --git a/tools/bpf/bpftool/map_perf_ring.c b/tools/bpf/bpftool/map_perf_ring.c index 6b0c41015..309d5a1e6 100644 --- a/tools/bpf/bpftool/map_perf_ring.c +++ b/tools/bpf/bpftool/map_perf_ring.c @@ -195,10 +195,9 @@ int do_event_pipe(int argc, char **argv) opts.map_keys = &ctx.idx; pb = perf_buffer__new_raw(map_fd, MMAP_PAGE_CNT, &perf_attr, print_bpf_output, &ctx, &opts); - err = libbpf_get_error(pb); - if (err) { + if (!pb) { p_err("failed to create perf buffer: %s (%d)", - strerror(err), err); + strerror(errno), errno); goto err_close_map; } @@ -213,7 +212,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), errno); goto err_close_pb; } }
strerror() expects a positive errno, however 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 not get an accurate message. This patch fixed all "strerror(err)" patterns in bpftool. Specially in btf.c#L823, hashmap__append() is an internal function of libbpf and will not change errno, so there's a little difference. Some libbpf_get_error() calls are kept for return values. Changes since v1: https://lore.kernel.org/bpf/SY4P282MB1084B61CD8671DFA395AA8579D539@SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM/ Check directly for NULL values instead of calling libbpf_get_error(). Signed-off-by: Tianyi Liu <i.pear@outlook.com> --- tools/bpf/bpftool/btf.c | 11 +++++------ tools/bpf/bpftool/gen.c | 4 ++-- tools/bpf/bpftool/map_perf_ring.c | 7 +++---- 3 files changed, 10 insertions(+), 12 deletions(-)