by Taeung Song

[permalink] [raw]

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

> On Aug 9, 2015, at 12:01 PM, Namhyung Kim <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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