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
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.)

>> +
>> return 0;
>> }
>>
>> 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);
}
}

> 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);
+ }
}
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.


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 08:21    [W:0.076 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site