Messages in this thread | | | Date | Fri, 18 Mar 2016 08:27:22 +0900 | From | Namhyung Kim <> | Subject | Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class |
| |
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 <namhyung@kernel.org> > >>Cc: Jiri Olsa <jolsa@kernel.org> > >>Signed-off-by: Taeung Song <treeze.taeung@gmail.com> > >>--- > >> 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 <subcmd/parse-options.h> > >> #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 <subcmd/exec-cmd.h> > >> #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.
> > > 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?
Thanks, Namhyung
> > > > > > >>+ > >>+ return 0; > >>+}
| |