Subject: Re: [PATCH v8 3/4] perf config: Prepare all default configs
To: Namhyung Kim <namhyung@kernel.org>
References: <1460620401-23430-1-git-send-email-treeze.taeung@gmail.com>
 <1460620401-23430-4-git-send-email-treeze.taeung@gmail.com>
 <20160414121916.GJ9056@kernel.org> <570FC86D.5060909@gmail.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
        linux-kernel@vger.kernel.org, Jiri Olsa <jolsa@kernel.org>,
        Ingo Molnar <mingo@kernel.org>, Peter Zijlstra <peterz@infradead.org>,
        Masami Hiramatsu <mhiramat@kernel.org>
From: Taeung Song <treeze.taeung@gmail.com>
Message-ID: <5714F556.1000308@gmail.com>
Date: Mon, 18 Apr 2016 23:55:18 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <570FC86D.5060909@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9200
Lines: 291

Hi, Namhyung

On 04/15/2016 01:42 AM, Taeung Song wrote:
> Hi, Arnaldo
>
> On 04/14/2016 09:19 PM, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Apr 14, 2016 at 04:53:20PM +0900, Taeung Song escreveu:
>>> To precisely manage configs,
>>> prepare all default perf's configs that contain
>>> default section name, variable name, value
>>> and correct type, not string type.
>>>
>>> In the near future, this will be used when
>>> checking type of config variable or showing
>>> all configs with default values, etc.
>>>
>>> Acked-by: 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 | 110
>>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>>   tools/perf/util/config.h |  62 +++++++++++++++++++++++++-
>>>   2 files changed, 168 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>>> index dad7d82..5604392 100644
>>> --- a/tools/perf/util/config.c
>>> +++ b/tools/perf/util/config.c
>>> @@ -15,6 +15,7 @@
>>>   #include "util/llvm-utils.h"   /* perf_llvm_config */
>>>   #include "config.h"
>>>
>>> +#define MAX_CONFIGS 64
>>
>> Do we have to add another arbitrary maximum? Where is it used?
>>
>
> IMHO, it is my idea. If you want to avoid using this arbitrary maxinum,
> I'd modify the code.
>
> MAX_CONFIGS is used in order to declare two-dimensional arrays
> 'default_config_items'
> as below.
>
> struct perf_config_item default_config_items[][MAX_CONFIGS] = {
>      [CONFIG_COLORS] = {
>          CONF_STR_VAR("top", "red, default"),
>          CONF_STR_VAR("medium", "green, default"),
> ...(omitted)...
>
> IMHO, it is because of perf_config_set__init() in [PATCH v8 4/4].
> If we don't use two-dimensional arrays, I think the code of
> perf_config_set__init() could be complicated.
>

As above, I used MAX_CONFIGS because of two-dimensinal arrays 
'default_config_items'.

What do you think about it ?

We don't need to add this arbitrary maximum ?
or would you mind, if I look for other way about 'default_config_item' ?

Thanks,
Taeung

>>>   #define MAXNAME (256)
>>>
>>>   #define DEBUG_CACHE_DIR ".debug"
>>> @@ -29,6 +30,111 @@ static int config_file_eof;
>>>
>>>   const char *config_exclusive_filename;
>>>
>>> +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" }
>>> +};
>>> +
>>> +struct perf_config_item default_config_items[][MAX_CONFIGS] = {
>>> +    [CONFIG_COLORS] = {
>>> +        CONF_STR_VAR("top", "red, default"),
>>> +        CONF_STR_VAR("medium", "green, default"),
>>> +        CONF_STR_VAR("normal", "lightgray, default"),
>>> +        CONF_STR_VAR("selected", "white, lightgray"),
>>> +        CONF_STR_VAR("jump_arrows", "blue, default"),
>>> +        CONF_STR_VAR("addr", "magenta, default"),
>>> +        CONF_STR_VAR("root", "white, blue"),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_TUI] = {
>>> +        CONF_BOOL_VAR("report", true),
>>> +        CONF_BOOL_VAR("annotate", true),
>>> +        CONF_BOOL_VAR("top", true),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_BUILDID] = {
>>> +        CONF_STR_VAR("dir", "~/.debug"),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_ANNOTATE] = {
>>> +        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()
>>> +    },
>>> +    [CONFIG_GTK] = {
>>> +        CONF_BOOL_VAR("annotate", false),
>>> +        CONF_BOOL_VAR("report", false),
>>> +        CONF_BOOL_VAR("top", false),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_PAGER] = {
>>> +        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()
>>> +    },
>>> +    [CONFIG_HELP] = {
>>> +        CONF_STR_VAR("format", "man"),
>>> +        CONF_INT_VAR("autocorrect", 0),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_HIST] = {
>>> +        CONF_STR_VAR("percentage", "absolute"),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_UI] = {
>>> +        CONF_BOOL_VAR("show-headers", true),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_CALL_GRAPH] = {
>>> +        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()
>>> +    },
>>> +    [CONFIG_REPORT] = {
>>> +        CONF_BOOL_VAR("group", true),
>>> +        CONF_BOOL_VAR("children", true),
>>> +        CONF_FLOAT_VAR("percent-limit", 0),
>>> +        CONF_U64_VAR("queue-size", 0),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_TOP] = {
>>> +        CONF_BOOL_VAR("children", true),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_MAN] = {
>>> +        CONF_STR_VAR("viewer", "man"),
>>> +        CONF_END()
>>> +    },
>>> +    [CONFIG_KMEM] = {
>>> +        CONF_STR_VAR("default", "slab"),
>>> +        CONF_END()
>>> +    }
>>> +};
>>> +
>>>   static int get_next_char(void)
>>>   {
>>>       int c;
>>> @@ -659,7 +765,7 @@ struct perf_config_set *perf_config_set__new(void)
>>>
>>>   static void perf_config_item__delete(struct perf_config_item *item)
>>>   {
>>> -    zfree(&item->name);
>>> +    zfree((char **)&item->name);
>>>       zfree(&item->value);
>>>       free(item);
>>>   }
>>> @@ -677,7 +783,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..84dcc1d 100644
>>> --- a/tools/perf/util/config.h
>>> +++ b/tools/perf/util/config.h
>>> @@ -4,14 +4,34 @@
>>>   #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 perf_config_item {
>>> -    char *name;
>>> +    const char *name;
>>>       char *value;
>>> +    union {
>>> +        bool b;
>>> +        int i;
>>> +        u32 l;
>>> +        u64 ll;
>>> +        float f;
>>> +        double d;
>>> +        const char *s;
>>> +    } default_value;
>>> +    enum perf_config_type type;
>>>       struct list_head node;
>>>   };
>>>
>>>   struct perf_config_section {
>>> -    char *name;
>>> +    const char *name;
>>>       struct list_head items;
>>>       struct list_head node;
>>>   };
>>> @@ -20,6 +40,44 @@ struct perf_config_set {
>>>       struct list_head sections;
>>>   };
>>>
>>> +enum perf_config_secion_idx {
>>> +    CONFIG_COLORS,
>>> +    CONFIG_TUI,
>>> +    CONFIG_BUILDID,
>>> +    CONFIG_ANNOTATE,
>>> +    CONFIG_GTK,
>>> +    CONFIG_PAGER,
>>> +    CONFIG_HELP,
>>> +    CONFIG_HIST,
>>> +    CONFIG_UI,
>>> +    CONFIG_CALL_GRAPH,
>>> +    CONFIG_REPORT,
>>> +    CONFIG_TOP,
>>> +    CONFIG_MAN,
>>> +    CONFIG_KMEM,
>>> +    CONFIG_END
>>> +};
>>> +
>>> +#define CONF_VAR(_name, _field, _val, _type)                \
>>> +    { .name = _name, .default_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