Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752728AbbHRLnb (ORCPT ); Tue, 18 Aug 2015 07:43:31 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:37807 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752457AbbHRLn3 (ORCPT ); Tue, 18 Aug 2015 07:43:29 -0400 Date: Tue, 18 Aug 2015 12:43:26 +0100 From: Matt Fleming To: =?iso-8859-1?Q?Rapha=EBl?= Beamonte Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Ingo Molnar , Jiri Olsa , Adrian Hunter , Yunlong Song , Matt Fleming , Kan Liang , Steven Rostedt , Namhyung Kim , Hemant Kumar , Masami Hiramatsu , Wang Nan , linux-kernel@vger.kernel.org Subject: Re: [PATCH] perf: fix confusing messages when not able to read trace events files Message-ID: <20150818114326.GD3233@codeblueprint.co.uk> References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3996 Lines: 95 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 > --- > 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/