From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932754AbbHHJvH (ORCPT ); Sat, 8 Aug 2015 05:51:07 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:34601 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932257AbbHHJvE convert rfc822-to-8bit (ORCPT ); Sat, 8 Aug 2015 05:51:04 -0400 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2070.6\)) Subject: Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config From: Taewoong Song In-Reply-To: <55C405E2.3060908@gmail.com> Date: Sat, 8 Aug 2015 18:50:59 +0900 Cc: Arnaldo Carvalho de Melo , "linux-kernel@vger.kernel.org" , jolsa@redhat.com, Ingo Molnar Content-Transfer-Encoding: 8BIT Message-Id: <9C7D1EDC-754E-4B4A-8986-8435BF5B343E@gmail.com> References: <1437926311-16226-1-git-send-email-treeze.taeung@gmail.com> <1437926311-16226-5-git-send-email-treeze.taeung@gmail.com> <20150727084847.GD22022@danjae.kornet> <55C405E2.3060908@gmail.com> To: Namhyung Kim X-Mailer: Apple Mail (2.2070.6) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Namhyung > On Aug 7, 2015, at 10:12 AM, taeung wrote: > > Hi, Namhyung > > On 07/27/2015 05:48 PM, Namhyung Kim wrote: >> On Mon, Jul 27, 2015 at 12:58:30AM +0900, Taeung Song wrote: >>> A option 'list-all' is to display both current config variables and >>> all possible config variables with default values. >>> The syntax examples are like below >>> >>> perf config [options] >>> >>> display all perf config with default values. >>> # perf config -a | --list-all >>> >>> Signed-off-by: Taeung Song >>> --- >>> tools/perf/Documentation/perf-config.txt | 6 ++++ >>> tools/perf/builtin-config.c | 48 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 54 insertions(+) >>> >>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt >>> index cd4b1a6..d8b3acc 100644 >>> --- a/tools/perf/Documentation/perf-config.txt >>> +++ b/tools/perf/Documentation/perf-config.txt >>> @@ -11,6 +11,8 @@ SYNOPSIS >>> 'perf config' [] [section.name[=value] ...] >>> or >>> 'perf config' [] -l | --list >>> +or >>> +'perf config' [] -a | --list-all >>> DESCRIPTION >>> ----------- >>> @@ -31,6 +33,10 @@ OPTIONS >>> For writing and reading options: write to system-wide >>> '$(sysconfdir)/perfconfig' or read it. >>> +-a:: >>> +--list-all:: >>> + Show current and all possible config variables with default values. >>> + >>> CONFIGURATION FILE >>> ------------------ >>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c >>> index 6d9f28c..f4a1569 100644 >>> --- a/tools/perf/builtin-config.c >>> +++ b/tools/perf/builtin-config.c >>> @@ -23,6 +23,7 @@ static const char * const config_usage[] = { >>> }; >>> #define ACTION_LIST (1<<0) >>> +#define ACTION_LIST_ALL (1<<1) >>> static const struct option config_options[] = { >>> OPT_GROUP("Config file location"), >>> @@ -31,6 +32,8 @@ static const struct option config_options[] = { >>> OPT_GROUP("Action"), >>> OPT_BIT('l', "list", &actions, >>> "show current config variables", ACTION_LIST), >>> + OPT_BIT('a', "list-all", &actions, >>> + "show current and all possible config variables with default values", ACTION_LIST_ALL), >> Why did you use OPT_BIT? Do you want to support multiple 'actions' at >> the same time? I'd rather support just one action, but I won't insist >> it strongly.. Anyway, setting bits will confuse the switch statement >> in the cmd_config(). >> >> Thanks, >> Namhyung >> > I don't understand why setting bits will confuse the switch statement. > Is the reason about readability of source code ? > > But I searched for other parse-option which can be replaced. > Is it better to use OPT_SET_INT instead of OPT_BIT ? > I modified source code to use OPT_SET_UINT but the problem is happened. I declared "enum actions” to use OPT_SET_UINT like this. enum actions { ACTION_LIST, ACTION_LIST_ALL, ACTION_REMOVE } actions; But default value of actions” variable is 0 and ACTION_LIST is also 0 so when ‘get’ or ’set’ feature is used, ‘default:' of the switch statement in cmd_config() can’t reached because of default value of ‘actions’ variable is 0. Use other parse-option function instead of OPT_SET_UINT ? Thanks, Taeung > Thanks, > Taeung > >>> OPT_END() >>> }; >>> @@ -539,6 +542,45 @@ static int collect_current_config(const char *var, const char *value, >>> normalize_value(section_name, name, value)); >>> } >>> +static int show_all_config(void) >>> +{ >>> + int i; >>> + bool has_config; >>> + struct config_section *section_node; >>> + struct config_element *element_node; >>> + >>> + for (i = 0; default_configsets[i].section_name != NULL; i++) { >>> + find_config(§ion_node, &element_node, >>> + default_configsets[i].section_name, default_configsets[i].name); >>> + >>> + if (!element_node) >>> + printf("%s.%s=%s\n", default_configsets[i].section_name, >>> + default_configsets[i].name, default_configsets[i].value); >>> + else >>> + printf("%s.%s=%s\n", section_node->name, >>> + element_node->name, element_node->value); >>> + } >>> + >>> + /* Print config variables the default configsets haven't */ >>> + list_for_each_entry(section_node, §ions, list) { >>> + list_for_each_entry(element_node, §ion_node->element_head, list) { >>> + has_config = false; >>> + for (i = 0; default_configsets[i].section_name != NULL; i++) { >>> + if (!strcmp(default_configsets[i].section_name, section_node->name) >>> + && !strcmp(default_configsets[i].name, element_node->name)) { >>> + has_config = true; >>> + break; >>> + } >>> + } >>> + if (!has_config) >>> + printf("%s.%s=%s\n", section_node->name, >>> + element_node->name, element_node->value); >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value) >>> { >>> char *section_name; >>> @@ -617,6 +659,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) >>> else >>> goto out_err; >>> goto out; >>> + case ACTION_LIST_ALL: >>> + if (argc == 0) >>> + ret = show_all_config(); >>> + else >>> + goto out_err; >>> + goto out; >>> default: >>> if ((!has_option || use_global_config || use_system_config) >>> && argc == 0) { >>> -- >>> 1.9.1 >>> > > -- > Thanks, > Taeung >