Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162068AbbKEPef (ORCPT ); Thu, 5 Nov 2015 10:34:35 -0500 Received: from mail.kernel.org ([198.145.29.136]:40866 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161963AbbKEPed (ORCPT ); Thu, 5 Nov 2015 10:34:33 -0500 Date: Thu, 5 Nov 2015 12:34:28 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: namhyung@kernel.org, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/5] perf tools: Improve BPF related error messages output Message-ID: <20151105153428.GS13236@kernel.org> References: <1446697622-4072-1-git-send-email-wangnan0@huawei.com> <1446697622-4072-3-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-3-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: 9734 Lines: 276 Em Thu, Nov 05, 2015 at 04:26:59AM +0000, Wang Nan escreveu: > A series of bpf loader related error code is introduced to help error > delivering. Functions are improved to return those new error code. > Functions which return pointers are adjusted to encode error code into > return value using "ERR_PTR". So: > +#define BPF_LOADER_ERRNO__START 3900 This is done in terms of LIBBPF_ERRNO__START, right? Why not do this as an enum and also, again, look at tools/perf/util/target.h and see how it is done, then you can do something like: > +#define BPF_LOADER_ERRNO__START 3900 > +#define BPF_LOADER_ERRNO__ECONFIG 3900 /* Invalid config string */ > +#define BPF_LOADER_ERRNO__EGROUP 3901 /* Invalid group name */ > +#define BPF_LOADER_ERRNO__EEVENTNAME 3902 /* Event name is missing */ > +#define BPF_LOADER_ERRNO__EINTERNAL 3903 /* BPF loader internal > error */ > +#define BPF_LOADER_ERRNO__ECOMPILE 3904 /* Error when compiling BPF > scriptlet */ > +#define BPF_LOADER_ERRNO__END 3905 enum bpf_loader_errno { __BPF_LOADER_ERRNO__START = (LIBBPF_ERRNO__START - 100), BPF_LOADER_ERRNO__CONFIG = __BPF_LOADER_ERRNO__START, BPF_LOADER_ERRNO__CONFIG /* Invalid config string */ BPF_LOADER_ERRNO__GROUP /* Invalid group name */ BPF_LOADER_ERRNO__EVENTNAME /* Event name is missing */ BPF_LOADER_ERRNO__INTERNAL /* BPF loader internal error */ BPF_LOADER_ERRNO__COMPILE /* Error when compiling BPF > scriptlet */ __BPF_LOADER_ERRNO__END, }; In the process I removed that extra 'E', heck, we have BPF_LOADER_ERRNO__, that should be enough indication that this is an ERRNO code, no? 8-) So, I'm removing the first patch as well, better get that one right from start. - Arnaldo > bpf_loader_strerror() is improved to convert those error message to > string. It detected the value of error code and calls libbpf_strerror() > and strerror_r() accordingly, so caller don't need to consider checking > the range of error code. > > Signed-off-by: Wang Nan > Cc: Arnaldo Carvalho de Melo > Cc: Namhyung Kim > --- > tools/perf/util/bpf-loader.c | 70 +++++++++++++++++++++++++++++++++++++----- > tools/perf/util/bpf-loader.h | 18 +++++++++++ > tools/perf/util/parse-events.c | 7 +++-- > 3 files changed, 84 insertions(+), 11 deletions(-) > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c > index 72ad3dc..4b9bb2a 100644 > --- a/tools/perf/util/bpf-loader.c > +++ b/tools/perf/util/bpf-loader.c > @@ -14,6 +14,10 @@ > #include "probe-finder.h" // for MAX_PROBES > #include "llvm-utils.h" > > +#if BPF_LOADER_ERRNO__END >= LIBBPF_ERRNO__START > +# error Too many BPF loader error code > +#endif > + > #define DEFINE_PRINT_FN(name, level) \ > static int libbpf_##name(const char *fmt, ...) \ > { \ > @@ -53,7 +57,7 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source) > > err = llvm__compile_bpf(filename, &obj_buf, &obj_buf_sz); > if (err) > - return ERR_PTR(err); > + return ERR_PTR(-BPF_LOADER_ERRNO__ECOMPILE); > obj = bpf_object__open_buffer(obj_buf, obj_buf_sz, filename); > free(obj_buf); > } else > @@ -113,14 +117,14 @@ config_bpf_program(struct bpf_program *prog) > if (err < 0) { > pr_debug("bpf: '%s' is not a valid config string\n", > config_str); > - err = -EINVAL; > + err = -BPF_LOADER_ERRNO__ECONFIG; > goto errout; > } > > if (pev->group && strcmp(pev->group, PERF_BPF_PROBE_GROUP)) { > pr_debug("bpf: '%s': group for event is set and not '%s'.\n", > config_str, PERF_BPF_PROBE_GROUP); > - err = -EINVAL; > + err = -BPF_LOADER_ERRNO__EGROUP; > goto errout; > } else if (!pev->group) > pev->group = strdup(PERF_BPF_PROBE_GROUP); > @@ -132,9 +136,9 @@ config_bpf_program(struct bpf_program *prog) > } > > if (!pev->event) { > - pr_debug("bpf: '%s': event name is missing\n", > + pr_debug("bpf: '%s': event name is missing. Section name should be 'key=value'\n", > config_str); > - err = -EINVAL; > + err = -BPF_LOADER_ERRNO__EEVENTNAME; > goto errout; > } > pr_debug("bpf: config '%s' is ok\n", config_str); > @@ -285,7 +289,7 @@ int bpf__foreach_tev(struct bpf_object *obj, > (void **)&priv); > if (err || !priv) { > pr_debug("bpf: failed to get private field\n"); > - return -EINVAL; > + return -BPF_LOADER_ERRNO__EINTERNAL; > } > > pev = &priv->pev; > @@ -308,11 +312,25 @@ int bpf__foreach_tev(struct bpf_object *obj, > return 0; > } > > +#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) > + > +struct { > + int code; > + const char *msg; > +} bpf_loader_strerror_table[] = { > + {BPF_LOADER_ERRNO__ECONFIG, "Invalid config string"}, > + {BPF_LOADER_ERRNO__EGROUP, "Invalid group name"}, > + {BPF_LOADER_ERRNO__EEVENTNAME, "No event name found in config string"}, > + {BPF_LOADER_ERRNO__EINTERNAL, "BPF loader internal error"}, > + {BPF_LOADER_ERRNO__ECOMPILE, "Error when compiling BPF scriptlet"}, > +}; > + > static int > bpf_loader_strerror(int err, char *buf, size_t size) > { > char sbuf[STRERR_BUFSIZE]; > const char *msg; > + unsigned int i; > > if (!buf || !size) > return -1; > @@ -322,6 +340,21 @@ bpf_loader_strerror(int err, char *buf, size_t size) > if (err >= LIBBPF_ERRNO__START) > return libbpf_strerror(err, buf, size); > > + if (err >= BPF_LOADER_ERRNO__START) { > + for (i = 0; i < ARRAY_SIZE(bpf_loader_strerror_table); i++) { > + if (bpf_loader_strerror_table[i].code == err) { > + msg = bpf_loader_strerror_table[i].msg; > + snprintf(buf, size, "%s", msg); > + buf[size - 1] = '\0'; > + return 0; > + } > + } > + > + snprintf(buf, size, "Unknown bpf loader error %d", err); > + buf[size - 1] = '\0'; > + return -1; > + } > + > msg = strerror_r(err, sbuf, sizeof(sbuf)); > snprintf(buf, size, "%s", msg); > buf[size - 1] = '\0'; > @@ -351,13 +384,34 @@ bpf_loader_strerror(int err, char *buf, size_t size) > }\ > buf[size - 1] = '\0'; > > +int bpf__strerror_prepare_load(const char *filename, bool source, > + int err, char *buf, size_t size) > +{ > + size_t n; > + int ret; > + > + n = snprintf(buf, size, "Failed to load %s%s: ", > + filename, source ? " from source" : ""); > + if (n >= size) { > + buf[size - 1] = '\0'; > + return 0; > + } > + buf += n; > + size -= n; > + > + ret = bpf_loader_strerror(err, buf, size); > + buf[size - 1] = '\0'; > + return ret; > +} > + > int bpf__strerror_probe(struct bpf_object *obj __maybe_unused, > int err, char *buf, size_t size) > { > bpf__strerror_head(err, buf, size); > bpf__strerror_entry(EEXIST, "Probe point exist. Try use 'perf probe -d \"*\"'"); > - bpf__strerror_entry(EPERM, "You need to be root, and /proc/sys/kernel/kptr_restrict should be 0\n"); > - bpf__strerror_entry(ENOENT, "You need to check probing points in BPF file\n"); > + bpf__strerror_entry(EACCES, "You need to be root"); > + bpf__strerror_entry(EPERM, "You need to be root, and /proc/sys/kernel/kptr_restrict should be 0"); > + bpf__strerror_entry(ENOENT, "You need to check probing points in BPF file"); > bpf__strerror_end(buf, size); > return 0; > } > diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h > index ccd8d7f..490c78c 100644 > --- a/tools/perf/util/bpf-loader.h > +++ b/tools/perf/util/bpf-loader.h > @@ -11,6 +11,14 @@ > #include "probe-event.h" > #include "debug.h" > > +#define BPF_LOADER_ERRNO__START 3900 > +#define BPF_LOADER_ERRNO__ECONFIG 3900 /* Invalid config string */ > +#define BPF_LOADER_ERRNO__EGROUP 3901 /* Invalid group name */ > +#define BPF_LOADER_ERRNO__EEVENTNAME 3902 /* Event name is missing */ > +#define BPF_LOADER_ERRNO__EINTERNAL 3903 /* BPF loader internal error */ > +#define BPF_LOADER_ERRNO__ECOMPILE 3904 /* Error when compiling BPF scriptlet */ > +#define BPF_LOADER_ERRNO__END 3905 > + > struct bpf_object; > #define PERF_BPF_PROBE_GROUP "perf_bpf_probe" > > @@ -19,6 +27,8 @@ typedef int (*bpf_prog_iter_callback_t)(struct probe_trace_event *tev, > > #ifdef HAVE_LIBBPF_SUPPORT > struct bpf_object *bpf__prepare_load(const char *filename, bool source); > +int bpf__strerror_prepare_load(const char *filename, bool source, > + int err, char *buf, size_t size); > > void bpf__clear(void); > > @@ -67,6 +77,14 @@ __bpf_strerror(char *buf, size_t size) > return 0; > } > > +int bpf__strerror_prepare_load(const char *filename __maybe_unused, > + bool source __maybe_unused, > + int err __maybe_unused, > + char *buf, size_t size) > +{ > + return __bpf_strerror(buf, size); > +} > + > static inline int > bpf__strerror_probe(struct bpf_object *obj __maybe_unused, > int err __maybe_unused, > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index c75b25d..e48d9da 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -642,9 +642,10 @@ int parse_events_load_bpf(struct parse_events_evlist *data, > snprintf(errbuf, sizeof(errbuf), > "BPF support is not compiled"); > else > - snprintf(errbuf, sizeof(errbuf), > - "BPF object file '%s' is invalid", > - bpf_file_name); > + bpf__strerror_prepare_load(bpf_file_name, > + source, > + -err, errbuf, > + sizeof(errbuf)); > > data->error->help = strdup("(add -v to see detail)"); > data->error->str = strdup(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/