[PATCH bpf-next] libbpf: Use text error for btf_custom_path failures

[PATCH bpf-next] libbpf: Use text error for btf_custom_path failures

[Dmitrii Dolgov]

Use libbpf_strerror_r to expand the error when failed to parse the btf
file at btf_custom_path. It does not change a lot locally, but since the
error will bubble up through a few layers, it may become quite
confusing otherwise. As an example here is what happens when the file
indicated via btf_custom_path does not exist and the caller uses
strerror as well:

    libbpf: failed to parse target BTF: -2
    libbpf: failed to perform CO-RE relocations: -2
    libbpf: failed to load object 'bpf_probe'
    libbpf: failed to load BPF skeleton 'bpf_probe': -2
    [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)

In this context "No such file or directory" could be easily
misinterpreted as belonging to some other part of loading process, e.g.
the BPF object itself. With this change it would look a bit better:

    libbpf: failed to parse target BTF: No such file or directory
    libbpf: failed to perform CO-RE relocations: -2
    libbpf: failed to load object 'bpf_probe'
    libbpf: failed to load BPF skeleton 'bpf_probe': -2
    [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 tools/lib/bpf/libbpf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 05c4db355f28..02a47552ad14 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5683,7 +5683,10 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
 		obj->btf_vmlinux_override = btf__parse(targ_btf_path, NULL);
 		err = libbpf_get_error(obj->btf_vmlinux_override);
 		if (err) {
-			pr_warn("failed to parse target BTF: %d\n", err);
+			char *cp, errmsg[STRERR_BUFSIZE];
+
+			cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
+			pr_warn("failed to parse target BTF: %s\n", cp);
 			return err;
 		}
 	}
-- 
2.31.1

Re: [PATCH bpf-next] libbpf: Use text error for btf_custom_path failures

[Andrii Nakryiko]

On Tue, Feb 28, 2023 at 6:26 AM Dmitrii Dolgov <9erthalion6@gmail.com> wrote:
>
> Use libbpf_strerror_r to expand the error when failed to parse the btf
> file at btf_custom_path. It does not change a lot locally, but since the
> error will bubble up through a few layers, it may become quite
> confusing otherwise. As an example here is what happens when the file
> indicated via btf_custom_path does not exist and the caller uses
> strerror as well:
>
>     libbpf: failed to parse target BTF: -2
>     libbpf: failed to perform CO-RE relocations: -2
>     libbpf: failed to load object 'bpf_probe'
>     libbpf: failed to load BPF skeleton 'bpf_probe': -2
>     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
>
> In this context "No such file or directory" could be easily
> misinterpreted as belonging to some other part of loading process, e.g.
> the BPF object itself. With this change it would look a bit better:
>
>     libbpf: failed to parse target BTF: No such file or directory
>     libbpf: failed to perform CO-RE relocations: -2
>     libbpf: failed to load object 'bpf_probe'
>     libbpf: failed to load BPF skeleton 'bpf_probe': -2
>     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---

I find these text-only error messages more harmful, actually. Very
often their literal meaning is confusing, and instead the process is
to guess what's -Exxx error they represent, and go from there.

Recently me and Quentin discussed moving towards an approach where
we'd log both symbolic error value (-EPERM instead of -1) and also
human-readable text message. So I'd prefer us figuring out how to do
this ergonomically in libbpf and bpftool code base, and start moving
in that direction.

>  tools/lib/bpf/libbpf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 05c4db355f28..02a47552ad14 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5683,7 +5683,10 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>                 obj->btf_vmlinux_override = btf__parse(targ_btf_path, NULL);
>                 err = libbpf_get_error(obj->btf_vmlinux_override);
>                 if (err) {
> -                       pr_warn("failed to parse target BTF: %d\n", err);
> +                       char *cp, errmsg[STRERR_BUFSIZE];
> +
> +                       cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> +                       pr_warn("failed to parse target BTF: %s\n", cp);
>                         return err;
>                 }
>         }
> --
> 2.31.1
>

Re: [PATCH bpf-next] libbpf: Use text error for btf_custom_path failures

[Dmitry Dolgov]

