Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754001AbbHXUwF (ORCPT ); Mon, 24 Aug 2015 16:52:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40489 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752296AbbHXUwC (ORCPT ); Mon, 24 Aug 2015 16:52:02 -0400 Date: Mon, 24 Aug 2015 22:51:55 +0200 From: Jiri Olsa To: Matt Fleming Cc: =?iso-8859-1?Q?Rapha=EBl?= Beamonte , Arnaldo Carvalho de Melo , Peter Zijlstra , Ingo Molnar , Jiri Olsa , Adrian Hunter , Yunlong Song , Matt Fleming , Kan Liang , Steven Rostedt , Namhyung Kim , Hemant Kumar , Masami Hiramatsu , Wang Nan , linux-kernel@vger.kernel.org Subject: Re: [PATCH] perf: fix confusing messages when not able to read trace events files Message-ID: <20150824205155.GH3699@krava.redhat.com> References: <20150818114326.GD3233@codeblueprint.co.uk> <20150820135201.GB2567@codeblueprint.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150820135201.GB2567@codeblueprint.co.uk> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2589 Lines: 55 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/