Message-ID: <5706380D.1010507@gmail.com>
Date:	Thu, 7 Apr 2016 19:35:57 +0900
From:	Taeung Song <treeze.taeung@...il.com>
To:	Masami Hiramatsu <mhiramat@...nel.org>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all
 default configs

Hi, Masami

Because of my unclear response,
I resend this email after submitting v7 this patchset. :)

On 04/05/2016 08:23 PM, Masami Hiramatsu wrote:
> On Tue, 5 Apr 2016 15:06:47 +0900
> Taeung Song <treeze.taeung@...il.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@...il.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@...nel.org>
>>>> Cc: Jiri Olsa <jolsa@...nel.org>
>>>> Signed-off-by: Taeung Song <treeze.taeung@...il.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.

I tested multiple cases (e.g after removing ~/.perfconfig
or with noting configured config files(but there are config files).

But has_value isn't always true because config_item has both 'value'
and 'default_value' and perf_config_set__init() don't handle 'value'.

Even though perf_config_set__init() initializes the set with default values,
the function don't use 'value' and handle 'default_value' variable
of each config_item.

(The reason that config_item has both 'value' and 'default' is
for upcoming features (e.g. --list-all, getting, setting, --remove, etc.))

And I used has_value to know whether config files have config 
information or not in show_config().


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

I added it at '[PATCH v7 2/4] perf config: Let show_config() work with 
perf_config_set'.

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

Powered by blists - more mailing lists