Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755857AbcDGKgE (ORCPT ); Thu, 7 Apr 2016 06:36:04 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:34387 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752175AbcDGKgC (ORCPT ); Thu, 7 Apr 2016 06:36:02 -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> <570355F7.7060205@gmail.com> <20160405202333.c3347e223a5162bfa6811e25@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: <5706380D.1010507@gmail.com> Date: Thu, 7 Apr 2016 19:35:57 +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: <20160405202333.c3347e223a5162bfa6811e25@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: 10451 Lines: 347 Hi, Masami Because of my unclear response, I resend this email after submitting v7 this patchset. :) On 04/05/2016 08:23 PM, Masami Hiramatsu wrote: > On Tue, 5 Apr 2016 15:06:47 +0900 > Taeung Song wrote: > >> 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.) > > 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. I tested multiple cases (e.g after removing ~/.perfconfig or with noting configured config files(but there are config files). But has_value isn't always true because config_item has both 'value' and 'default_value' and perf_config_set__init() don't handle 'value'. Even though perf_config_set__init() initializes the set with default values, the function don't use 'value' and handle 'default_value' variable of each config_item. (The reason that config_item has both 'value' and 'default' is for upcoming features (e.g. --list-all, getting, setting, --remove, etc.)) And I used has_value to know whether config files have config information or not in show_config(). > But even though, if has_value > is intended to detect unexpected bugs, also introducing (set != NULL) > check is more natural. I added it at '[PATCH v7 2/4] perf config: Let show_config() work with perf_config_set'. Thanks, Taeung > >>>> 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 >>>> >>> >>> > >