Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756011AbcDEGGz (ORCPT ); Tue, 5 Apr 2016 02:06:55 -0400 Received: from mail-pa0-f65.google.com ([209.85.220.65]:36330 "EHLO mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751797AbcDEGGx (ORCPT ); Tue, 5 Apr 2016 02:06:53 -0400 Subject: Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs To: Masami Hiramatsu References: <1459761427-16214-1-git-send-email-treeze.taeung@gmail.com> <1459761427-16214-5-git-send-email-treeze.taeung@gmail.com> <20160404222823.2d5e7add53eabf95ee0c4273@kernel.org> Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra From: Taeung Song Message-ID: <570355F7.7060205@gmail.com> Date: Tue, 5 Apr 2016 15:06:47 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160404222823.2d5e7add53eabf95ee0c4273@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8350 Lines: 299 Hi, Masami On 04/04/2016 10:28 PM, Masami Hiramatsu wrote: > On Mon, 4 Apr 2016 18:17:07 +0900 > Taeung Song 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 >> Cc: Jiri Olsa >> Signed-off-by: Taeung Song >> --- >> 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.) >> + >> 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(§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); } } > 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); + } } 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. 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 >> > >