| Message ID | SY4P282MB1084C694AB3F6C2DD3DD3A379D529@SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Add friendly error prompt for missing .BTF section | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for 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-13 | success | Logs for test_progs_no_alu32 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-10 | success | Logs for test_progs 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-7 | success | Logs for test_maps 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-8 | success | Logs for test_maps 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-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| 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 |
On Sun, Sep 25, 2022 at 10:54 PM Tianyi Liu <i.pear@outlook.com> wrote: > > Fresh users usually forget to turn on BTF generating flags compiling > kernels, and will receive a confusing error "No such file or directory" > (from return value ENOENT) with command "bpftool btf dump file vmlinux". > > Hope this can help them find the mistake. > > Signed-off-by: Tianyi Liu <i.pear@outlook.com> > --- > tools/lib/bpf/btf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index 2d14f1a52..9fbae1f3d 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -990,6 +990,8 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf, > err = 0; > > if (!btf_data) { > + pr_warn("Failed to get section %s from ELF %s, check CONFIG_DEBUG_INFO_BTF if compiling kernel\n", > + BTF_ELF_SEC, path); This is going to be very confusing for any user trying to load BTF from some other ELF file. If we want to add such helpful suggestion (and even then it's a bit of a hit and miss, as not every passed in file is supposed to be vmlinux kernel image), it should be done in bpftool proper. > err = -ENOENT; > goto done; > } > -- > 2.37.3 >
> -----Original Messages----- > From: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Date: Tuesday, September 27, 2022 at 12:37 > To: Tianyi Liu <i.pear@outlook.com> > Cc: andrii@kernel.org <andrii@kernel.org>, bpf@vger.kernel.org <bpf@vger.kernel.org>, trivial@kernel.org <trivial@kernel.org> > Subject: Re: [PATCH] libbpf: Add friendly error prompt for missing .BTF section > On Sun, Sep 25, 2022 at 10:54 PM Tianyi Liu <i.pear@outlook.com> wrote: > > > > Fresh users usually forget to turn on BTF generating flags compiling > > kernels, and will receive a confusing error "No such file or directory" > > (from return value ENOENT) with command "bpftool btf dump file vmlinux". > > > > Hope this can help them find the mistake. > > > > Signed-off-by: Tianyi Liu <i.pear@outlook.com> > > --- > > tools/lib/bpf/btf.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > index 2d14f1a52..9fbae1f3d 100644 > > --- a/tools/lib/bpf/btf.c > > +++ b/tools/lib/bpf/btf.c > > @@ -990,6 +990,8 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf, > > err = 0; > > > > if (!btf_data) { > > + pr_warn("Failed to get section %s from ELF %s, check CONFIG_DEBUG_INFO_BTF if compiling kernel\n", > > + BTF_ELF_SEC, path); > > This is going to be very confusing for any user trying to load BTF > from some other ELF file. If we want to add such helpful suggestion > (and even then it's a bit of a hit and miss, as not every passed in > file is supposed to be vmlinux kernel image), it should be done in > bpftool proper. > > > err = -ENOENT; > > goto done; > > } > > -- > > 2.37.3 > > Hi Andrii : I agree with you, I will try to implement it in bpftool. But there’s another problem here, in btf_parse_elf() from tools/lib/bpf/btf.c: If the path does not exist, open() assigns ENOENT to errno, and btf_parse_elf() returns -ENOENT. Besides, if .BTF section can not be found in the ELF file, btf_parse_elf() also returns -ENOENT, the same as above. So we can't determine which kind of error it is from the outside. Could we change the err code in the second case to make it clearer, Such as changing ENOENT to EPROTO / adding a new error code? Or we can just warn that .BTF section does not exist. Thanks.
Tianyi Liu wrote: > > -----Original Messages----- > > From: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > Date: Tuesday, September 27, 2022 at 12:37 > > To: Tianyi Liu <i.pear@outlook.com> > > Cc: andrii@kernel.org <andrii@kernel.org>, bpf@vger.kernel.org <bpf@vger.kernel.org>, trivial@kernel.org <trivial@kernel.org> > > Subject: Re: [PATCH] libbpf: Add friendly error prompt for missing .BTF section > > On Sun, Sep 25, 2022 at 10:54 PM Tianyi Liu <i.pear@outlook.com> wrote: > > > > > > Fresh users usually forget to turn on BTF generating flags compiling > > > kernels, and will receive a confusing error "No such file or directory" > > > (from return value ENOENT) with command "bpftool btf dump file vmlinux". > > > > > > Hope this can help them find the mistake. > > > > > > Signed-off-by: Tianyi Liu <i.pear@outlook.com> > > > --- > > > tools/lib/bpf/btf.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > > index 2d14f1a52..9fbae1f3d 100644 > > > --- a/tools/lib/bpf/btf.c > > > +++ b/tools/lib/bpf/btf.c > > > @@ -990,6 +990,8 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf, > > > err = 0; > > > > > > if (!btf_data) { > > > + pr_warn("Failed to get section %s from ELF %s, check CONFIG_DEBUG_INFO_BTF if compiling kernel\n", > > > + BTF_ELF_SEC, path); > > > > This is going to be very confusing for any user trying to load BTF > > from some other ELF file. If we want to add such helpful suggestion > > (and even then it's a bit of a hit and miss, as not every passed in > > file is supposed to be vmlinux kernel image), it should be done in > > bpftool proper. > > > > > err = -ENOENT; > > > goto done; > > > } > > > -- > > > 2.37.3 > > > > > Hi Andrii : > I agree with you, I will try to implement it in bpftool. > > But there’s another problem here, in btf_parse_elf() from tools/lib/bpf/btf.c: > If the path does not exist, open() assigns ENOENT to errno, and btf_parse_elf() > returns -ENOENT. Besides, if .BTF section can not be found in the ELF file, > btf_parse_elf() also returns -ENOENT, the same as above. So we can't determine > which kind of error it is from the outside. Just do a stat() on the file to see if it exists on error and then give more verbose warning? We did something similar in our loader code for tooling fwiw. > > Could we change the err code in the second case to make it clearer, Such as > changing ENOENT to EPROTO / adding a new error code? Or we can just warn > that .BTF section does not exist. I don't think its needed. > > Thanks. >
On Thu, Sep 29, 2022 at 6:58 PM John Fastabend <john.fastabend@gmail.com> wrote: > > Tianyi Liu wrote: > > > -----Original Messages----- > > > From: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > Date: Tuesday, September 27, 2022 at 12:37 > > > To: Tianyi Liu <i.pear@outlook.com> > > > Cc: andrii@kernel.org <andrii@kernel.org>, bpf@vger.kernel.org <bpf@vger.kernel.org>, trivial@kernel.org <trivial@kernel.org> > > > Subject: Re: [PATCH] libbpf: Add friendly error prompt for missing .BTF section > > > On Sun, Sep 25, 2022 at 10:54 PM Tianyi Liu <i.pear@outlook.com> wrote: > > > > > > > > Fresh users usually forget to turn on BTF generating flags compiling > > > > kernels, and will receive a confusing error "No such file or directory" > > > > (from return value ENOENT) with command "bpftool btf dump file vmlinux". > > > > > > > > Hope this can help them find the mistake. > > > > > > > > Signed-off-by: Tianyi Liu <i.pear@outlook.com> > > > > --- > > > > tools/lib/bpf/btf.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > > > index 2d14f1a52..9fbae1f3d 100644 > > > > --- a/tools/lib/bpf/btf.c > > > > +++ b/tools/lib/bpf/btf.c > > > > @@ -990,6 +990,8 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf, > > > > err = 0; > > > > > > > > if (!btf_data) { > > > > + pr_warn("Failed to get section %s from ELF %s, check CONFIG_DEBUG_INFO_BTF if compiling kernel\n", > > > > + BTF_ELF_SEC, path); > > > > > > This is going to be very confusing for any user trying to load BTF > > > from some other ELF file. If we want to add such helpful suggestion > > > (and even then it's a bit of a hit and miss, as not every passed in > > > file is supposed to be vmlinux kernel image), it should be done in > > > bpftool proper. > > > > > > > err = -ENOENT; > > > > goto done; > > > > } > > > > -- > > > > 2.37.3 > > > > > > > > Hi Andrii : > > I agree with you, I will try to implement it in bpftool. > > > > But there’s another problem here, in btf_parse_elf() from tools/lib/bpf/btf.c: > > If the path does not exist, open() assigns ENOENT to errno, and btf_parse_elf() > > returns -ENOENT. Besides, if .BTF section can not be found in the ELF file, > > btf_parse_elf() also returns -ENOENT, the same as above. So we can't determine > > which kind of error it is from the outside. > > Just do a stat() on the file to see if it exists on error and then > give more verbose warning? We did something similar in our loader > code for tooling fwiw. > > > > > Could we change the err code in the second case to make it clearer, Such as > > changing ENOENT to EPROTO / adding a new error code? Or we can just warn > > that .BTF section does not exist. > > I don't think its needed. Agree. I don't think we should modify libbpf for this, best to add extra check in bpftool. > > > > > Thanks. > > > >
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 2d14f1a52..9fbae1f3d 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -990,6 +990,8 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf, err = 0; if (!btf_data) { + pr_warn("Failed to get section %s from ELF %s, check CONFIG_DEBUG_INFO_BTF if compiling kernel\n", + BTF_ELF_SEC, path); err = -ENOENT; goto done; }
Fresh users usually forget to turn on BTF generating flags compiling kernels, and will receive a confusing error "No such file or directory" (from return value ENOENT) with command "bpftool btf dump file vmlinux". Hope this can help them find the mistake. Signed-off-by: Tianyi Liu <i.pear@outlook.com> --- tools/lib/bpf/btf.c | 2 ++ 1 file changed, 2 insertions(+)