Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1033013AbbKEP0P (ORCPT ); Thu, 5 Nov 2015 10:26:15 -0500 Received: from mail.kernel.org ([198.145.29.136]:40319 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031459AbbKEP0N (ORCPT ); Thu, 5 Nov 2015 10:26:13 -0500 Date: Thu, 5 Nov 2015 12:26:07 -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: <20151105152607.GR13236@kernel.org> References: <1446697622-4072-1-git-send-email-wangnan0@huawei.com> <1446697622-4072-2-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446697622-4072-2-git-send-email-wangnan0@huawei.com> 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: 17754 Lines: 570 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: [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/