Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Taewoong Song <treeze.taeung@gmail.com>]

> On Aug 9, 2015, at 12:01 PM, Namhyung Kim <namhyung@kernel.org> 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 <treeze.taeung@gmail.com>
>>>> ---
>>>> 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' [<file-option>] [section.name[=value] ...]
>>>> or
>>>> 'perf config' [<file-option>] -l | --list
>>>> +or
>>>> +'perf config' [<file-option>] -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