Message-Id: <20160405202333.c3347e223a5162bfa6811e25@kernel.org>
Date:	Tue, 5 Apr 2016 20:23:33 +0900
From:	Masami Hiramatsu <mhiramat@...nel.org>
To:	Taeung Song <treeze.taeung@...il.com>
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

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. But even though, if has_value
is intended to detect unexpected bugs, also introducing (set != NULL)
check is more natural.

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


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists