Re: [PATCH v7 3/7] perf config: Add global variable 'config_set'

[Taeung Song <treeze.taeung@gmail.com>]

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 <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   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