lkml.org 
[lkml]   [2016]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
From
Date


On 04/05/2016 08:23 PM, Masami Hiramatsu wrote:
> On Tue, 5 Apr 2016 15:06:47 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> Hi, Masami
>>
>> On 04/04/2016 10:28 PM, Masami Hiramatsu wrote:
>>> On Mon, 4 Apr 2016 18:17:07 +0900
>>> Taeung Song <treeze.taeung@gmail.com> wrote:
>>>
>>>> To avoid duplicated config variables and
>>>> use perf_config_set classifying between standard
>>>> perf config variables and unknown or new config
>>>> variables other than them, initialize perf_config_set
>>>> with all default configs.
>>>>
>>>> And this will be needed when showing all configs with
>>>> default value or checking correct type of a config
>>>> variable in the near future.
>>>>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>>> ---
>>>> tools/perf/builtin-config.c | 11 +++++++----
>>>> tools/perf/util/config.c | 33 ++++++++++++++++++++++++++++++---
>>>> tools/perf/util/config.h | 7 +++++++
>>>> 3 files changed, 44 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>>>> index 1133224..66a269e 100644
>>>> --- a/tools/perf/builtin-config.c
>>>> +++ b/tools/perf/builtin-config.c
>>>> @@ -35,23 +35,26 @@ static struct option config_options[] = {
>>>>
>>>> static int show_config(struct perf_config_set *set)
>>>> {
>>>> + bool has_value = false;
>>>> struct perf_config_section *section;
>>>> struct perf_config_item *item;
>>>> struct list_head *sections = &set->sections;
>>>>
>>>> - if (list_empty(sections))
>>>> - return -1;
>>>> -
>>>> list_for_each_entry(section, sections, node) {
>>>> list_for_each_entry(item, &section->items, node) {
>>>> char *value = item->value;
>>>>
>>>> - if (value)
>>>> + if (value) {
>>>> printf("%s.%s=%s\n", section->name,
>>>> item->name, value);
>>>> + has_value = true;
>>>> + }
>>>> }
>>>> }
>>>>
>>>> + if (!has_value)
>>>> + return -1;
>>>
>>> Could this path be really executed? It seems that the set is
>>> always initialized with default values. This means not empty.
>>> If it is for asserting bugs, we also have to check set != NULL
>>> beforehand.
>>
>> I got it.
>> But before calling show_config(set), the set is checked as below
>>
>> ... (omitted) ...
>> set = perf_config_set__new();
>> if (!set) {
>> ret = -1;
>> goto out_err;
>> }
>>
>> switch (actions) {
>> case ACTION_LIST:
>> if (argc) {
>> pr_err("Error: takes no arguments\n");
>> parse_options_usage(config_usage, config_options, "l", 1);
>> } else {
>> ret = show_config(set);
>>
>> ...(omitted)...
>>
>> I thought that the set can never be NULL in show_config().
>> Is it wrong ?
>> (I'm afraid that there are things I misunderstood.)
>
> Hmm, my main point was has_value is always true because
> perf_config_set__init() initializes the set with default value.
> If so, has_value can be removed.

'value' isn't used in perf_config_set__init() as below.

1) util/config.c

...(omitted)...
struct perf_config_item default_config_items[][MAX_CONFIGS] = {
[CONFIG_COLORS] = {
CONF_STR_VAR("top", "red, default"),
...(omitted)...

2) util/config.h

...(omitted)...
#define CONF_VAR(_name, _field, _val, _type) \
{ .name = _name, .default_value._field = _val, .type = _type }

...(omitted)...
#define CONF_STR_VAR(_name, _val) \
CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
...(omitted)...


config_item has both 'default_value' and 'value'
because of usability for upcoming functionality e.g (setting, getting,
--list-all, --remove, etc).

New config item or modified existing config item only use 'value'.
IMHO, I think has_value haven't to be removed.

> But even though, if has_value
> is intended to detect unexpected bugs, also introducing (set != NULL)
> check is more natural.

I got it.
I'll check set!=NULL at the very beginning in show_config().

Thanks,
Taeung


>
>>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>>>> index f937124..1855560 100644
>>>> --- a/tools/perf/util/config.c
>>>> +++ b/tools/perf/util/config.c
>>>> @@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
>>>> if (!section)
>>>> return NULL;
>>>>
>>>> + section->is_custom = true;
>>>> INIT_LIST_HEAD(&section->items);
>>>> section->name = strdup(section_name);
>>>> if (!section->name) {
>>>> @@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
>>>> if (!item)
>>>> return NULL;
>>>>
>>>> + item->is_custom = true;
>>>> item->name = strdup(name);
>>>> if (!item->name) {
>>>> pr_debug("%s: strdup failed\n", __func__);
>>>> @@ -751,12 +753,35 @@ out_free:
>>>> return -1;
>>>> }
>>>>
>>>> +static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
>>>> +{
>>>> + int i, j;
>>>> + struct perf_config_section *section;
>>>> + struct perf_config_item *items;
>>>> + struct list_head *sections = &set->sections;
>>>> +
>>>> + INIT_LIST_HEAD(&set->sections);
>>>> +
>>>> + for (i = 0; i != CONFIG_END; i++) {
>>>> + section = &default_sections[i];
>>>> + INIT_LIST_HEAD(&section->items);
>>>> +
>>>> + items = default_config_items[i];
>>>> + for (j = 0; items[j].name != NULL; j++)
>>>> + list_add_tail(&items[j].node, &section->items);
>>>> +
>>>> + list_add_tail(&section->node, sections);
>>>> + }
>>>> +
>>>> + return set;
>>>> +}
>>>> +
>>>> struct perf_config_set *perf_config_set__new(void)
>>>> {
>>>> struct perf_config_set *set = zalloc(sizeof(*set));
>>>>
>>>> if (set) {
>>>> - INIT_LIST_HEAD(&set->sections);
>>>> + perf_config_set__init(set);
>>>> perf_config(collect_config, set);
>>>> }
>>>>
>>>> @@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
>>>>
>>>> list_for_each_entry_safe(item, tmp, &section->items, node) {
>>>> list_del_init(&item->node);
>>>> - perf_config_item__delete(item);
>>>> + if (item->is_custom)
>>>> + perf_config_item__delete(item);
>>>> }
>>>> }
>>>>
>>>> @@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
>>>>
>>>> list_for_each_entry_safe(section, tmp, &set->sections, node) {
>>>> list_del_init(&section->node);
>>>> - perf_config_section__delete(section);
>>>> + if (section->is_custom)
>>>> + perf_config_section__delete(section);
>>>
>>> Here, unless perf_config_section__delete() is called, the items
>>> under the section is never be checked.
>>
>> It is my stupid mistake..
>>
>>> However, if a user changes
>>> existing item value, its section->is_custom is false, but the
>>> item itself is_custom be true.
>>> This means that such custom item values may not be freed.
>>
>> And there is one more what I missed.
>> I didn't consider that when existing item's 'value' is set by set_value(),
>> it can not be freed. So fix it as below.
>>
>> static void perf_config_item__delete(struct perf_config_item *item)
>> {
>> - zfree((char **)&item->name);
>> zfree(&item->value);
>> - free(item);
>> + if (item->is_allocated) {
>> + zfree((char **)&item->name);
>> + free(item);
>> + }
>> }
>>
>> static void perf_config_section__purge(struct perf_config_section
>> *section)
>> {
>> struct perf_config_item *item, *tmp;
>>
>> list_for_each_entry_safe(item, tmp, &section->items, node) {
>> list_del_init(&item->node);
>> - if (item->is_custom)
>> - perf_config_item__delete(item);
>> + perf_config_item__delete(item);
>> }
>> }
>
> Good catch! it also should be done :)

Thanks you!

>
>>
>>
>>> So, the section->is_custom flag should be checked in *__delete
>>> method as below,
>>> static void perf_config_section__delete(struct perf_config_section *section)
>>> {
>>> perf_config_section__purge(section);
>>> if (section->is_custom) {
>>> zfree(&section->name);
>>> free(section);
>>> }
>>> }
>>>
>>> Or, you can just call perf_config_section__purge(section) in
>>> perf_config_set__purge().
>>>
>>
>> I'll modify this code by the former that you said like below.
>>
>>
>> static void perf_config_section__delete(struct perf_config_section
>> *section)
>> {
>> perf_config_section__purge(section);
>> - zfree((char **)&section->name);
>> - free(section);
>> + if (section->is_allocated) {
>> + zfree((char **)&section->name);
>> + free(section);
>> + }
>> }
>
> Looks good :)

Thanks you :-)

>>
>> static void perf_config_set__purge(struct perf_config_set *set)
>> @@ -819,8 +822,7 @@ static void perf_config_set__purge(struct
>> perf_config_set *set)
>>
>> list_for_each_entry_safe(section, tmp, &set->sections, node) {
>> list_del_init(&section->node);
>> - if (section->is_custom)
>> - perf_config_section__delete(section);
>> + perf_config_section__delete(section);
>> }
>> }
>>
>>
>>
>>> Also, I like ->allocated instead of ->is_custom.
>>>
>>
>> What you said is right.
>> Because the variable means whether a config section or item is allocated,
>> not customized by a user.
>> I'll rename it.
>
> Thank you!
>
>>
>>
>> Thanks,
>> Taeung
>>
>>>
>>>> }
>>>> }
>>>>
>>>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>>>> index 84dcc1d..1df96b7 100644
>>>> --- a/tools/perf/util/config.h
>>>> +++ b/tools/perf/util/config.h
>>>> @@ -14,6 +14,11 @@ enum perf_config_type {
>>>> CONFIG_TYPE_STRING
>>>> };
>>>>
>>>> +/**
>>>> + * struct perf_config_item - element of perf's configs
>>>> + *
>>>> + * @is_custom: unknown or new config other than default config
>>>> + */
>>>> struct perf_config_item {
>>>> const char *name;
>>>> char *value;
>>>> @@ -27,11 +32,13 @@ struct perf_config_item {
>>>> const char *s;
>>>> } default_value;
>>>> enum perf_config_type type;
>>>> + bool is_custom;
>>>> struct list_head node;
>>>> };
>>>>
>>>> struct perf_config_section {
>>>> const char *name;
>>>> + bool is_custom;
>>>> struct list_head items;
>>>> struct list_head node;
>>>> };
>>>> --
>>>> 2.5.0
>>>>
>>>
>>>
>
>

\
 
 \ /
  Last update: 2016-04-05 14:01    [W:0.399 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site