From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161596AbcFGPOW (ORCPT ); Tue, 7 Jun 2016 11:14:22 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:35478 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932411AbcFGPOR (ORCPT ); Tue, 7 Jun 2016 11:14:17 -0400 Subject: Re: [PATCH v7 3/7] perf config: Add global variable 'config_set' To: Arnaldo Carvalho de Melo References: <1465291577-20973-1-git-send-email-treeze.taeung@gmail.com> <1465291577-20973-4-git-send-email-treeze.taeung@gmail.com> <20160607140509.GA11589@kernel.org> Cc: linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Ingo Molnar , Peter Zijlstra , Alexander Shishkin , Masami Hiramatsu , Jiri Olsa , Wang Nan From: Taeung Song Message-ID: <5756E4C4.5080306@gmail.com> Date: Wed, 8 Jun 2016 00:14:12 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160607140509.GA11589@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 Good afternoon :) On 06/07/2016 11:05 PM, Arnaldo Carvalho de Melo wrote: > Em Tue, Jun 07, 2016 at 06:26:13PM +0900, Taeung Song escreveu: >> The config set is prepared by collecting >> all configs from config files (i.e. user config >> ~/.perfconfig and system config $(sysconfdir)/perfconfig) >> so the config set contains all config key-value pairs. >> >> We need to use it as global variable to share it. > > We should generally avoid global variables, and in this case, from > looking just at this patch, I'm not convinced we need to introduce more > global variables, isn't this object instantiated and deleted at the > cmd_config() function? So, can't we just pass it to any function needing > to access it? The reason why I added global variable 'config_set' that all config key-value pairs from config files is because of upcoming perf_config_set__delete() and perf_config(). This variable can be used not only at cmd_config() but also at new perf_config() and future perf_config_set__delete(). Wherever new perf_config() is called, this function check whether the global variable 'config_set' is initialized or not. And this function use the variable 'config_set' like below. (One of intended purposes of new perf_config() is to avoid the repetitive work that read config files, every time it is called and to reuse the global variable 'config_set' that already contains all config info.) +int perf_config(config_fn_t fn, void *data) +{ + if (config_set == NULL) + config_set = perf_config_set__new(); + + return perf_config_set__iter(config_set, fn, data); +} Finally, after sub-command finished, called perf_config_set__delete() use the global variable as argument to free it as below. diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,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_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); Sure, we can avoid 'extern' for the global variable 'config_set'. But IMHO, we may need to use a global variable 'config_set' at only util/config.c at least. (If perf_config_set__delete() don't take arguments) (But it seems natural that we should pair between destructor and constructor as you said. http://marc.info/?l=linux-kernel&m=146463657721848&w=2 ) And of course, there are other way e.g. modifying arguments of perf_config(), using perf_config_set__new() at the very beginning at main() or etc... Nevertheless, if we haven't to use the global variable, I'd find other way. > > I applied the first two patches. > Thank you! Taeung > >> And in near future, the variable will be handled in perf_config() >> and other functions at util/config.c >> >> Cc: Namhyung Kim >> Cc: Jiri Olsa >> Cc: Masami Hiramatsu >> Cc: Alexander Shishkin >> Signed-off-by: Taeung Song >> --- >> tools/perf/builtin-config.c | 9 ++++----- >> tools/perf/util/config.c | 1 + >> tools/perf/util/config.h | 2 ++ >> 3 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c >> index fe1b77f..b3bc01a 100644 >> --- a/tools/perf/builtin-config.c >> +++ b/tools/perf/builtin-config.c >> @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) >> int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) >> { >> int ret = 0; >> - struct perf_config_set *set; >> char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); >> >> argc = parse_options(argc, argv, config_options, config_usage, >> @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) >> else if (use_user_config) >> config_exclusive_filename = user_config; >> >> - set = perf_config_set__new(); >> - if (!set) { >> + config_set = perf_config_set__new(); >> + if (!config_set) { >> ret = -1; >> goto out_err; >> } >> @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) >> pr_err("Error: takes no arguments\n"); >> parse_options_usage(config_usage, config_options, "l", 1); >> } else { >> - ret = show_config(set); >> + ret = show_config(config_set); >> if (ret < 0) { >> const char * config_filename = config_exclusive_filename; >> if (!config_exclusive_filename) >> @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) >> usage_with_options(config_usage, config_options); >> } >> >> - perf_config_set__delete(set); >> + perf_config_set__delete(config_set); >> out_err: >> return ret; >> } >> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c >> index 8749eca..02fc6d5 100644 >> --- a/tools/perf/util/config.c >> +++ b/tools/perf/util/config.c >> @@ -28,6 +28,7 @@ static int config_linenr; >> static int config_file_eof; >> >> const char *config_exclusive_filename; >> +struct perf_config_set *config_set; >> >> static int get_next_char(void) >> { >> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h >> index 22ec626..ea157a4 100644 >> --- a/tools/perf/util/config.h >> +++ b/tools/perf/util/config.h >> @@ -20,6 +20,8 @@ struct perf_config_set { >> struct list_head sections; >> }; >> >> +extern struct perf_config_set *config_set; >> + >> struct perf_config_set *perf_config_set__new(void); >> void perf_config_set__delete(struct perf_config_set *set); >> >> -- >> 2.5.0