Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162086AbbKEPfT (ORCPT ); Thu, 5 Nov 2015 10:35:19 -0500 Received: from mail.kernel.org ([198.145.29.136]:40911 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031333AbbKEPfS (ORCPT ); Thu, 5 Nov 2015 10:35:18 -0500 Date: Thu, 5 Nov 2015 12:35:11 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: namhyung@kernel.org, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org, Ingo Molnar , Jiri Olsa , Alexei Starovoitov Subject: Re: [PATCH 1/5] bpf tools: Improve libbpf error reporting Message-ID: <20151105153511.GT13236@kernel.org> References: <1446697622-4072-1-git-send-email-wangnan0@huawei.com> <1446697622-4072-2-git-send-email-wangnan0@huawei.com> <20151105152607.GR13236@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151105152607.GR13236@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 19055 Lines: 575 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 > > Cc: Arnaldo Carvalho de Melo > > Cc: Namhyung Kim > > --- > > 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 > > #include > > +#include > > + > > +#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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/