Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Thu, 5 Nov 2015 12:34:28 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 2/5] perf tools: Improve BPF related error messages output |
| |
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 <wangnan0@huawei.com> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Namhyung Kim <namhyung@kernel.org> > --- > 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
| ![\](/images/icornerr.gif) |