> On Wed, Mar 01, 2023 at 11:02:25AM -0800, Andrii Nakryiko wrote:
>
> > Use libbpf_strerror_r to expand the error when failed to parse the btf
> > file at btf_custom_path. It does not change a lot locally, but since the
> > error will bubble up through a few layers, it may become quite
> > confusing otherwise. As an example here is what happens when the file
> > indicated via btf_custom_path does not exist and the caller uses
> > strerror as well:
> >
> >     libbpf: failed to parse target BTF: -2
> >     libbpf: failed to perform CO-RE relocations: -2
> >     libbpf: failed to load object 'bpf_probe'
> >     libbpf: failed to load BPF skeleton 'bpf_probe': -2
> >     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
> >
> > In this context "No such file or directory" could be easily
> > misinterpreted as belonging to some other part of loading process, e.g.
> > the BPF object itself. With this change it would look a bit better:
> >
> >     libbpf: failed to parse target BTF: No such file or directory
> >     libbpf: failed to perform CO-RE relocations: -2
> >     libbpf: failed to load object 'bpf_probe'
> >     libbpf: failed to load BPF skeleton 'bpf_probe': -2
> >     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
>
> I find these text-only error messages more harmful, actually. Very
> often their literal meaning is confusing, and instead the process is
> to guess what's -Exxx error they represent, and go from there.
>
> Recently me and Quentin discussed moving towards an approach where
> we'd log both symbolic error value (-EPERM instead of -1) and also
> human-readable text message. So I'd prefer us figuring out how to do
> this ergonomically in libbpf and bpftool code base, and start moving
> in that direction.

Fair enough, thanks. I would love to try out any suggestions in this
area -- we were recently looking into error handling, and certain parts
were suboptimal.

Talking about confusing text error messages, I'm curious about -ESRCH
usage. It's being used in libbpf and various subsystem as well to
indicate that something wasn't found, so I guess it's an established
practice. But then in case btf__load_vmlinux_btf can't find a proper
file and reports an error, the caller gets surprising "No such process"
out of strerror. Am I missing something, is it implemented like this on
purpose?

Re: [PATCH bpf-next] libbpf: Use text error for btf_custom_path failures

[Andrii Nakryiko]

On Wed, Mar 1, 2023 at 1:09 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Wed, Mar 01, 2023 at 11:02:25AM -0800, Andrii Nakryiko wrote:
> >
> > > Use libbpf_strerror_r to expand the error when failed to parse the btf
> > > file at btf_custom_path. It does not change a lot locally, but since the
> > > error will bubble up through a few layers, it may become quite
> > > confusing otherwise. As an example here is what happens when the file
> > > indicated via btf_custom_path does not exist and the caller uses
> > > strerror as well:
> > >
> > >     libbpf: failed to parse target BTF: -2
> > >     libbpf: failed to perform CO-RE relocations: -2
> > >     libbpf: failed to load object 'bpf_probe'
> > >     libbpf: failed to load BPF skeleton 'bpf_probe': -2
> > >     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
> > >
> > > In this context "No such file or directory" could be easily
> > > misinterpreted as belonging to some other part of loading process, e.g.
> > > the BPF object itself. With this change it would look a bit better:
> > >
> > >     libbpf: failed to parse target BTF: No such file or directory
> > >     libbpf: failed to perform CO-RE relocations: -2
> > >     libbpf: failed to load object 'bpf_probe'
> > >     libbpf: failed to load BPF skeleton 'bpf_probe': -2
> > >     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
> >
> > I find these text-only error messages more harmful, actually. Very
> > often their literal meaning is confusing, and instead the process is
> > to guess what's -Exxx error they represent, and go from there.
> >
> > Recently me and Quentin discussed moving towards an approach where
> > we'd log both symbolic error value (-EPERM instead of -1) and also
> > human-readable text message. So I'd prefer us figuring out how to do
> > this ergonomically in libbpf and bpftool code base, and start moving
> > in that direction.
>
> Fair enough, thanks. I would love to try out any suggestions in this
> area -- we were recently looking into error handling, and certain parts
> were suboptimal.
>
> Talking about confusing text error messages, I'm curious about -ESRCH
> usage. It's being used in libbpf and various subsystem as well to
> indicate that something wasn't found, so I guess it's an established
> practice. But then in case btf__load_vmlinux_btf can't find a proper
> file and reports an error, the caller gets surprising "No such process"
> out of strerror. Am I missing something, is it implemented like this on
> purpose?

It's probably not 100% consistent throughout libbpf, but -ESRCH is
used to denote "a process to determine/find something failed". -ENOENT
is used when we are requested to find a specific entry, and it's not
there (but otherwise there were no errors encountered). That's the
distinction.

The problem with those text explanations of errors is that they are
coming from Linux's usage of them in the context of process or file
manipulations, and I don't see a way around that. I'd like to minimize
the use of custom error codes.

