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

From: Taeung Song
Date: Tue May 10 2016 - 07:49:26 EST

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@xxxxxxxxxx>
Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
---
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