On Thu, Nov 7, 2024 at 5:40 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> On 07/11/2024 00:39, Andrii Nakryiko wrote:
> > On Mon, Nov 4, 2024 at 9:01 AM Mykyta Yatsenko
> > <mykyta.yatsenko5@xxxxxxxxx> wrote:
> >> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> >>
> >> Libbpf may report error in 2 ways:
> >> 1. Numeric errno
> >> 2. Errno's text representation, returned by strerror
> >> Both ways may be confusing for users: numeric code requires people to
> >> know how to find its meaning and strerror may be too generic and
> >> unclear.
> >>
> >> This patch modifies libbpf error reporting by swapping numeric codes and
> >> strerror with the standard short error name, for example:
> >> "failed to attach: -22" becomes "failed to attach: EINVAL".
> >>
> >> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> >> ---
> >> tools/lib/bpf/libbpf.c | 429 ++++++++++++++++++++++-------------------
> > We have use cases for strerr() in all of libbpf .c files, let's do the
> > conversion there as well. But I'd probably split adding strerr()
> > helper into first separate patch, and then would do the rest of
> > conversions in either one gigantic patch or split into some logical
> > groups of a few .c files (like, linker.c separate from libbpf.c,
> > separate from bpf.c, if we have any strerr() uses there). We have tons
> > of error message prints :)
> >
> > pw-bot: cr
> >
> >> 1 file changed, 231 insertions(+), 198 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 711173acbcef..26608d8585ec 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -336,6 +336,83 @@ static inline __u64 ptr_to_u64(const void *ptr)
> >> return (__u64) (unsigned long) ptr;
> >> }
> >>
> >> +/*
> >> + ** string returned from errstr() is invalidated upon the next call
> >> + */
> > keep it as single-line comment, but if you needed multi-line, it
> > should be formatted like so:
> >
> > /*
> > * blah blah blah lorem ipsum
> > */
> >
> >> +static const char *errstr(int err)
> > let's move this function into str_error.c, it doesn't have to live in
> > already huge libbpf.c (and you'll need to "expose" it in str_error.h
> > to use from not just libbpf.c anyways)
> >
> >> +{
> >> + static __thread char buf[11];
> > nit: make it buf[12] to technically handle "-2000000000" ?
> >
> >> + const char *str;
> >> + bool neg;
> >> +
> >> + if (err < 0) {
> >> + err = -err;
> >> + neg = true;
> >> + }
> > honestly, thinking about this a bit more, I think it's ok to always
> > emit negative error in the buffer (because that's what it should
> > always be, at least when this is used internally in libbpf).
> >
> > So let's have, just:
> >
> > if (err > 0)
> > err = -err;
> >
> > to make it explicit that negative error is the common/expected way
> >
> >
> >> +
> >> + switch (err) {
> >> + case EINVAL:
> >> + str = "-EINVAL"; break;
> > then for all of these we can have a nice and compact style:
> >
> > case -EINVAL: return "-EINVAL";
> > case -EPERM: return "-PERM";
> >
> >> + case EPERM:
> >> + str = "-EPERM"; break;
> >> + case ENOMEM:
> >> + str = "-ENOMEM"; break;
> >> + case ENOENT:
> >> + str = "-ENOENT"; break;
> >> + case E2BIG:
> >> + str = "-E2BIG"; break;
> >> + case EEXIST:
> >> + str = "-EEXIST"; break;
> >> + case EFAULT:
> >> + str = "-EFAULT"; break;
> >> + case ENOSPC:
> >> + str = "-ENOSPC"; break;
> >> + case EACCES:
> >> + str = "-EACCES"; break;
> >> + case EAGAIN:
> >> + str = "-EAGAIN"; break;
> >> + case EBADF:
> >> + str = "-EBADF"; break;
> >> + case ENAMETOOLONG:
> >> + str = "-ENAMETOOLONG"; break;
> >> + case ESRCH:
> >> + str = "-ESRCH"; break;
> >> + case EBUSY:
> >> + str = "-EBUSY"; break;
> >> + case ENOTSUP:
> > Is this one coming from public UAPI header? I don't think so.
> > include/linux/errno.h is not exported to user-space. This means that
> > Github version of libbpf will have trouble with compiling this. This
> > works ok inside kernel repo, but we should be careful about relying on
> > internal headers.
> Got it.
> >
> >
> > Please check all the other ones. BTW, how did you end up with this
> > exact set of errors?
> First I took all errors that bpf syscall sets, then just grepped for
> uppercase strings
> starting with E in tools/lib/bpf.
> The number of items very roughly matches what you suggested it to be
> (10-20), I have around 26.
Ok, I was going to suggest to at least search across main BPF code
base in kernel (kernel/bpf/...) and maybe tracing stuff as well
(kernel/trace/... and kernel/events/...). That should cover a lot of
relevant grounds. Check net/... but that might use a lot of niche
error codes that won't ever come up from BPF side of things, so I'm
not sure about that. We shall use our best judgement for that.
> >
> >> + str = "-ENOTSUP"; break;
> >> + case EPROTO:
> >> + str = "-EPROTO"; break;
> >> + case ERANGE:
> >> + str = "-ERANGE"; break;
> >> + case EMSGSIZE:
> >> + str = "-EMSGSIZE"; break;
> >> + case EINTR:
> >> + str = "-EINTR"; break;
> >> + case ENODATA:
> >> + str = "-ENODATA"; break;
> >> + case EIO:
> >> + str = "-EIO"; break;
> >> + case EUCLEAN:
> >> + str = "-EUCLEAN"; break;
> >> + case EDOM:
> >> + str = "-EDOM"; break;
> >> + case EPROTONOSUPPORT:
> >> + str = "-EPROTONOSUPPORT"; break;
> >> + case EDEADLK:
> >> + str = "-EDEADLK"; break;
> >> + case EOVERFLOW:
> >> + str = "-EOVERFLOW"; break;
> >> + default:
> >> + snprintf(buf, sizeof(buf), "%d", err);
> >> + return buf;
> > and then here we'll just
> >
> > snprintf(buf, sizeof(buf), "%d", err);
> > return buf;
> >
> >> + }
> >> + if (!neg)
> >> + ++str;
> >> +
> >> + return str;
> >> +}
> >> +
> > [...]
>
>