But this is the reason I'd like to output `-ESRCH` instead of either
-3 or "No such process". Something like "-ESRCH (No such process)" is
a compromise, but better than nothing.

Or we could stick to just -ESRCH. That might be better than test
descriptions, as we at least don't confuse them with irrelevant
descriptions.

But Quentin might find it not very user-friendly for his bpftool use
cases, probably.

Re: [PATCH bpf-next] libbpf: Use text error for btf_custom_path failures

[Quentin Monnet]

2023-03-04 15:39 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Wed, Mar 1, 2023 at 1:09 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>>
>>> On Wed, Mar 01, 2023 at 11:02:25AM -0800, Andrii Nakryiko wrote:
>>>
>>>> Use libbpf_strerror_r to expand the error when failed to parse the btf
>>>> file at btf_custom_path. It does not change a lot locally, but since the
>>>> error will bubble up through a few layers, it may become quite
>>>> confusing otherwise. As an example here is what happens when the file
>>>> indicated via btf_custom_path does not exist and the caller uses
>>>> strerror as well:
>>>>
>>>>     libbpf: failed to parse target BTF: -2
>>>>     libbpf: failed to perform CO-RE relocations: -2
>>>>     libbpf: failed to load object 'bpf_probe'
>>>>     libbpf: failed to load BPF skeleton 'bpf_probe': -2
>>>>     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
>>>>
>>>> In this context "No such file or directory" could be easily
>>>> misinterpreted as belonging to some other part of loading process, e.g.
>>>> the BPF object itself. With this change it would look a bit better:
>>>>
>>>>     libbpf: failed to parse target BTF: No such file or directory
>>>>     libbpf: failed to perform CO-RE relocations: -2
>>>>     libbpf: failed to load object 'bpf_probe'
>>>>     libbpf: failed to load BPF skeleton 'bpf_probe': -2
>>>>     [caller]: failed to load BPF object (errno: 2 | message: No such file or directory)
>>>
>>> I find these text-only error messages more harmful, actually. Very
>>> often their literal meaning is confusing, and instead the process is
>>> to guess what's -Exxx error they represent, and go from there.
>>>
>>> Recently me and Quentin discussed moving towards an approach where
>>> we'd log both symbolic error value (-EPERM instead of -1) and also
>>> human-readable text message. So I'd prefer us figuring out how to do
>>> this ergonomically in libbpf and bpftool code base, and start moving
>>> in that direction.
>>
>> Fair enough, thanks. I would love to try out any suggestions in this
>> area -- we were recently looking into error handling, and certain parts
>> were suboptimal.
>>
>> Talking about confusing text error messages, I'm curious about -ESRCH
>> usage. It's being used in libbpf and various subsystem as well to
>> indicate that something wasn't found, so I guess it's an established
>> practice. But then in case btf__load_vmlinux_btf can't find a proper
>> file and reports an error, the caller gets surprising "No such process"
>> out of strerror. Am I missing something, is it implemented like this on
>> purpose?
> 
> It's probably not 100% consistent throughout libbpf, but -ESRCH is
> used to denote "a process to determine/find something failed". -ENOENT
> is used when we are requested to find a specific entry, and it's not
> there (but otherwise there were no errors encountered). That's the
> distinction.
> 
> The problem with those text explanations of errors is that they are
> coming from Linux's usage of them in the context of process or file
> manipulations, and I don't see a way around that. I'd like to minimize
> the use of custom error codes.
> 
> But this is the reason I'd like to output `-ESRCH` instead of either
> -3 or "No such process". Something like "-ESRCH (No such process)" is
> a compromise, but better than nothing.
> 
> Or we could stick to just -ESRCH. That might be better than test
> descriptions, as we at least don't confuse them with irrelevant
> descriptions.
> 
> But Quentin might find it not very user-friendly for his bpftool use
> cases, probably.

Yes, even though the messages are sometimes confusing I find that there
are also occasions where they're actually useful to users not familiar
with the error names (not easy to figure out what "ESRCH" means if
you've never seen it before), so I'd avoid removing them entirely from
bpftool. Just as Andrii writes, we talked [0] on displaying both the
error name and the description through libbpf_strerror_r():

	Error: can't get next program: [-EPERM] Operation not permitted

So that users with more knowledge can skip the description and just look
at the error name.

I haven't started to work on this, though.

[0]
https://lore.kernel.org/all/CAEf4BzZMJGrRhNeQeWB0fRsuRYUv01aZGhvDeFV2o5zdpRbR-w@mail.gmail.com/