On Thu, Oct 31, 2024 at 3:00 PM 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 | 401 +++++++++++++++++++++--------------------
> 1 file changed, 205 insertions(+), 196 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 711173acbcef..fac6b744302b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -226,6 +226,42 @@ static const char * const prog_type_name[] = {
> [BPF_PROG_TYPE_NETFILTER] = "netfilter",
> };
>
> +static const char * const errno_text[] = {
> + [1] = "EPERM", [2] = "ENOENT", [3] = "ESRCH",
> + [4] = "EINTR", [5] = "EIO", [6] = "ENXIO", [7] = "E2BIG",
> + [8] = "ENOEXEC", [9] = "EBADF", [10] = "ECHILD", [11] = "EAGAIN",
> + [12] = "ENOMEM", [13] = "EACCES", [14] = "EFAULT", [15] = "ENOTBLK",
> + [16] = "EBUSY", [17] = "EEXIST", [18] = "EXDEV", [19] = "ENODEV",
> + [20] = "ENOTDIR", [21] = "EISDIR", [22] = "EINVAL", [23] = "ENFILE",
> + [24] = "EMFILE", [25] = "ENOTTY", [26] = "ETXTBSY", [27] = "EFBIG",
> + [28] = "ENOSPC", [29] = "ESPIPE", [30] = "EROFS", [31] = "EMLINK",
> + [32] = "EPIPE", [33] = "EDOM", [34] = "ERANGE", [35] = "EDEADLK",
> + [36] = "ENAMETOOLONG", [37] = "ENOLCK", [38] = "ENOSYS", [39] = "ENOTEMPTY",
> + [40] = "ELOOP", [42] = "ENOMSG", [43] = "EIDRM", [44] = "ECHRNG",
> + [45] = "EL2NSYNC", [46] = "EL3HLT", [47] = "EL3RST", [48] = "ELNRNG",
> + [49] = "EUNATCH", [50] = "ENOCSI", [51] = "EL2HLT", [52] = "EBADE",
> + [53] = "EBADR", [54] = "EXFULL", [55] = "ENOANO", [56] = "EBADRQC",
> + [57] = "EBADSLT", [59] = "EBFONT", [60] = "ENOSTR", [61] = "ENODATA",
> + [62] = "ETIME", [63] = "ENOSR", [64] = "ENONET", [65] = "ENOPKG",
> + [66] = "EREMOTE", [67] = "ENOLINK", [68] = "EADV", [69] = "ESRMNT",
> + [70] = "ECOMM", [71] = "EPROTO", [72] = "EMULTIHOP", [73] = "EDOTDOT",
> + [74] = "EBADMSG", [75] = "EOVERFLOW", [76] = "ENOTUNIQ", [77] = "EBADFD",
> + [78] = "EREMCHG", [79] = "ELIBACC", [80] = "ELIBBAD", [81] = "ELIBSCN",
> + [82] = "ELIBMAX", [83] = "ELIBEXEC", [84] = "EILSEQ", [85] = "ERESTART",
> + [86] = "ESTRPIPE", [87] = "EUSERS", [88] = "ENOTSOCK", [89] = "EDESTADDRREQ",
> + [90] = "EMSGSIZE", [91] = "EPROTOTYPE", [92] = "ENOPROTOOPT", [93] = "EPROTONOSUPPORT",
> + [94] = "ESOCKTNOSUPPORT", [95] = "EOPNOTSUPP", [96] = "EPFNOSUPPORT", [97] = "EAFNOSUPPORT",
> + [98] = "EADDRINUSE", [99] = "EADDRNOTAVAIL", [100] = "ENETDOWN", [101] = "ENETUNREACH",
> + [102] = "ENETRESET", [103] = "ECONNABORTED", [104] = "ECONNRESET", [105] = "ENOBUFS",
> + [106] = "EISCONN", [107] = "ENOTCONN", [108] = "ESHUTDOWN", [109] = "ETOOMANYREFS",
> + [110] = "ETIMEDOUT", [111] = "ECONNREFUSED", [112] = "EHOSTDOWN", [113] = "EHOSTUNREACH",
> + [114] = "EALREADY", [115] = "EINPROGRESS", [116] = "ESTALE", [117] = "EUCLEAN",
> + [118] = "ENOTNAM", [119] = "ENAVAIL", [120] = "EISNAM", [121] = "EREMOTEIO",
> + [122] = "EDQUOT", [123] = "ENOMEDIUM", [124] = "EMEDIUMTYPE", [125] = "ECANCELED",
> + [126] = "ENOKEY", [127] = "EKEYEXPIRED", [128] = "EKEYREVOKED", [129] = "EKEYREJECTED",
> + [130] = "EOWNERDEAD", [131] = "ENOTRECOVERABLE", [132] = "ERFKILL", [133] = "EHWPOISON",
> +};
> +
some error codes are architecture-specific, so lookup table is not
ideal. I think it will be better to just have a simple switch()
statement in errstr() function
also, I don't think, practically speaking, that we need to have all
possible error codes translated. There are 10-20 of errors that might
happen, and I'd be fine having error number for other rare case (plus
we can always update the list)
one important thing that seems to be missing here is:
#define ENOTSUPP 524
It's not part of Linux UAPI (so there won't be ENOTSUPP identifier),
but we should make sure to understand it, as it happens very often
with bpf() subsystem
> static int __base_pr(enum libbpf_print_level level, const char *format,
> va_list args)
> {
> @@ -336,6 +372,20 @@ static inline __u64 ptr_to_u64(const void *ptr)
> return (__u64) (unsigned long) ptr;
> }
>
let's add a comment reminding that string returned from this helper is
invalidated upon next errstr() call
> +static const char *errstr(int err)
> +{
> + static __thread char buf[11];
> +
> + if (err < 0)
> + err = -err;
> +
> + if (err < ARRAY_SIZE(errno_text) && errno_text[err])
> + return errno_text[err];
nit: we should preserve original minus sign
> +
> + snprintf(buf, ARRAY_SIZE(buf), "%d", err);
nit: sizeof(buf), ARRAY_SIZE() is useful when element type is not a byte
and also please restore original minus sign here as well
> + return buf;
> +}
> +
> int libbpf_set_strict_mode(enum libbpf_strict_mode mode)
> {
> /* as of v1.0 libbpf_set_strict_mode() is a no-op */
> @@ -1550,11 +1600,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
> } else {
> obj->efile.fd = open(obj->path, O_RDONLY | O_CLOEXEC);
> if (obj->efile.fd < 0) {
> - char errmsg[STRERR_BUFSIZE], *cp;
> -
> - err = -errno;
> - cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> - pr_warn("elf: failed to open %s: %s\n", obj->path, cp);
> + pr_warn("elf: failed to open %s: %s\n", obj->path, errstr(err));
> return err;
> }
>
> @@ -1960,8 +2006,8 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
> if (map->mmaped == MAP_FAILED) {
> err = -errno;
> map->mmaped = NULL;
> - pr_warn("failed to alloc map '%s' content buffer: %d\n",
> - map->name, err);
> + pr_warn("failed to alloc map '%s' content buffer: %s\n",
> + map->name, errstr(err));
if it fits in under 100 characters, let's make such pr_*() statements
single-line
> zfree(&map->real_name);
> zfree(&map->name);
> return err;
> @@ -2125,7 +2171,7 @@ static int parse_u64(const char *value, __u64 *res)
> *res = strtoull(value, &value_end, 0);
> if (errno) {
> err = -errno;
> - pr_warn("failed to parse '%s' as integer: %d\n", value, err);
> + pr_warn("failed to parse '%s': %s\n", value, errstr(err));
> return err;
> }
> if (*value_end) {
[...]
> @@ -5026,11 +5074,10 @@ bpf_object__probe_loading(struct bpf_object *obj)
> ret = bpf_prog_load(BPF_PROG_TYPE_TRACEPOINT, NULL, "GPL", insns, insn_cnt, &opts);
> if (ret < 0) {
> ret = errno;
> - cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
> - pr_warn("Error in %s():%s(%d). Couldn't load trivial BPF "
> + pr_warn("Error in %s():%s. Couldn't load trivial BPF "
nit: ':<space> %s'
> "program. Make sure your kernel supports BPF "
> "(CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is "
while you are at it, please make the message a single-line string to
improve grepping
> - "set to big enough value.\n", __func__, cp, ret);
> + "set to big enough value.\n", __func__, errstr(ret));
> return -ret;
> }
> close(ret);
[...]
> @@ -5287,12 +5326,9 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
> def->max_entries, &create_attr);
> }
> if (map_fd < 0 && (create_attr.btf_key_type_id || create_attr.btf_value_type_id)) {
> - char *cp, errmsg[STRERR_BUFSIZE];
> -
> err = -errno;
> - cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> - pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> - map->name, cp, err);
> + pr_warn("Error in bpf_create_map_xattr(%s):%s. Retrying without BTF.\n",
please add space (I know it didn't have it before, but let's make all
this as consistent as possible)
> + map->name, errstr(err));
> create_attr.btf_fd = 0;
> create_attr.btf_key_type_id = 0;
> create_attr.btf_value_type_id = 0;
I didn't really look through the rest of conversions, but please make
sure the styling is consistent (if there are any deviations)
[...]
pw-bot: cr
