Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031231AbcCRAwX (ORCPT ); Thu, 17 Mar 2016 20:52:23 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:33608 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752728AbcCRAwO (ORCPT ); Thu, 17 Mar 2016 20:52:14 -0400 Subject: Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class To: Namhyung Kim References: <1457957769-3700-1-git-send-email-treeze.taeung@gmail.com> <1457957769-3700-2-git-send-email-treeze.taeung@gmail.com> <20160317123157.GA1670@danjae.kornet> <56EABAC4.107@gmail.com> <20160317232722.GC5194@sejong> Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra From: Taeung Song Message-ID: <56EB513A.5010805@gmail.com> Date: Fri, 18 Mar 2016 09:52:10 +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: <20160317232722.GC5194@sejong> Content-Type: text/plain; charset=utf-8; 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: 7230 Lines: 256 On 03/18/2016 08:27 AM, Namhyung Kim wrote: > On Thu, Mar 17, 2016 at 11:10:12PM +0900, Taeung Song wrote: >> Hi, Namhyung >> >> On 03/17/2016 09:31 PM, Namhyung Kim wrote: >>> Hi Taeung, >>> >>> On Mon, Mar 14, 2016 at 09:16:05PM +0900, Taeung Song wrote: >>>> This infrastructure code was designed for >>>> upcoming features of perf-config. >>>> >>>> That collect config key-value pairs from user and >>>> system config files (i.e. user wide ~/.perfconfig >>>> and system wide $(sysconfdir)/perfconfig) >>>> to manage perf's configs. >>>> >>>> Cc: Namhyung Kim >>>> Cc: Jiri Olsa >>>> Signed-off-by: Taeung Song >>>> --- >>>> tools/perf/builtin-config.c | 1 + >>>> tools/perf/util/config.c | 123 ++++++++++++++++++++++++++++++++++++++++++++ >>>> tools/perf/util/config.h | 21 ++++++++ >>>> 3 files changed, 145 insertions(+) >>>> create mode 100644 tools/perf/util/config.h >>>> >>>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c >>>> index c42448e..412c725 100644 >>>> --- a/tools/perf/builtin-config.c >>>> +++ b/tools/perf/builtin-config.c >>>> @@ -12,6 +12,7 @@ >>>> #include >>>> #include "util/util.h" >>>> #include "util/debug.h" >>>> +#include "util/config.h" >>>> >>>> static bool use_system_config, use_user_config; >>>> >>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >>>> index 4e72763..b9660e4 100644 >>>> --- a/tools/perf/util/config.c >>>> +++ b/tools/perf/util/config.c >>>> @@ -13,6 +13,7 @@ >>>> #include >>>> #include "util/hist.h" /* perf_hist_config */ >>>> #include "util/llvm-utils.h" /* perf_llvm_config */ >>>> +#include "config.h" >>>> >>>> #define MAXNAME (256) >>>> >>>> @@ -506,6 +507,128 @@ out: >>>> return ret; >>>> } >>>> >>>> +static struct perf_config_item *find_config(struct list_head *config_list, >>>> + const char *section, >>>> + const char *name) >>>> +{ >>>> + struct perf_config_item *config; >>>> + >>>> + list_for_each_entry(config, config_list, list) { >>>> + if (!strcmp(config->section, section) && >>>> + !strcmp(config->name, name)) >>>> + return config; >>>> + } >>> >>> Hmm.. why do you remove the section list? >>> >> >> IMHO, there are several reasons >> >> 1) To use only one list (default config, custom config(user/system)) >> >> 1-1) I used two list that were 'list_head sections' >> and 'config_item default_configs[]'. So if checking >> type of config variable, two for-loop must be needed >> for each list. Because two structure was different i.e. >> >> 'sections' list mean config_section list >> that each section contain config_element list. >> (there wasn't telling about correct type of 'value' instead of string(char >> *)) >> >> struct config_element { >> char *name; >> char *value; >> struct list_head list; >> }; >> >> struct config_section { >> char *name; >> struct list_head element_head; >> struct list_head list; >> }; >> >> 'struct config_item default_configs[]' mean all default configs. >> >> struct config_item { >> const char *section; >> const char *name; >> union { >> bool b; >> int i; >> u32 l; >> u64 ll; >> float f; >> double d; >> const char *s; >> } value; >> enum config_type type; >> const char *desc; >> }; >> >> >> IMHO, I think this is a bit complex >> and I want to simplify the perf's config list on perf-config. >> >> 2) A small number of perf's configs >> >> I think perf's configs aren't too many so I think >> two structure for section and element aren't needed. > > OK. > > >> >> 3) A object for a config variable need to have enough info for itself >> >> This is a bit similar to 1) reason. >> If using only 'struct config_item' for the config list, >> it can contain section name, name, values(default, user config, >> system config, both config), correct type, etc. >> >> If we do, we needn't to find detail for a config variable at other objects >> e.g. >> When we find correct type of a config variable, >> we needn't to do for-loop for default_configs[] in order to know the >> type. > > I'm not sure I understand you correctly, but I think this is not > related to the two-level structure. What you said is right. I just thought using two lists (list_head sections, default_configs[]) was complex.. At this patchset, I only focused on simplifying the config list on perf-config.. As you said, it hasn't problems to use the two-level structure i.e. struct config_element struct config_section (that has multiple elements) So, what about this structures ? (you may already think about it, but..) struct config_element { char *name; char *value; /*from the two config file (user/system)*/ union { bool b; int i; u32 l; u64 ll; float f; double d; const char *s; } default_value; /* perf's default config value */ enum config_type type; struct list_head list; }; struct config_section { char *name; struct list_head element_head; struct list_head list; }; Because I want to use only one list (default_configs + sections) for more concise code than old code. > >> >> >> I think this is better than old two structure. >> >>>> + >>>> + return NULL; >>>> +} >>>> + >>>> +static struct perf_config_item *add_config(struct list_head *config_list, >>>> + const char *section, >>>> + const char *name) >>>> +{ >>>> + struct perf_config_item *config = zalloc(sizeof(*config)); >>>> + >>>> + if (!config) >>>> + return NULL; >>>> + >>>> + config->section = strdup(section); >>>> + if (!section) >>>> + goto out_err; >>>> + >>>> + config->name = strdup(name); >>>> + if (!name) { >>>> + free((char *)config->section); >>>> + goto out_err; >>>> + } >>>> + >>>> + list_add_tail(&config->list, config_list); >>>> + return config; >>>> + >>>> +out_err: >>>> + free(config); >>>> + pr_err("%s: strdup failed\n", __func__); >>>> + return NULL; >>>> +} >>>> + >>>> +static int set_value(struct perf_config_item *config, const char *value) >>>> +{ >>>> + char *val = strdup(value); >>>> + >>>> + if (!val) >>>> + return -1; >>>> + config->value = val; >>> >>> It seems to overwrite old value.. >>> >> >> Yes, I know it. >> If don't using '--user' or '--system', >> there isn't exclusive config file path >> then have to read both config files. >> >> But because user config file has a high order of priority, >> if two config file has same variable, old value(for system config) >> must be overwrote by new value(for user config). > > But shouldn't it free the old value before overwriting? > Sorry, I missed free() out. I'll fix it. Thanks, Taeung > >> >>> >>> >>>> + >>>> + return 0; >>>> +}