[PATCH] perf: bugzilla 100781

[PATCH] perf: bugzilla 100781

[Raphaël Beamonte]

Hi,

I tried myself at solving the bugzilla report 100781 that can be
found here: https://bugzilla.kernel.org/show_bug.cgi?id=100781

You'll find in the following email the patch I did for that.
I welcome any advice or remarks (or insults, but please be gentle,
I'm still a newbie!) to make that patch better in hope that it
could be used upstream and thus close that bug report!

Thanks,
Raphaël


Raphaël Beamonte (1):
  perf: fix confusing messages when not able to read trace events files

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

-- 
2.1.4

[PATCH] perf: fix confusing messages when not able to read trace events files

[Raphaël Beamonte]

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 <raphael.beamonte@gmail.com>
---
 tools/perf/util/parse-events.c  | 14 +++++++++++---
 tools/perf/util/parse-options.c |  6 ++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 09f8d23..17f787c 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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;
@@ -405,7 +406,10 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 	snprintf(evt_path, MAXPATHLEN, "%s/%s", tracing_events_path, sys_name);
 	evt_dir = opendir(evt_path);
 	if (!evt_dir) {
-		perror("Can't open event dir");
+		debugfs__strerror_open(
+			errno, errbuf, sizeof(errbuf),
+			evt_path + strlen(debugfs_mountpoint) + 1);
+		fprintf(stderr, "%s\n", errbuf);
 		return -1;
 	}
 
@@ -437,13 +441,17 @@ static int add_tracepoint_event(struct list_head *list, int *idx,
 static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 				    char *sys_name, char *evt_name)
 {
+	char errbuf[BUFSIZ];
 	struct dirent *events_ent;
 	DIR *events_dir;
 	int ret = 0;
 
 	events_dir = opendir(tracing_events_path);
 	if (!events_dir) {
-		perror("Can't open event dir");
+		debugfs__strerror_open(
+			errno, errbuf, sizeof(errbuf),
+			tracing_events_path + strlen(debugfs_mountpoint) + 1);
+		fprintf(stderr, "%s\n", errbuf);
 		return -1;
 	}
 
@@ -1156,7 +1164,7 @@ int parse_events_option(const struct option *opt, const char *str,
 	struct parse_events_error err = { .idx = 0, };
 	int ret = parse_events(evlist, str, &err);
 
-	if (ret)
+	if (ret && errno != EACCES)
 		parse_events_print_error(&err, str);
 
 	return ret;
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 01626be..55319d9 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -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)) {
 			case -1:
+				/* 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;
-- 
2.1.4

Re: [PATCH] perf: fix confusing messages when not able to read trace events files

[Matt Fleming]

On Sun, 16 Aug, at 09:39:12PM, Raphaël Beamonte wrote:
> 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 <raphael.beamonte@gmail.com>
> ---
>  tools/perf/util/parse-events.c  | 14 +++++++++++---
>  tools/perf/util/parse-options.c |  6 ++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 09f8d23..17f787c 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -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;
> @@ -405,7 +406,10 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
>  	snprintf(evt_path, MAXPATHLEN, "%s/%s", tracing_events_path, sys_name);
>  	evt_dir = opendir(evt_path);
>  	if (!evt_dir) {
> -		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?

> @@ -1156,7 +1164,7 @@ int parse_events_option(const struct option *opt, const char *str,
>  	struct parse_events_error err = { .idx = 0, };
>  	int ret = parse_events(evlist, str, &err);
>  
> -	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?

> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index 01626be..55319d9 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -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)) {
>  			case -1:
> +				/* 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?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] perf: fix confusing messages when not able to read trace events files

[Raphaël Beamonte]

2015-08-18 7:43 GMT-04:00 Matt Fleming <matt@codeblueprint.co.uk>:
>> -             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?

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?

>> -     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?

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


>>                       switch (parse_short_opt(ctx, options)) {
>>                       case -1:
>> +                             /* 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?

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;

Thanks!
Raphaël

Re: [PATCH] perf: fix confusing messages when not able to read trace events files

[Matt Fleming]

On Tue, 18 Aug, at 01:45:00PM, Raphaël Beamonte wrote:
> 
> 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.
 
Hehe, so many path names!

> 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?
 
I'll bet that most people on the Cc list are at Linux Plumbers of
LinuxCon NA this week, so take this suggestion with a pinch of salt:

What about something like this, just as a random idea?

diff --git a/tools/lib/api/fs/debugfs.c b/tools/lib/api/fs/debugfs.c
index 8305b3e9d48e..b8cbdf656d69 100644
--- a/tools/lib/api/fs/debugfs.c
+++ b/tools/lib/api/fs/debugfs.c
@@ -108,11 +108,20 @@ int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename
 	return 0;
 }
 
-int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
+int debugfs__strerror_open_tp_sys(int err, char *buf, size_t size, const char *sys_name)
 {
 	char path[PATH_MAX];
 
-	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
+	snprintf(path, PATH_MAX, "tracing/events/%s", sys_name ?: "");
 
 	return debugfs__strerror_open(err, buf, size, path);
 }
+
+int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
+{
+	char sys_name[PATH_MAX];
+
+	snprintf(sys_name, PATH_MAX, "%s/%s", sys, name ?: "*");
+
+	return debugfs__strerror_open_tp_sys(err, buf, size, sys_name);
+}

> 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.

> >>                       switch (parse_short_opt(ctx, options)) {
> >>                       case -1:
> >> +                             /* 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?
> 
> 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;

The code you're referring to (munging the callback return value) was
taken from git, and was introduced with the following commit

 https://git.kernel.org/cgit/git/git.git/commit/?id=07fe54db3cdf42500ac2e893b670fd74841afdc4

I think you can safely refactor this to not munge the callback return
values.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] perf: fix confusing messages when not able to read trace events files

[Jiri Olsa]

On Thu, Aug 20, 2015 at 02:52:01PM +0100, Matt Fleming wrote:

SNIP

> > 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.

hum, I haven't digested all of this thread yet, but I happened
to actually work on this recently - the tracepoint parsing
error propagation ... I did some initial patchset not ready
to be posted but working, please check it in:
  https://git.kernel.org/cgit/linux/kernel/git/jolsa/perf.git/log/?h=perf/tp_5

tracefs and debugfs just give location for accessing tracepoint,
the tracing_events_path is currently initialized to one of them

we could somehow prettyfy it, like unify all the related
interface to make it clear, like the debugfs__strerror_open
shouldn't be 'debugfs' specific and take tracefs into account
as in my patchset

jirka