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

[Jiri Olsa]

On Thu, Aug 20, 2015 at 02:52:01PM +0100, Matt Fleming wrote:

SNIP

> > 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.

hum, I haven't digested all of this thread yet, but I happened
to actually work on this recently - the tracepoint parsing
error propagation ... I did some initial patchset not ready
to be posted but working, please check it in:
  https://git.kernel.org/cgit/linux/kernel/git/jolsa/perf.git/log/?h=perf/tp_5

tracefs and debugfs just give location for accessing tracepoint,
the tracing_events_path is currently initialized to one of them

we could somehow prettyfy it, like unify all the related
interface to make it clear, like the debugfs__strerror_open
shouldn't be 'debugfs' specific and take tracefs into account
as in my patchset

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/