perf: bugzilla 100781

If a non-root user tries to specify a trace event and the tracefs files can't be read, it will tell about it in a somewhat cryptic way and as well say that the tracepoint is unknown, which is obvious, since the tracefs files were not read.

This patch changes this behavior by using the debugfs__strerror_open function to report the access error in a more elegant way, as well as provide a hint like the one provided by the perf trace tool.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=100781

Signed-off-by Raphaël Beamonte <[email protected]>

---
 tools/perf/util/parse-events.c  | 14 +++++++++++---
 tools/perf/util/parse-options.c |  6 ++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

tools/perf/util/parse-events.c +1 -0

index 09f8d23..17f787c 100644
@@ -398,6 +398,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 				      char *sys_name, char *evt_name)
 {
 	char evt_path[MAXPATHLEN];
+	char errbuf[BUFSIZ];
 	struct dirent *evt_ent;
 	DIR *evt_dir;
 	int ret = 0;

tools/perf/util/parse-options.c +0 -0

index 01626be..55319d9 100644
@@ -400,6 +400,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 				return usage_with_options_internal(usagestr, options, 0);
 			switch (parse_short_opt(ctx, options)) {

tools/perf/util/parse-events.c static int add_tracepoint_multi_event(struct list_head *list, int *idx, · line 411

Quoted 4 lines

tools/perf/util/parse-events.c +3 -1

-             perror("Can't open event dir");
+             debugfs__strerror_open(
+                     errno, errbuf, sizeof(errbuf),
+                     evt_path + strlen(debugfs_mountpoint) + 1);

The way the filename is passed seems a bit hacky. What's wrong with calling debugfs__strerror_open_tp() instead?

tools/perf/util/parse-events.c int parse_events_option(const struct option *opt, const char *str, · line 1168

Quoted 4 lines

tools/perf/util/parse-events.c +1 -1

-     if (ret)
+     if (ret && errno != EACCES)
              parse_events_print_error(&err, str);

This is not a scalable solution. As more and more errors are handled at the caller the "if (errno != FOO)" expression will grow to be too large. There's also another problem in that you can't be sure 'errno' hasn't been modified by the time you reach this point, since it's a global variable and available for any code to modify.

This is taken straight from the errno(3) man page,

"Its value is significant only when the return value of the call
indicated an error (i.e., -1 from most system calls; -1 or NULL from most library functions); a function that succeeds is allowed to change errno."

Is there some way to pass the error message back up the stack in &err and not call fprintf() from add_tracepoint_multi_event() etc?

tools/perf/util/parse-options.c int parse_options_step(struct parse_opt_ctx_t *ctx, · line 411

Quoted 11 lines

                      switch (parse_short_opt(ctx, options)) {
                      case -1:

tools/perf/util/parse-options.c +6 -0

+                             /* If the error is an access error, we should already have
+                              * taken care of it, and the usage information will provide
+                              * no help to the user.
+                              */
+                             if (errno == EACCES)
+                                     return -1;
                              return parse_options_usage(usagestr, options, arg, 1);
                      case -2:
                              goto unknown;

Same comment applies here about using errno. Maybe what we want is a new return code to signal "the caller has already printed informative messages, so just return", if none of the existing values make sense?

debugfs__strerror_open_tp is using that call to form the path:

        snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");

Where for add_tracepoint_multi_sys we just need the tracing/events part, and for add_tracepoint_multi_event we just need tracing/events/%s. It is thus not adapted for what we need here. Moreover, to get those paths, I have to get the tracing/events part (I didn't want to hardcode it, as the tracing_events_path contains it) and, in the second case only, the sys_name. The problem with the tracing_events variable is that it contains the debugfs mountpoint part (it's an absolute path, not relative, and is thus hardcoded even though the debugfs_mountpoint contains the debugfs mountpoint absolute path). This is why it ends up being the way it is in my patch.

I think the tracing_events_path has been made that way to avoid building paths with snprintf each time we needed to access directly the tracing/events dir. I don't know if changing the tracing_events_path variable to a relative path would be acceptable? If so, it would clearly clean up the path in that debugfs__strerror_open call. Thoughts?

The err variable doesn't go down to the add_tracepoint_multi_event() call. It actually stops in parse_events_parse() where parse_events_add_tracepoint is being called using only the idx part of data (util/parse-events.y:389). I think it would be possible to pass the whole data variable (struct parse_events_evlist) down those variables to still have access to &err, but it would imply quite a lot of changes in there. I'm up to it though, if it seems that's the right thing to do! What is your take on

Quoted 12 lines

Review

Quoted 11 lines

                      switch (parse_short_opt(ctx, options)) {
                      case -1:

(quoted diff) +6 -0

+                             /* If the error is an access error, we should already have
+                              * taken care of it, and the usage information will provide
+                              * no help to the user.
+                              */
+                             if (errno == EACCES)
+                                     return -1;
                              return parse_options_usage(usagestr, options, arg, 1);
                      case -2:
                              goto unknown;

Quoted 3 lines

Same comment applies here about using errno. Maybe what we want is a new return code to signal "the caller has already printed informative messages, so just return", if none of the existing values make sense?

Would also need code refactoring: parse_short_opt calls get_value that calls parse_events_option, but unfortunately get_value drops the return code of parse_events_option to return only -1 on fail and 0 on success (parse-options.c:142 in the case OPTION_CALLBACK). I think it's mostly to prevent mistakes with the callback function return code and the get_value/parse_short_opt return codes (0, -1, -3 for get_value, -2 or the get_value return code for parse_short_opt). How would you see a good manner of refactoring that?
Catch only a specific return code in get_value that could be returned instead of -1 when it is met ? For instance:

        ret = (*opt->callback)(opt, NULL, 0);
        if (ret == -4)
                return ret;
        return (ret) ? (-1) : 0;

Quoted 8 lines

The err variable doesn't go down to the add_tracepoint_multi_event() call. It actually stops in parse_events_parse() where parse_events_add_tracepoint is being called using only the idx part of data (util/parse-events.y:389). I think it would be possible to pass the whole data variable (struct parse_events_evlist) down those variables to still have access to &err, but it would imply quite a lot of changes in there. I'm up to it though, if it seems that's the right thing to do! What is your take on

You bring up a good point. Perf would benefit greatly from easy access to a struct parse_events_error variable, so that it isn't required to pass it as an argument to every function.

Now, I know that generally global variables are frowned upon but I think an exception can be made for error handling, because, assuming errors are fatal (and if they're not they're called warnings), you should never have multiple things writing to it at the same time, and you should only ever execute error paths once you've written to it.

And if you really, really don't like naked accesses to global variables you can always use a wrapper function.

With global access to error data it becomes trivial to improve the error handling of other functions in a piecemeal way, without requiring changes to every function in the callstack. No one likes reviewing large patches ;-)

I would suggest setting up a global struct parse_events_error object, and making changes to parse_events_print_error() to handle non-parse related errors, such as ENOMEM, ENOENT, etc, etc.