Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161523AbbKFOGy (ORCPT ); Fri, 6 Nov 2015 09:06:54 -0500 Received: from mail.kernel.org ([198.145.29.136]:37720 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161396AbbKFOGx (ORCPT ); Fri, 6 Nov 2015 09:06:53 -0500 Date: Fri, 6 Nov 2015 11:06:45 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: namhyung@kernel.org, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org, jolsa@kernel.org Subject: Re: [PATCH v2 1/7] bpf tools: Improve libbpf error reporting Message-ID: <20151106140645.GE13236@kernel.org> References: <1446817783-86722-1-git-send-email-wangnan0@huawei.com> <1446817783-86722-2-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446817783-86722-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: 19196 Lines: 617 Em Fri, Nov 06, 2015 at 01:49:37PM +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. > > load_program() is improved not to dump log buffer if it is empty. log > buffer is also used to deduce whether the error is caused by invalid > program or other problem. > > v1 -> v2: > > - Using macro for error code. > > - Fetch error message based on array index, eliminate for-loop. > > - Use log buffer to detect the reason of failure. 3 new error code > are introduced to replace LIBBPF_ERRNO__LOAD. Thanks a lot for taking into account, from what I quickly skimmed, all the review comments, really appreciated! Applying and retesting! - Arnaldo > In v1: > > # perf record -e ./test_ill_program.o ls > event syntax error: './test_ill_program.o' > \___ Failed to load program: Validate your program and check 'license'/'version' sections in your object > [SKIP] > > # perf record -e ./test_kversion_nomatch_program.o ls > event syntax error: './test_kversion_nomatch_program.o' > \___ Failed to load program: Validate your program and check 'license'/'version' sections in your object > [SKIP] > > # perf record -e ./test_big_program.o ls > event syntax error: './test_big_program.o' > \___ Failed to load program: Validate your program and check 'license'/'version' sections in your object > [SKIP] > > In v2: > > # perf record -e ./test_ill_program.o ls > event syntax error: './test_ill_program.o' > \___ Kernel verifier blocks program loading > [SKIP] > > # perf record -e ./test_kversion_nomatch_program.o > event syntax error: './test_kversion_nomatch_program.o' > \___ Incorrect kernel version > [SKIP] > (Will be further improved by following patches) > > # perf record -e ./test_big_program.o > event syntax error: './test_big_program.o' > \___ Program too big > [SKIP] > > Signed-off-by: Wang Nan > Cc: Arnaldo Carvalho de Melo > Cc: Namhyung Kim > --- > tools/lib/bpf/libbpf.c | 159 ++++++++++++++++++++++++++++------------- > tools/lib/bpf/libbpf.h | 20 ++++++ > tools/perf/tests/llvm.c | 2 +- > tools/perf/util/bpf-loader.c | 33 +++++++-- > tools/perf/util/parse-events.c | 4 +- > 5 files changed, 159 insertions(+), 59 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 9f3c8cf..07b492d 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -61,6 +61,60 @@ void libbpf_set_print(libbpf_print_fn_t warn, > __pr_debug = debug; > } > > +#define STRERR_BUFSIZE 128 > + > +#define ERRNO_OFFSET(e) ((e) - __LIBBPF_ERRNO__START) > +#define ERRCODE_OFFSET(c) ERRNO_OFFSET(LIBBPF_ERRNO__##c) > +#define NR_ERRNO (__LIBBPF_ERRNO__END - __LIBBPF_ERRNO__START) > + > +static const char *libbpf_strerror_table[NR_ERRNO] = { > + [ERRCODE_OFFSET(LIBELF)] = "Something wrong in libelf", > + [ERRCODE_OFFSET(FORMAT)] = "BPF object format invalid", > + [ERRCODE_OFFSET(KVERSION)] = "'version' section incorrect or lost", > + [ERRCODE_OFFSET(ENDIAN)] = "Endian missmatch", > + [ERRCODE_OFFSET(INTERNAL)] = "Internal error in libbpf", > + [ERRCODE_OFFSET(RELOC)] = "Relocation failed", > + [ERRCODE_OFFSET(VERIFY)] = "Kernel verifier blocks program loading", > + [ERRCODE_OFFSET(PROG2BIG)] = "Program too big", > + [ERRCODE_OFFSET(KVER)] = "Incorrect kernel version", > +}; > + > +int libbpf_strerror(int err, char *buf, size_t size) > +{ > + 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; > + } > + > + if (err < __LIBBPF_ERRNO__END) { > + const char *msg; > + > + msg = libbpf_strerror_table[ERRNO_OFFSET(err)]; > + 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 +312,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 +359,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__LIBELF; > } > > if (obj->efile.obj_buf_sz > 0) { > @@ -331,14 +385,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__LIBELF; > 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__FORMAT; > goto errout; > } > ep = &obj->efile.ehdr; > @@ -346,7 +400,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__FORMAT; > goto errout; > } > > @@ -374,14 +428,14 @@ bpf_object__check_endianness(struct bpf_object *obj) > goto mismatch; > break; > default: > - return -EINVAL; > + return -LIBBPF_ERRNO__ENDIAN; > } > > return 0; > > mismatch: > pr_warning("Error: endianness mismatch.\n"); > - return -EINVAL; > + return -LIBBPF_ERRNO__ENDIAN; > } > > static int > @@ -402,7 +456,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__FORMAT; > } > memcpy(&kver, data, sizeof(kver)); > obj->kern_version = kver; > @@ -444,7 +498,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__FORMAT; > } > > while ((scn = elf_nextscn(elf, scn)) != NULL) { > @@ -456,7 +510,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__FORMAT; > goto out; > } > > @@ -464,7 +518,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__FORMAT; > goto out; > } > > @@ -472,7 +526,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__FORMAT; > goto out; > } > pr_debug("section %s, size %ld, link %d, flags %lx, type=%d\n", > @@ -495,7 +549,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__FORMAT; > } else > obj->efile.symbols = data; > } else if ((sh.sh_type == SHT_PROGBITS) && > @@ -504,7 +558,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 +631,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__FORMAT; > } > > insn_idx = rel.r_offset / sizeof(struct bpf_insn); > @@ -587,20 +642,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__FORMAT; > } > > 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__RELOC; > } > > 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__RELOC; > } > > prog->reloc_desc[i].insn_idx = insn_idx; > @@ -683,7 +738,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__RELOC; > } > insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD; > insns[insn_idx].imm = map_fds[map_idx]; > @@ -721,7 +776,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__INTERNAL; > } > > for (i = 0; i < obj->efile.nr_reloc; i++) { > @@ -734,21 +789,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__INTERNAL; > } > > 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__RELOC; > } > > err = bpf_program__collect_reloc(prog, nr_maps, > shdr, data, > obj->efile.symbols); > if (err) > - return -EINVAL; > + return err; > } > return 0; > } > @@ -777,13 +832,23 @@ load_program(struct bpf_insn *insns, int insns_cnt, > goto out; > } > > - ret = -EINVAL; > + ret = -LIBBPF_ERRNO__LOAD; > pr_warning("load bpf program failed: %s\n", strerror(errno)); > > - if (log_buf) { > + if (log_buf && log_buf[0] != '\0') { > + ret = -LIBBPF_ERRNO__VERIFY; > pr_warning("-- BEGIN DUMP LOG ---\n"); > pr_warning("\n%s\n", log_buf); > pr_warning("-- END LOG --\n"); > + } else { > + if (insns_cnt >= BPF_MAXINSNS) { > + pr_warning("Program too large (%d insns), at most %d insns\n", > + insns_cnt, BPF_MAXINSNS); > + ret = -LIBBPF_ERRNO__PROG2BIG; > + } else if (log_buf) { > + pr_warning("log buffer is empty\n"); > + ret = -LIBBPF_ERRNO__KVER; > + } > } > > out: > @@ -831,7 +896,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__KVERSION; > } > return 0; > } > @@ -840,32 +905,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__LIBELF); > } > > 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 +983,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 +994,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 +1051,7 @@ const char * > bpf_object__get_name(struct bpf_object *obj) > { > if (!obj) > - return NULL; > + return ERR_PTR(-EINVAL); > return obj->path; > } > > @@ -1043,7 +1104,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..30a40e9 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -10,6 +10,26 @@ > > #include > #include > +#include > + > +enum libbpf_errno { > + __LIBBPF_ERRNO__START = 4000, > + > + /* Something wrong in libelf */ > + LIBBPF_ERRNO__LIBELF = __LIBBPF_ERRNO__START, > + LIBBPF_ERRNO__FORMAT, /* BPF object format invalid */ > + LIBBPF_ERRNO__KVERSION, /* Incorrect or no 'version' section */ > + LIBBPF_ERRNO__ENDIAN, /* Endian missmatch */ > + LIBBPF_ERRNO__INTERNAL, /* Internal error in libbpf */ > + LIBBPF_ERRNO__RELOC, /* Relocation failed */ > + LIBBPF_ERRNO__LOAD, /* Load program failure for unknown reason */ > + LIBBPF_ERRNO__VERIFY, /* Kernel verifier blocks program loading */ > + LIBBPF_ERRNO__PROG2BIG, /* Program too big */ > + LIBBPF_ERRNO__KVER, /* Incorrect kernel version */ > + __LIBBPF_ERRNO__END, > +}; > + > +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..c46256b 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,6 @@ 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_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/