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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150824205155.GH3699@krava.redhat.com>
Date:	Mon, 24 Aug 2015 22:51:55 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Matt Fleming <matt@...eblueprint.co.uk>
Cc:	Raphaël Beamonte <raphael.beamonte@...il.com>,
	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 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/