Re: [PATCH 1/4] perf config: Introduce default_config_item for all default config key-value pairs

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

Hi, Arnaldo, Namhyung and jirka :)

On 05/10/2016 02:17 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, May 09, 2016 at 08:41:46PM +0900, Taeung Song escreveu:
>> We currently use values of actual type(int, bool, char *, etc.)
>> when initializing default perf config values.
>>
>> But I suggest using new config arrays(default_config_items)
>> that have all default perf config key-value pairs.
>>
>> Because if we do, we can manage default perf config values at one spot (like util/config.c)
>> and it can be easy and simple to modify default config values or add new configs.
>>
>> In the near future, this could be used when
>> showing all configs with default values,
>> initializing default values of each actual config variable
>> with the default_config_item arrays and etc.
>
> looks ok

Thank you for your review !! :-)

We can use this patch as it is, but what about this change ?

diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 84414af..8e143fb 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -5,6 +5,9 @@
  #include <linux/list.h>

  enum perf_config_type {
+        /* special type */
+        CONFIG_END,
+
          CONFIG_TYPE_BOOL,
          CONFIG_TYPE_INT,
          CONFIG_TYPE_LONG,
@@ -14,8 +17,10 @@ enum perf_config_type {
          CONFIG_TYPE_STRING
  };

+#define MAX_CONFIG_NAME 100
+
  struct default_config_item {
-        const char *name;
+        const char name[MAX_CONFIG_NAME];
          union {
                  bool b;
                  int i;
@@ -62,7 +67,7 @@ struct perf_config_set {
  #define CONF_STR_VAR(_name, _val)                       \
          CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
  #define CONF_END()                                      \
-        { .name = NULL }
+        { .type = CONFIG_END }



Because if we do, we can use name of default_config_item
as a constant to replace values of actual type(int, char *, etc)
with colors_config_items[].name more easy e.g.

(Contrastively if we use 'name' member variable as 'const char *' type,
we can't use it as a constant like below. It is a limitation of C language.)


diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 385d0de..207c62d 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -508,31 +508,31 @@ static struct ui_browser_colorset {
  } ui_browser__colorsets[] = {
          {
                  .colorset = HE_COLORSET_TOP,
-                .name     = "top",
+                .name     = colors_config_items[CONFIG_COLORS_TOP].name,
          },
          {
                  .colorset = HE_COLORSET_MEDIUM,
-                .name     = "medium",
+                .name     = colors_config_items[CONFIG_COLORS_MEDIUM].name,
          },
          {
                  .colorset = HE_COLORSET_NORMAL,
-                .name     = "normal",
+                .name     = colors_config_items[CONFIG_COLORS_NORMAL].name,
          },
          {
                  .colorset = HE_COLORSET_SELECTED,
-                .name     = "selected",
+                .name     = 
colors_config_items[CONFIG_COLORS_SELECTED].name,
          },
          {
                  .colorset = HE_COLORSET_JUMP_ARROWS,
-                .name     = "jump_arrows",
+                .name     = 
colors_config_items[CONFIG_COLORS_JUMP_ARROWS].name,
          },
          {
                  .colorset = HE_COLORSET_ADDR,
-                .name     = "addr",
+                .name     = colors_config_items[CONFIG_COLORS_ADDR].name,
          },
          {
                  .colorset = HE_COLORSET_ROOT,
-                .name     = "root",
+                .name     = colors_config_items[CONFIG_COLORS_ROOT].name,
          },
          {
                  .name = NULL,


If you give me your opinion or other way, I'd appreciate it. :)


Thanks,
Taeung

>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/util/config.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   tools/perf/util/config.h |  46 ++++++++++++++-
>>   2 files changed, 194 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index dad7d82..3a72ed7 100644
>> --- a/tools/perf/util/config.c
>> +++ b/tools/perf/util/config.c
>> @@ -29,6 +29,154 @@ static int config_file_eof;
>>
>>   const char *config_exclusive_filename;
>>
>> +const struct perf_config_section default_sections[] = {
>> +	{ .name = "colors" },
>> +	{ .name = "tui" },
>> +	{ .name = "buildid" },
>> +	{ .name = "annotate" },
>> +	{ .name = "gtk" },
>> +	{ .name = "pager" },
>> +	{ .name = "help" },
>> +	{ .name = "hist" },
>> +	{ .name = "ui" },
>> +	{ .name = "call-graph" },
>> +	{ .name = "report" },
>> +	{ .name = "top" },
>> +	{ .name = "man" },
>> +	{ .name = "kmem" },
>> +	{ .name = "intel-pt" },
>> +	{ .name = "convert" },
>> +};
>> +
>> +const struct default_config_item colors_config_items[] = {
>> +	CONF_STR_VAR("top", "red, default"),
>> +	CONF_STR_VAR("medium", "green, default"),
>> +	CONF_STR_VAR("normal", "default, default"),
>> +	CONF_STR_VAR("selected", "black, yellow"),
>> +	CONF_STR_VAR("jump_arrows", "blue, default"),
>> +	CONF_STR_VAR("addr", "magenta, default"),
>> +	CONF_STR_VAR("root", "white, blue"),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item tui_config_items[] = {
>> +	CONF_BOOL_VAR("report", true),
>> +	CONF_BOOL_VAR("annotate", true),
>> +	CONF_BOOL_VAR("top", true),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item buildid_config_items[] = {
>> +	CONF_STR_VAR("dir", "~/.debug"),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item annotate_config_items[] = {
>> +	CONF_BOOL_VAR("hide_src_code", false),
>> +	CONF_BOOL_VAR("use_offset", true),
>> +	CONF_BOOL_VAR("jump_arrows", true),
>> +	CONF_BOOL_VAR("show_nr_jumps", false),
>> +	CONF_BOOL_VAR("show_linenr", false),
>> +	CONF_BOOL_VAR("show_total_period", false),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item gtk_config_items[] = {
>> +	CONF_BOOL_VAR("annotate", false),
>> +	CONF_BOOL_VAR("report", false),
>> +	CONF_BOOL_VAR("top", false),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item pager_config_items[] = {
>> +	CONF_BOOL_VAR("cmd", true),
>> +	CONF_BOOL_VAR("report", true),
>> +	CONF_BOOL_VAR("annotate", true),
>> +	CONF_BOOL_VAR("top", true),
>> +	CONF_BOOL_VAR("diff", true),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item help_config_items[] = {
>> +	CONF_STR_VAR("format", "man"),
>> +	CONF_INT_VAR("autocorrect", 0),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item hist_config_items[] = {
>> +	CONF_STR_VAR("percentage", "absolute"),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item ui_config_items[] = {
>> +	CONF_BOOL_VAR("show-headers", true),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item call_graph_config_items[] = {
>> +	CONF_STR_VAR("record-mode", "fp"),
>> +	CONF_LONG_VAR("dump-size", 8192),
>> +	CONF_STR_VAR("print-type", "graph"),
>> +	CONF_STR_VAR("order", "callee"),
>> +	CONF_STR_VAR("sort-key", "function"),
>> +	CONF_DOUBLE_VAR("threshold", 0.5),
>> +	CONF_LONG_VAR("print-limit", 0),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item report_config_items[] = {
>> +	CONF_BOOL_VAR("group", true),
>> +	CONF_BOOL_VAR("children", true),
>> +	CONF_FLOAT_VAR("percent-limit", 0),
>> +	CONF_U64_VAR("queue-size", 0),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item top_config_items[] = {
>> +	CONF_BOOL_VAR("children", true),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item man_config_items[] = {
>> +	CONF_STR_VAR("viewer", "man"),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item kmem_config_items[] = {
>> +	CONF_STR_VAR("default", "slab"),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item intel_pt_config_items[] = {
>> +	CONF_INT_VAR("cache-divisor", 64),
>> +	CONF_BOOL_VAR("mispred-all", false),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item convert_config_items[] = {
>> +	CONF_U64_VAR("queue-size", 0),
>> +	CONF_END()
>> +};
>> +
>> +const struct default_config_item *default_config_items[] = {
>> +	colors_config_items,
>> +	tui_config_items,
>> +	buildid_config_items,
>> +	annotate_config_items,
>> +	gtk_config_items,
>> +	pager_config_items,
>> +	help_config_items,
>> +	hist_config_items,
>> +	ui_config_items,
>> +	call_graph_config_items,
>> +	report_config_items,
>> +	top_config_items,
>> +	man_config_items,
>> +	kmem_config_items,
>> +	intel_pt_config_items,
>> +	convert_config_items,
>> +};
>> +
>>   static int get_next_char(void)
>>   {
>>   	int c;
>> @@ -677,7 +825,7 @@ static void perf_config_section__purge(struct perf_config_section *section)
>>   static void perf_config_section__delete(struct perf_config_section *section)
>>   {
>>   	perf_config_section__purge(section);
>> -	zfree(&section->name);
>> +	zfree((char **)&section->name);
>>   	free(section);
>>   }
>>
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index 22ec626..84414af 100644
>> --- a/tools/perf/util/config.h
>> +++ b/tools/perf/util/config.h
>> @@ -4,6 +4,30 @@
>>   #include <stdbool.h>
>>   #include <linux/list.h>
>>
>> +enum perf_config_type {
>> +	CONFIG_TYPE_BOOL,
>> +	CONFIG_TYPE_INT,
>> +	CONFIG_TYPE_LONG,
>> +	CONFIG_TYPE_U64,
>> +	CONFIG_TYPE_FLOAT,
>> +	CONFIG_TYPE_DOUBLE,
>> +	CONFIG_TYPE_STRING
>> +};
>> +
>> +struct default_config_item {
>> +	const char *name;
>> +	union {
>> +		bool b;
>> +		int i;
>> +		u32 l;
>> +		u64 ll;
>> +		float f;
>> +		double d;
>> +		const char *s;
>> +	} value;
>> +	enum perf_config_type type;
>> +};
>> +
>>   struct perf_config_item {
>>   	char *name;
>>   	char *value;
>> @@ -11,7 +35,7 @@ struct perf_config_item {
>>   };
>>
>>   struct perf_config_section {
>> -	char *name;
>> +	const char *name;
>>   	struct list_head items;
>>   	struct list_head node;
>>   };
>> @@ -20,6 +44,26 @@ struct perf_config_set {
>>   	struct list_head sections;
>>   };
>>
>> +#define CONF_VAR(_name, _field, _val, _type)			\
>> +	{ .name = _name, .value._field = _val, .type = _type }
>> +
>> +#define CONF_BOOL_VAR(_name, _val)			\
>> +	CONF_VAR(_name, b, _val, CONFIG_TYPE_BOOL)
>> +#define CONF_INT_VAR(_name, _val)			\
>> +	CONF_VAR(_name, i, _val, CONFIG_TYPE_INT)
>> +#define CONF_LONG_VAR(_name, _val)			\
>> +	CONF_VAR(_name, l, _val, CONFIG_TYPE_LONG)
>> +#define CONF_U64_VAR(_name, _val)			\
>> +	CONF_VAR(_name, ll, _val, CONFIG_TYPE_U64)
>> +#define CONF_FLOAT_VAR(_name, _val)			\
>> +	CONF_VAR(_name, f, _val, CONFIG_TYPE_FLOAT)
>> +#define CONF_DOUBLE_VAR(_name, _val)			\
>> +	CONF_VAR(_name, d, _val, CONFIG_TYPE_DOUBLE)
>> +#define CONF_STR_VAR(_name, _val)			\
>> +	CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
>> +#define CONF_END()					\
>> +	{ .name = NULL }
>> +
>>   struct perf_config_set *perf_config_set__new(void);
>>   void perf_config_set__delete(struct perf_config_set *set);
>>
>> --
>> 2.5.0