Message-ID: <20150820135201.GB2567@codeblueprint.co.uk>
Date:	Thu, 20 Aug 2015 14:52:01 +0100
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Raphaël Beamonte <raphael.beamonte@...il.com>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Yunlong Song <yunlong.song@...wei.com>,
	Matt Fleming <matt.fleming@...el.com>,
	Kan Liang <kan.liang@...el.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Hemant Kumar <hemant@...ux.vnet.ibm.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Wang Nan <wangnan0@...wei.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf: fix confusing messages when not able to read trace
 events files

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists