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

[Namhyung Kim]

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@xxxxxxxxx>
> >>---
> >>  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).


> 
> 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.

Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/