Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370614

Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs

Show all headers | View raw

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. 

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

Also, I like ->allocated instead of ->is_custom.

Thank you,

>  	}
>  }
>  
> 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@kernel.org>

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v6 0/4] Infrastructure code for perf-config Taeung Song <treeze.taeung@gmail.com> - 2016-04-04 11:20 +0200
  [PATCH v6 3/4] perf config: Prepare all default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-04 11:20 +0200
  [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set Taeung Song <treeze.taeung@gmail.com> - 2016-04-04 11:20 +0200
    Re: [PATCH v6 2/4] perf config: Let show_config() work with  perf_config_set Masami Hiramatsu <mhiramat@kernel.org> - 2016-04-04 14:50 +0200
      Re: [PATCH v6 2/4] perf config: Let show_config() work with  perf_config_set Taeung Song <treeze.taeung@gmail.com> - 2016-04-05 04:00 +0200
  [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-04 11:20 +0200
    Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all  default configs Masami Hiramatsu <mhiramat@kernel.org> - 2016-04-04 15:30 +0200
      Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all  default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-05 08:10 +0200
        Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all  default configs Masami Hiramatsu <mhiramat@kernel.org> - 2016-04-05 13:30 +0200
          Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all  default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-05 13:50 +0200
          Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all  default configs Taeung Song <treeze.taeung@gmail.com> - 2016-04-07 12:40 +0200

csiph-web