Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1208297 > unrolled thread

[PATCH] perf: bugzilla 100781

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf: bugzilla 100781 Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-17 03:40 +0200
    [PATCH] perf: fix confusing messages when not able to read trace events files Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-17 03:40 +0200
      Re: [PATCH] perf: fix confusing messages when not able to read trace  events files Matt Fleming <matt@codeblueprint.co.uk> - 2015-08-18 13:50 +0200
        Re: [PATCH] perf: fix confusing messages when not able to read trace  events files Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-18 19:50 +0200

#1208297 — [PATCH] perf: bugzilla 100781

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

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

[toc] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web