From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932910AbbHIHy7 (ORCPT ); Sun, 9 Aug 2015 03:54:59 -0400 Received: from mail-pd0-f176.google.com ([209.85.192.176]:35308 "EHLO mail-pd0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754312AbbHIHy5 convert rfc822-to-8bit (ORCPT ); Sun, 9 Aug 2015 03:54:57 -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: <20150809030135.GB29295@danjae.kornet> Date: Sun, 9 Aug 2015 16:54:52 +0900 Cc: Arnaldo Carvalho de Melo , "linux-kernel@vger.kernel.org" , jolsa@redhat.com, Ingo Molnar Content-Transfer-Encoding: 8BIT Message-Id: 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> <20150809030135.GB29295@danjae.kornet> 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 > On Aug 9, 2015, at 12:01 PM, Namhyung Kim wrote: > > On Fri, Aug 07, 2015 at 10:12:02AM +0900, 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 ? > > Supposed you set ADD as 1 and DEL as 2. If you want do both action, > it'll have value of 3. But switch statement only have case 1 or 2 > (unless you give all possible combinations - but I don't think we want > it). > I understood what you said. Combinations of actions isn’t needed. So we don’t need to use OPT_BIT in perf-config. But I thought that if more than two separate or same actions is used error messages should be printed like “error: only one action at a time” like this. # perf config -l -l error: only one action at a time # perf config -l -a -r test.test error: only one action at a time Then using more than two actions should be blocked. I thought about 2 solutions 1) comparing original ‘argc' and ‘argc' after parse_options() work like this. int origin_argc = arc -1; argc = parse_options(argc, argv, config_options, config_usage, PARSE_OPT_STOP_AT_NON_OPTION); if (origin_argc != argc -1) pr_err(“error: only one action at a time\n"); 2) using OPT_BIT and HAS_MULTI_BIT() HAS_MULTI_BIT() can check whether more than two actions is used or not. Is needed this exception handing ? Or it isn’t needed ? > >> >> But I searched for other parse-option which can be replaced. >> Is it better to use OPT_SET_INT instead of OPT_BIT ? > > Please just use OPT_INTEGER. I modified source code to use OPT_INTEGER. But OPT_INTEGER require entering integer value like this. # perf config —list=1 or # perf config -l 1 So, I just use OPT_SET_UINT because it can have default value in contrast with OPT_INTEGER like this. static const struct option config_options[] = { OPT_GROUP(“Action”), OPT_SET_UINT(‘l’, “list”, &actions, “show current config variables”, ACTION_LIST), And I declared enum variable to distinguish between default value 0 and value of ACTION_LIST like this. enum actions { ACTION_LIST = 1, ACTION_LIST_ALL, ACTION_REMOVE } actions; Aren’t there problems if using OPT_SET_UINT instead of OPT_INTEGER ? Thanks, Taeung > > Thanks, > Namhyung