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

From: Masami Hiramatsu
Date: Tue Apr 05 2016 - 07:23:43 EST

On Tue, 5 Apr 2016 15:06:47 +0900
Taeung Song <treeze.taeung@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx>
> >> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> >> Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxx>