Re: [PATCH 2/5] perf tools: Improve BPF related error messages output

From: Arnaldo Carvalho de Melo
Date: Thu Nov 05 2015 - 10:34:37 EST

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH 1/5] bpf tools: Improve libbpf error reporting"
Previous message: Peter Zijlstra: "Re: [PATCH] sched,numa cap pte scanning overhead to 3% of run time"
In reply to: Wang Nan: "[PATCH 2/5] perf tools: Improve BPF related error messages output"
Next in thread: Wang Nan: "[PATCH 5/5] perf test: Add 'perf test BPF'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH 1/5] bpf tools: Improve libbpf error reporting"
Previous message: Peter Zijlstra: "Re: [PATCH] sched,numa cap pte scanning overhead to 3% of run time"
In reply to: Wang Nan: "[PATCH 2/5] perf tools: Improve BPF related error messages output"
Next in thread: Wang Nan: "[PATCH 5/5] perf test: Add 'perf test BPF'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]