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

[Matt Fleming <matt@codeblueprint.co.uk>]

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