Re: [PATCH 1/5] bpf tools: Improve libbpf error reporting
From: Arnaldo Carvalho de Melo
Date: Thu Nov 05 2015 - 10:35:24 EST
Em Thu, Nov 05, 2015 at 12:26:07PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Nov 05, 2015 at 04:26:58AM +0000, Wang Nan escreveu:
> > In this patch, a series libbpf specific error numbers and
> > libbpf_strerror() are created to help reporting error to caller.
> > Functions are updated to pass correct error number through macro
> > CHECK_ERR().
> >
> > All users of bpf_object__open{_buffer}() and bpf_program__title()
> > in perf are modified accordingly. In addition, due to error code
> > changing, bpf__strerror_load() also modified to use new error code.
> >
> > bpf__strerror_head() is also changed accordingly so it can parse
> > libbpf error. bpf_loader_strerror() is introduced for it, and will
> > be improved by following patch.
>
> I am applying this, test shows a improvement in error reporting, but
> please look below for some suggestions:
I take that back, better fix this now, see the other message about the
BPF loader part too.
>
> [root@zoo ~]# perf record -e /tmp/foo.o sleep 1
> event syntax error: '/tmp/foo.o'
> \___ Failed to load program: Validate your program and check 'license'/'version' sections in your object
>
> Now this is a long error message, maybe:
>
> event syntax error: '/tmp/foo.o'
> \___ Failed to load: Check 'license'/'version' sections
>
> And then, here we _know_ the problem is in the license, so why not use
> that knowledge to help the user further and show instead:
>
> event syntax error: '/tmp/foo.o'
> \___ Failed to load: 'version'(4.2.0) doesn't match running kernel (4.3.0)
>
> More below
>
> > Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/libbpf.c | 149 ++++++++++++++++++++++++++++-------------
> > tools/lib/bpf/libbpf.h | 12 ++++
> > tools/perf/tests/llvm.c | 2 +-
> > tools/perf/util/bpf-loader.c | 36 ++++++++--
> > tools/perf/util/parse-events.c | 4 +-
> > 5 files changed, 145 insertions(+), 58 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 9f3c8cf..c4dcee0 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -61,6 +61,62 @@ void libbpf_set_print(libbpf_print_fn_t warn,
> > __pr_debug = debug;
> > }
> >
> > +#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
> > +#define STRERR_BUFSIZE 128
> > +
> > +struct {
> > + int code;
> > + const char *msg;
> > +} libbpf_strerror_table[] = {
> > + {LIBBPF_ERRNO__ELIBELF, "Something wrong in libelf"},
> > + {LIBBPF_ERRNO__EFORMAT, "BPF object format invalid"},
> > + {LIBBPF_ERRNO__EKVERSION, "'version' section incorrect or lost"},
> > + {LIBBPF_ERRNO__EENDIAN, "Endian missmatch"},
> > + {LIBBPF_ERRNO__EINTERNAL, "Internal error in libbpf"},
> > + {LIBBPF_ERRNO__ERELOC, "Relocation failed"},
> > + {LIBBPF_ERRNO__ELOAD, "Failed to load program"},
> > +};
> > +
> > +int libbpf_strerror(int err, char *buf, size_t size)
> > +{
> > + unsigned int i;
> > +
> > + if (!buf || !size)
> > + return -1;
> > +
> > + err = err > 0 ? err : -err;
> > +
> > + if (err < LIBBPF_ERRNO__START) {
> > + int ret;
> > +
> > + ret = strerror_r(err, buf, size);
> > + buf[size - 1] = '\0';
> > + return ret;
> > + }
>
> Since all those errors are in sequence, why not introduce a
> LIBBPF_ERRNO__END and test against it, and then index the
> libbpf_strerror_table directly using (err - LIBBPF_ERRNO__START)?
>
> When introducing a function, take a look at similar functions and reuse
> code, for instance: target__strerror() does this pretty well, I think.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(libbpf_strerror_table); i++) {
> > + if (libbpf_strerror_table[i].code == err) {
> > + const char *msg;
> > +
> > + msg = libbpf_strerror_table[i].msg;
> > + snprintf(buf, size, "%s", msg);
> > + buf[size - 1] = '\0';
> > + return 0;
> > + }
> > + }
> > +
> > + snprintf(buf, size, "Unknown libbpf error %d", err);
> > + buf[size - 1] = '\0';
> > + return -1;
> > +}
> > +
> > +#define CHECK_ERR(action, err, out) do { \
> > + err = action; \
> > + if (err) \
> > + goto out; \
> > +} while(0)
> > +
> > +
> > /* Copied from tools/perf/util/util.h */
> > #ifndef zfree
> > # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
> > @@ -258,7 +314,7 @@ static struct bpf_object *bpf_object__new(const char *path,
> > obj = calloc(1, sizeof(struct bpf_object) + strlen(path) + 1);
> > if (!obj) {
> > pr_warning("alloc memory failed for %s\n", path);
> > - return NULL;
> > + return ERR_PTR(-ENOMEM);
> > }
> >
> > strcpy(obj->path, path);
> > @@ -305,7 +361,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
> >
> > if (obj_elf_valid(obj)) {
> > pr_warning("elf init: internal error\n");
> > - return -EEXIST;
> > + return -LIBBPF_ERRNO__ELIBELF;
> > }
> >
> > if (obj->efile.obj_buf_sz > 0) {
> > @@ -331,14 +387,14 @@ static int bpf_object__elf_init(struct bpf_object *obj)
> > if (!obj->efile.elf) {
> > pr_warning("failed to open %s as ELF file\n",
> > obj->path);
> > - err = -EINVAL;
> > + err = -LIBBPF_ERRNO__ELIBELF;
> > goto errout;
> > }
> >
> > if (!gelf_getehdr(obj->efile.elf, &obj->efile.ehdr)) {
> > pr_warning("failed to get EHDR from %s\n",
> > obj->path);
> > - err = -EINVAL;
> > + err = -LIBBPF_ERRNO__EFORMAT;
> > goto errout;
> > }
> > ep = &obj->efile.ehdr;
> > @@ -346,7 +402,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
> > if ((ep->e_type != ET_REL) || (ep->e_machine != 0)) {
> > pr_warning("%s is not an eBPF object file\n",
> > obj->path);
> > - err = -EINVAL;
> > + err = -LIBBPF_ERRNO__EFORMAT;
> > goto errout;
> > }
> >
> > @@ -374,14 +430,14 @@ bpf_object__check_endianness(struct bpf_object *obj)
> > goto mismatch;
> > break;
> > default:
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EENDIAN;
> > }
> >
> > return 0;
> >
> > mismatch:
> > pr_warning("Error: endianness mismatch.\n");
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EENDIAN;
> > }
> >
> > static int
> > @@ -402,7 +458,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
> >
> > if (size != sizeof(kver)) {
> > pr_warning("invalid kver section in %s\n", obj->path);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EFORMAT;
> > }
> > memcpy(&kver, data, sizeof(kver));
> > obj->kern_version = kver;
> > @@ -444,7 +500,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> > if (!elf_rawdata(elf_getscn(elf, ep->e_shstrndx), NULL)) {
> > pr_warning("failed to get e_shstrndx from %s\n",
> > obj->path);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EFORMAT;
> > }
> >
> > while ((scn = elf_nextscn(elf, scn)) != NULL) {
> > @@ -456,7 +512,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> > if (gelf_getshdr(scn, &sh) != &sh) {
> > pr_warning("failed to get section header from %s\n",
> > obj->path);
> > - err = -EINVAL;
> > + err = -LIBBPF_ERRNO__EFORMAT;
> > goto out;
> > }
> >
> > @@ -464,7 +520,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> > if (!name) {
> > pr_warning("failed to get section name from %s\n",
> > obj->path);
> > - err = -EINVAL;
> > + err = -LIBBPF_ERRNO__EFORMAT;
> > goto out;
> > }
> >
> > @@ -472,7 +528,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> > if (!data) {
> > pr_warning("failed to get section data from %s(%s)\n",
> > name, obj->path);
> > - err = -EINVAL;
> > + err = -LIBBPF_ERRNO__EFORMAT;
> > goto out;
> > }
> > pr_debug("section %s, size %ld, link %d, flags %lx, type=%d\n",
> > @@ -495,7 +551,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> > if (obj->efile.symbols) {
> > pr_warning("bpf: multiple SYMTAB in %s\n",
> > obj->path);
> > - err = -EEXIST;
> > + err = -LIBBPF_ERRNO__EFORMAT;
> > } else
> > obj->efile.symbols = data;
> > } else if ((sh.sh_type == SHT_PROGBITS) &&
> > @@ -504,7 +560,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> > err = bpf_object__add_program(obj, data->d_buf,
> > data->d_size, name, idx);
> > if (err) {
> > - char errmsg[128];
> > + char errmsg[STRERR_BUFSIZE];
> > +
> > strerror_r(-err, errmsg, sizeof(errmsg));
> > pr_warning("failed to alloc program %s (%s): %s",
> > name, obj->path, errmsg);
> > @@ -576,7 +633,7 @@ bpf_program__collect_reloc(struct bpf_program *prog,
> >
> > if (!gelf_getrel(data, i, &rel)) {
> > pr_warning("relocation: failed to get %d reloc\n", i);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EFORMAT;
> > }
> >
> > insn_idx = rel.r_offset / sizeof(struct bpf_insn);
> > @@ -587,20 +644,20 @@ bpf_program__collect_reloc(struct bpf_program *prog,
> > &sym)) {
> > pr_warning("relocation: symbol %"PRIx64" not found\n",
> > GELF_R_SYM(rel.r_info));
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EFORMAT;
> > }
> >
> > if (insns[insn_idx].code != (BPF_LD | BPF_IMM | BPF_DW)) {
> > pr_warning("bpf: relocation: invalid relo for insns[%d].code 0x%x\n",
> > insn_idx, insns[insn_idx].code);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__ERELOC;
> > }
> >
> > map_idx = sym.st_value / sizeof(struct bpf_map_def);
> > if (map_idx >= nr_maps) {
> > pr_warning("bpf relocation: map_idx %d large than %d\n",
> > (int)map_idx, (int)nr_maps - 1);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__ERELOC;
> > }
> >
> > prog->reloc_desc[i].insn_idx = insn_idx;
> > @@ -683,7 +740,7 @@ bpf_program__relocate(struct bpf_program *prog, int *map_fds)
> > if (insn_idx >= (int)prog->insns_cnt) {
> > pr_warning("relocation out of range: '%s'\n",
> > prog->section_name);
> > - return -ERANGE;
> > + return -LIBBPF_ERRNO__ERELOC;
> > }
> > insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
> > insns[insn_idx].imm = map_fds[map_idx];
> > @@ -721,7 +778,7 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
> >
> > if (!obj_elf_valid(obj)) {
> > pr_warning("Internal error: elf object is closed\n");
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EINTERNAL;
> > }
> >
> > for (i = 0; i < obj->efile.nr_reloc; i++) {
> > @@ -734,21 +791,21 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
> >
> > if (shdr->sh_type != SHT_REL) {
> > pr_warning("internal error at %d\n", __LINE__);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EINTERNAL;
> > }
> >
> > prog = bpf_object__find_prog_by_idx(obj, idx);
> > if (!prog) {
> > pr_warning("relocation failed: no %d section\n",
> > idx);
> > - return -ENOENT;
> > + return -LIBBPF_ERRNO__ERELOC;
> > }
> >
> > err = bpf_program__collect_reloc(prog, nr_maps,
> > shdr, data,
> > obj->efile.symbols);
> > if (err)
> > - return -EINVAL;
> > + return err;
> > }
> > return 0;
> > }
> > @@ -777,7 +834,7 @@ load_program(struct bpf_insn *insns, int insns_cnt,
> > goto out;
> > }
> >
> > - ret = -EINVAL;
> > + ret = -LIBBPF_ERRNO__ELOAD;
> > pr_warning("load bpf program failed: %s\n", strerror(errno));
> >
> > if (log_buf) {
> > @@ -831,7 +888,7 @@ static int bpf_object__validate(struct bpf_object *obj)
> > if (obj->kern_version == 0) {
> > pr_warning("%s doesn't provide kernel version\n",
> > obj->path);
> > - return -EINVAL;
> > + return -LIBBPF_ERRNO__EKVERSION;
> > }
> > return 0;
> > }
> > @@ -840,32 +897,28 @@ static struct bpf_object *
> > __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz)
> > {
> > struct bpf_object *obj;
> > + int err;
> >
> > if (elf_version(EV_CURRENT) == EV_NONE) {
> > pr_warning("failed to init libelf for %s\n", path);
> > - return NULL;
> > + return ERR_PTR(-LIBBPF_ERRNO__ELIBELF);
> > }
> >
> > obj = bpf_object__new(path, obj_buf, obj_buf_sz);
> > - if (!obj)
> > - return NULL;
> > + if (IS_ERR(obj))
> > + return obj;
> >
> > - if (bpf_object__elf_init(obj))
> > - goto out;
> > - if (bpf_object__check_endianness(obj))
> > - goto out;
> > - if (bpf_object__elf_collect(obj))
> > - goto out;
> > - if (bpf_object__collect_reloc(obj))
> > - goto out;
> > - if (bpf_object__validate(obj))
> > - goto out;
> > + CHECK_ERR(bpf_object__elf_init(obj), err, out);
> > + CHECK_ERR(bpf_object__check_endianness(obj), err, out);
> > + CHECK_ERR(bpf_object__elf_collect(obj), err, out);
> > + CHECK_ERR(bpf_object__collect_reloc(obj), err, out);
> > + CHECK_ERR(bpf_object__validate(obj), err, out);
> >
> > bpf_object__elf_finish(obj);
> > return obj;
> > out:
> > bpf_object__close(obj);
> > - return NULL;
> > + return ERR_PTR(err);
> > }
> >
> > struct bpf_object *bpf_object__open(const char *path)
> > @@ -922,6 +975,8 @@ int bpf_object__unload(struct bpf_object *obj)
> >
> > int bpf_object__load(struct bpf_object *obj)
> > {
> > + int err;
> > +
> > if (!obj)
> > return -EINVAL;
> >
> > @@ -931,18 +986,16 @@ int bpf_object__load(struct bpf_object *obj)
> > }
> >
> > obj->loaded = true;
> > - if (bpf_object__create_maps(obj))
> > - goto out;
> > - if (bpf_object__relocate(obj))
> > - goto out;
> > - if (bpf_object__load_progs(obj))
> > - goto out;
> > +
> > + CHECK_ERR(bpf_object__create_maps(obj), err, out);
> > + CHECK_ERR(bpf_object__relocate(obj), err, out);
> > + CHECK_ERR(bpf_object__load_progs(obj), err, out);
> >
> > return 0;
> > out:
> > bpf_object__unload(obj);
> > pr_warning("failed to load object '%s'\n", obj->path);
> > - return -EINVAL;
> > + return err;
> > }
> >
> > void bpf_object__close(struct bpf_object *obj)
> > @@ -990,7 +1043,7 @@ const char *
> > bpf_object__get_name(struct bpf_object *obj)
> > {
> > if (!obj)
> > - return NULL;
> > + return ERR_PTR(-EINVAL);
> > return obj->path;
> > }
> >
> > @@ -1043,7 +1096,7 @@ const char *bpf_program__title(struct bpf_program *prog, bool needs_copy)
> > title = strdup(title);
> > if (!title) {
> > pr_warning("failed to strdup program title\n");
> > - return NULL;
> > + return ERR_PTR(-ENOMEM);
> > }
> > }
> >
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index bc80af0..6491be6 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -10,6 +10,18 @@
> >
> > #include <stdio.h>
> > #include <stdbool.h>
> > +#include <linux/err.h>
> > +
> > +#define LIBBPF_ERRNO__START 4000
> > +#define LIBBPF_ERRNO__ELIBELF 4000 /* Something wrong in libelf */
> > +#define LIBBPF_ERRNO__EFORMAT 4001 /* BPF object format invalid */
> > +#define LIBBPF_ERRNO__EKVERSION 4002 /* Incorrect or no 'version' section */
> > +#define LIBBPF_ERRNO__EENDIAN 4003 /* Endian missmatch */
> > +#define LIBBPF_ERRNO__EINTERNAL 4004 /* Internal error in libbpf */
> > +#define LIBBPF_ERRNO__ERELOC 4005 /* Relocation failed */
> > +#define LIBBPF_ERRNO__ELOAD 4006 /* Failed to load program */
> > +
> > +int libbpf_strerror(int err, char *buf, size_t size);
> >
> > /*
> > * In include/linux/compiler-gcc.h, __printf is defined. However
> > diff --git a/tools/perf/tests/llvm.c b/tools/perf/tests/llvm.c
> > index 512d362..8f713f6 100644
> > --- a/tools/perf/tests/llvm.c
> > +++ b/tools/perf/tests/llvm.c
> > @@ -27,7 +27,7 @@ static int test__bpf_parsing(void *obj_buf, size_t obj_buf_sz)
> > struct bpf_object *obj;
> >
> > obj = bpf_object__open_buffer(obj_buf, obj_buf_sz, NULL);
> > - if (!obj)
> > + if (IS_ERR(obj))
> > return -1;
> > bpf_object__close(obj);
> > return 0;
> > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > index 0c5d174..72ad3dc 100644
> > --- a/tools/perf/util/bpf-loader.c
> > +++ b/tools/perf/util/bpf-loader.c
> > @@ -59,9 +59,9 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
> > } else
> > obj = bpf_object__open(filename);
> >
> > - if (!obj) {
> > + if (IS_ERR(obj)) {
> > pr_debug("bpf: failed to load %s\n", filename);
> > - return ERR_PTR(-EINVAL);
> > + return obj;
> > }
> >
> > return obj;
> > @@ -96,9 +96,9 @@ config_bpf_program(struct bpf_program *prog)
> > int err;
> >
> > config_str = bpf_program__title(prog, false);
> > - if (!config_str) {
> > + if (IS_ERR(config_str)) {
> > pr_debug("bpf: unable to get title for program\n");
> > - return -EINVAL;
> > + return PTR_ERR(config_str);
> > }
> >
> > priv = calloc(sizeof(*priv), 1);
> > @@ -308,13 +308,34 @@ int bpf__foreach_tev(struct bpf_object *obj,
> > return 0;
> > }
> >
> > +static int
> > +bpf_loader_strerror(int err, char *buf, size_t size)
> > +{
> > + char sbuf[STRERR_BUFSIZE];
> > + const char *msg;
> > +
> > + if (!buf || !size)
> > + return -1;
> > +
> > + err = err > 0 ? err : -err;
> > +
> > + if (err >= LIBBPF_ERRNO__START)
> > + return libbpf_strerror(err, buf, size);
> > +
> > + msg = strerror_r(err, sbuf, sizeof(sbuf));
> > + snprintf(buf, size, "%s", msg);
> > + buf[size - 1] = '\0';
> > + return 0;
> > +}
> > +
> > #define bpf__strerror_head(err, buf, size) \
> > char sbuf[STRERR_BUFSIZE], *emsg;\
> > if (!size)\
> > return 0;\
> > if (err < 0)\
> > err = -err;\
> > - emsg = strerror_r(err, sbuf, sizeof(sbuf));\
> > + bpf_loader_strerror(err, sbuf, sizeof(sbuf));\
> > + emsg = sbuf;\
> > switch (err) {\
> > default:\
> > scnprintf(buf, size, "%s", emsg);\
> > @@ -345,8 +366,9 @@ int bpf__strerror_load(struct bpf_object *obj __maybe_unused,
> > int err, char *buf, size_t size)
> > {
> > bpf__strerror_head(err, buf, size);
> > - bpf__strerror_entry(EINVAL, "%s: Are you root and runing a CONFIG_BPF_SYSCALL kernel?",
> > - emsg)
> > + bpf__strerror_entry(LIBBPF_ERRNO__ELOAD,
> > + "%s: Validate your program and check 'license'/'version' sections in your object",
> > + emsg);
> > bpf__strerror_end(buf, size);
> > return 0;
> > }
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index bee6058..c75b25d 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -632,11 +632,11 @@ int parse_events_load_bpf(struct parse_events_evlist *data,
> > struct bpf_object *obj;
> >
> > obj = bpf__prepare_load(bpf_file_name, source);
> > - if (IS_ERR(obj) || !obj) {
> > + if (IS_ERR(obj)) {
> > char errbuf[BUFSIZ];
> > int err;
> >
> > - err = obj ? PTR_ERR(obj) : -EINVAL;
> > + err = PTR_ERR(obj);
> >
> > if (err == -ENOTSUP)
> > snprintf(errbuf, sizeof(errbuf),
> > --
> > 1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/