Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753787AbbHRRpV (ORCPT ); Tue, 18 Aug 2015 13:45:21 -0400 Received: from mail-yk0-f169.google.com ([209.85.160.169]:33078 "EHLO mail-yk0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752705AbbHRRpU convert rfc822-to-8bit (ORCPT ); Tue, 18 Aug 2015 13:45:20 -0400 MIME-Version: 1.0 In-Reply-To: <20150818114326.GD3233@codeblueprint.co.uk> References: <20150818114326.GD3233@codeblueprint.co.uk> From: =?UTF-8?Q?Rapha=C3=ABl_Beamonte?= Date: Tue, 18 Aug 2015 13:45:00 -0400 X-Google-Sender-Auth: w1VIzBXPVfkIEmZIsjo_5pYWymo Message-ID: Subject: Re: [PATCH] perf: fix confusing messages when not able to read trace events files To: Matt Fleming Cc: 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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4835 Lines: 99 2015-08-18 7:43 GMT-04:00 Matt Fleming : >> - 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/