Message-ID: <570355F7.7060205@gmail.com>
Date:	Tue, 5 Apr 2016 15:06:47 +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

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

>> +
>>   	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
>>
>
>

Powered by blists - more mailing lists