| Message ID | SY4P282MB1084B61CD8671DFA395AA8579D539@SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpftool: Fix error prompt of strerror | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| 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-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| 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-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| 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-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-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-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
Sun Sep 25 2022 17:18:32 GMT+0100 ~ Tianyi Liu <i.pear@outlook.com> > 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@outlook.com> > --- > 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@mail.gmail.com/
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); 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; } }
strerror() excepts a posivite errno, however libbpf_get_error() 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. This patch fixed all "strerror(err)" patten in bpftool. Signed-off-by: Tianyi Liu <i.pear@outlook.com> --- 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(-)