From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751700AbcFWL5F (ORCPT ); Thu, 23 Jun 2016 07:57:05 -0400 Received: from mail.kernel.org ([198.145.29.136]:40768 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750874AbcFWL5C (ORCPT ); Thu, 23 Jun 2016 07:57:02 -0400 Date: Thu, 23 Jun 2016 08:56:56 -0300 From: Arnaldo Carvalho de Melo To: Taeung Song Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Wang Nan , Jiri Olsa , Ingo Molnar Subject: Re: [PATCH v10 2/3] perf config: Reimplement perf_config() introducing new perf_config_init() and perf_config_finish() Message-ID: <20160623115656.GJ4213@kernel.org> References: <1466672119-4852-1-git-send-email-treeze.taeung@gmail.com> <1466672119-4852-3-git-send-email-treeze.taeung@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1466672119-4852-3-git-send-email-treeze.taeung@gmail.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Thu, Jun 23, 2016 at 05:55:18PM +0900, Taeung Song escreveu: > Many sub-commands use perf_config() but > everytime perf_config() is called, perf_config() always read config files. > (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') > > But it is better to use the config set that already contains all config > key-value pairs to avoid this repetitive work reading the config files > in perf_config(). (the config set mean a static variable 'config_set') > > In other words, if new perf_config_init() is called, > only first time 'config_set' is initialized collecting all configs from the config files. > And then we could use new perf_config() like old perf_config(). > When a sub-command finished, free the config set by perf_config_finish() at run_builtin(). > > If we do, 'config_set' can be reused wherever perf_config() is called > and a feature of old perf_config() is the same as new perf_config() work > without the repetitive work that read the config files. > > In summary, in order to use features about configuration, > we can call the functions at perf.c and other source files as below. > > # initialize a config set > perf_config_init() > > # configure actual variables from a config set > perf_config() > > # eliminate allocated config set > perf_config_finish() > > # destroy existing config set and initialize a new config set. > perf_config_refresh() > > Cc: Namhyung Kim > Cc: Jiri Olsa > Cc: Wang Nan > Cc: Peter Zijlstra > Cc: Ingo Molnar > Cc: Masami Hiramatsu > Cc: Alexander Shishkin > Signed-off-by: Taeung Song > --- > tools/perf/builtin-config.c | 4 ++ > tools/perf/perf.c | 2 + > tools/perf/util/config.c | 92 +++++++++++++++++++++++---------------------- > tools/perf/util/config.h | 29 ++++++++++++++ > 4 files changed, 82 insertions(+), 45 deletions(-) > > diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c > index fe1b77f..cfd1036 100644 > --- a/tools/perf/builtin-config.c > +++ b/tools/perf/builtin-config.c > @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) > else if (use_user_config) > config_exclusive_filename = user_config; > > + /* > + * At only 'config' sub-command, individually use the config set > + * because of reinitializing with options config file location. > + */ > set = perf_config_set__new(); > if (!set) { > ret = -1; > diff --git a/tools/perf/perf.c b/tools/perf/perf.c > index 66772da..280967e 100644 > --- a/tools/perf/perf.c > +++ b/tools/perf/perf.c > @@ -355,6 +355,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) > > perf_env__set_cmdline(&perf_env, argc, argv); > status = p->fn(argc, argv, prefix); > + perf_config_finish(); > exit_browser(status); > perf_env__exit(&perf_env); > bpf__clear(); > @@ -522,6 +523,7 @@ int main(int argc, const char **argv) > > srandom(time(NULL)); > > + perf_config_init(); > perf_config(perf_default_config, NULL); > set_buildid_dir(NULL); > > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c > index d15c592..a16f95d 100644 > --- a/tools/perf/util/config.c > +++ b/tools/perf/util/config.c > @@ -26,6 +26,7 @@ static FILE *config_file; > static const char *config_file_name; > static int config_linenr; > static int config_file_eof; > +static struct perf_config_set *config_set; > > const char *config_exclusive_filename; > > @@ -478,51 +479,6 @@ static int perf_config_global(void) > return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); > } > > -int perf_config(config_fn_t fn, void *data) > -{ > - int ret = -1; > - const char *home = NULL; > - > - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ > - if (config_exclusive_filename) > - return perf_config_from_file(fn, config_exclusive_filename, data); > - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { > - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) > - goto out; > - } > - > - home = getenv("HOME"); > - if (perf_config_global() && home) { > - char *user_config = strdup(mkpath("%s/.perfconfig", home)); > - struct stat st; > - > - if (user_config == NULL) { > - warning("Not enough memory to process %s/.perfconfig, " > - "ignoring it.", home); > - goto out; > - } > - > - if (stat(user_config, &st) < 0) > - goto out_free; > - > - if (st.st_uid && (st.st_uid != geteuid())) { > - warning("File %s not owned by current user or root, " > - "ignoring it.", user_config); > - goto out_free; > - } > - > - if (!st.st_size) > - goto out_free; > - > - ret = perf_config_from_file(fn, user_config, data); > - > -out_free: > - free(user_config); > - } > -out: > - return ret; > -} > - > static struct perf_config_section *find_section(struct list_head *sections, > const char *section_name) > { > @@ -706,6 +662,52 @@ struct perf_config_set *perf_config_set__new(void) > return set; > } > > +int perf_config(config_fn_t fn, void *data) > +{ > + int ret = 0; > + char key[BUFSIZ]; > + struct perf_config_section *section; > + struct perf_config_item *item; > + > + if (config_set == NULL) > + return -1; > + > + config_set__for_each(config_set, section, item) { > + char *value = item->value; > + > + if (value) { > + scnprintf(key, sizeof(key), "%s.%s", > + section->name, item->name); > + ret = fn(key, value, data); > + if (ret < 0) { > + pr_err("Error: wrong config key-value pair %s=%s\n", > + key, value); > + break; > + } > + } > + } > + > + return ret; > +} > + > +void perf_config_init(void) > +{ > + if (config_set == NULL) > + config_set = perf_config_set__new(); > +} > + > +void perf_config_finish(void) > +{ > + perf_config_set__delete(config_set); > + config_set = NULL; > +} > + > +void perf_config_refresh(void) > +{ > + perf_config_finish(); > + perf_config_init(); > +} > + > static void perf_config_item__delete(struct perf_config_item *item) > { > zfree(&item->name); > diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h > index 155a441..746c619 100644 > --- a/tools/perf/util/config.h > +++ b/tools/perf/util/config.h > @@ -33,5 +33,34 @@ const char *perf_etc_perfconfig(void); > > struct perf_config_set *perf_config_set__new(void); > void perf_config_set__delete(struct perf_config_set *set); > +void perf_config_init(void); > +void perf_config_finish(void); > +void perf_config_refresh(void); Please use double _ to separate the subsystem/class from its methods, i.e.: void perf_config__init(void); void perf_config__finish(void); void perf_config__refresh(void); > + > +/** > + * config_sections__for_each - iterate thru all the sections > + * @list: list_head instance to iterate > + * @section: struct perf_config_section iterator > + */ > +#define config_sections__for_each(list, section) \ > + list_for_each_entry(section, list, node) These macros operate on perf_config_sections, so please name it accordingly, i.e.: perf_config_sections__for_each() > + > +/** > + * config_items__for_each - iterate thru all the items > + * @list: list_head instance to iterate > + * @item: struct perf_config_item iterator > + */ > +#define config_items__for_each(list, item) \ > + list_for_each_entry(item, list, node) > + Ditto > +/** > + * config_set__for_each - iterate thru all the config section-item pairs > + * @set: evlist instance to iterate > + * @section: struct perf_config_section iterator > + * @item: struct perf_config_item iterator > + */ > +#define config_set__for_each(set, section, item) \ > + config_sections__for_each(&set->sections, section) \ > + config_items__for_each(§ion->items, item) Ditto. > #endif /* __PERF_CONFIG_H */ > -- > 2.5.0