2015-08-18 7:43 GMT-04:00 Matt Fleming <m...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
