On Tue, 5 Apr 2016 15:06:47 +0900 Taeung Song <[email protected]> wrote:
> Hi, Masami > > On 04/04/2016 10:28 PM, Masami Hiramatsu wrote: > > On Mon, 4 Apr 2016 18:17:07 +0900 > > Taeung Song <[email protected]> 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 <[email protected]> > >> Cc: Jiri Olsa <[email protected]> > >> Signed-off-by: Taeung Song <[email protected]> > >> --- > >> 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, §ion->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(§ion->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(§ion->items); > >> + > >> + items = default_config_items[i]; > >> + for (j = 0; items[j].name != NULL; j++) > >> + list_add_tail(&items[j].node, §ion->items); > >> + > >> + list_add_tail(§ion->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, §ion->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(§ion->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, §ion->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(§ion->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 **)§ion->name); > - free(section); > + if (section->is_allocated) { > + zfree((char **)§ion->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(§ion->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 <[email protected]>
