Date:	Tue, 18 Aug 2015 13:45:00 -0400
From:	Raphaël Beamonte <raphael.beamonte@...il.com>
To:	Matt Fleming <matt@...eblueprint.co.uk>
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

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