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.
Please check all the other ones. BTW, how did you end up with this
exact set of errors?
> + 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;
> +}
> +
[...]
