| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 May 2016 19:30:25 +0900
From: Taeung Song <treeze.taeung@...il.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc: Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH 1/4] perf config: Introduce default_config_item for all
default config key-value pairs
Hi, Arnaldo
On 05/11/2016 12:05 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 10, 2016 at 08:49:16PM +0900, Taeung Song escreveu:
>> 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
>> +
>
> No hard coded magical maximum values :-\
>
I got it.
I'll send v2 without the above change. :)
Thanks,
Taeung
>> 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@...nel.org>
>>>> Cc: Masami Hiramatsu <mhiramat@...nel.org>
>>>> Cc: Jiri Olsa <jolsa@...nel.org>
>>>> Signed-off-by: Taeung Song <treeze.taeung@...il.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(§ion->name);
>>>> + zfree((char **)§ion->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
Powered by blists - more mailing lists