Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1409768 > unrolled thread

[RFC][PATCH v4 0/6] perf config: Reimplement perf_config() using perf_config_set__inter()

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH v4 0/6] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 03:20 +0200
    [PATCH v4 5/6] perf config: Reset the config set at only 'config' sub-command Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 03:20 +0200
    [PATCH v4 4/6] perf config: Reimplement perf_config() using perf_config_set__iter() Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 03:20 +0200
    [PATCH v4 2/6] perf config: Add global variable 'config_set' Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 03:20 +0200
    [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 03:20 +0200
      Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to  initialize config set Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-05-31 15:50 +0200
        Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to  initialize config set Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 19:00 +0200
          Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to  initialize config set Taeung Song <treeze.taeung@gmail.com> - 2016-06-06 13:10 +0200
    [PATCH v4 6/6] perf config: Reimplement show_config() using perf_config() Taeung Song <treeze.taeung@gmail.com> - 2016-05-31 03:20 +0200

#1409768 — [RFC][PATCH v4 0/6] perf config: Reimplement perf_config() using perf_config_set__inter()

Everytime perf_config() is called, perf_config() always read config files.
(i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig')

But we need to use 'struct perf_config_set config_set' variable
that already contains all config key-value pairs
to avoid this repetitive work in perf_config().

In other words, if new perf_config() is called,
only first time 'config_set' is initialized
collecting all configs from config files and it work with perf_config_set__iter().

If we do, 'config_set' can be reused wherever using perf_config()
and a feature of old perf_config() is the same as new perf_config()
work without the repetitive work that read the config files.

IMHO, I think this patchset is needed because not only the repetitive work
should be avoided but also in near future, it would be smooth to manage perf configs.

Most important patch of this patchset is "[PATCH v4 4/6] perf config: Reimplement
perf_config() using perf_config_set__iter()" and PATCH 1/6 ~ 3/6 are preparation for it.

If you give me any feedback, I'd apprecicated it. :)

Thanks,
Taeung

v4:
- Keep perf_config_set__delete() as it is (Arnaldo)
- Remove perf_config_set__check() (Arnaldo)
- Keep the existing code about the config set at cmd_config() (Arnaldo)

v3:
- add freeing config set after sub-command work at run_builtin() (Namhyung)
- remove needless code about the config set at cmd_config()
- add a patch about a global variable 'config_set'

v2:
- split a patch into several patches
- reimplement show_config() using new perf_config()
- modify perf_config_set__delete using global variable 'config_set'
- reset config set when only 'config' sub-commaned work
  because of options for config file location

Taeung Song (6):
  perf config: Use new perf_config_set__init() to initialize config set
  perf config: Add global variable 'config_set'
  perf config: Use zfree() instead of free() at
    perf_config_set__delete()
  perf config: Reimplement perf_config() using perf_config_set__iter()
  perf config: Reset the config set at only 'config' sub-command
  perf config: Reimplement show_config() using perf_config()

 tools/perf/builtin-config.c |  41 +++++-------
 tools/perf/perf.c           |   1 +
 tools/perf/util/cache.h     |   1 +
 tools/perf/util/config.c    | 148 ++++++++++++++++++++++++++++----------------
 tools/perf/util/config.h    |   4 +-
 5 files changed, 115 insertions(+), 80 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1409769 — [PATCH v4 5/6] perf config: Reset the config set at only 'config' sub-command

When first calling perf_config(), the config set is
initialized collecting both user and system config files
(i.e. user config ~/.perfconfig and system config
$(sysconfdir)/perfconfig) so config set contains
not only user config but also system config key-value pairs.
(User config has higher priority than system config.)

But 'config' sub-command individually use the config set
so free the existing config set (i.e. a global variable config_set)
before reinstantiating it.

And 'config' sub-command have '--user' or '--system' options.
To reinitialize with the options, the config set should be reset
at the very beginning at cmd_config()

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index f23fe52..4dab41e 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -79,6 +79,11 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	else if (use_user_config)
 		config_exclusive_filename = user_config;
 
+	/*
+	 * Reset the config set at only 'config' sub-command
+	 * because of reinitializing with options config file location.
+	 */
+	perf_config_set__delete(&config_set);
 	config_set = perf_config_set__new();
 	if (!config_set) {
 		ret = -1;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1409770 — [PATCH v4 4/6] perf config: Reimplement perf_config() using perf_config_set__iter()

Everytime perf_config() is called, perf_config() always read config files.
(i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig')

But we need 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 global variable 'config_set')

In other words, if new perf_config() is called,
only first time 'config_set' is initialized collecting all configs
from the config files and it work with perf_config_set__iter().
And free the config set after sub-command work at run_builtin().

If we do, 'config_set' can be reused wherever using perf_config()
and a feature of old perf_config() is the same as new perf_config() work
without the repetitive work that read the config files.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Wang Nan <wangnan0@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@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/perf.c        |  1 +
 tools/perf/util/cache.h  |  1 +
 tools/perf/util/config.c | 86 +++++++++++++++++++++---------------------------
 3 files changed, 40 insertions(+), 48 deletions(-)

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();
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index 0d814bb..54bbd55 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -7,6 +7,7 @@
 #include <subcmd/pager.h>
 #include "../perf.h"
 #include "../ui/ui.h"
+#include "config.h"
 
 #include <linux/string.h>
 
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 79ded23..bf385ca 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -478,54 +478,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 = 0, found = 0;
-	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)) {
-		ret += perf_config_from_file(fn, perf_etc_perfconfig(),
-					    data);
-		found += 1;
-	}
-
-	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);
-		found += 1;
-out_free:
-		free(user_config);
-	}
-out:
-	if (found == 0)
-		return -1;
-	return ret;
-}
-
 static struct perf_config_section *find_section(struct list_head *sections,
 						const char *section_name)
 {
@@ -706,6 +658,44 @@ struct perf_config_set *perf_config_set__new(void)
 	return set;
 }
 
+static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data)
+{
+	struct perf_config_section *section;
+	struct perf_config_item *item;
+	struct list_head *sections;
+	char key[BUFSIZ];
+
+	if (set == NULL)
+		return -1;
+
+	sections = &set->sections;
+	if (list_empty(sections))
+		return -1;
+
+	list_for_each_entry(section, sections, node) {
+		list_for_each_entry(item, &section->items, node) {
+			char *value = item->value;
+
+			if (value) {
+				scnprintf(key, sizeof(key), "%s.%s",
+					  section->name, item->name);
+				if (fn(key, value, data) < 0)
+					return -1;
+			}
+		}
+	}
+
+	return 0;
+}
+
+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);
+}
+
 static void perf_config_item__delete(struct perf_config_item *item)
 {
 	zfree(&item->name);
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1409771 — [PATCH v4 2/6] perf config: Add global variable 'config_set'

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.
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 5d01899..adf2bad 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

[toc] | [prev] | [next] | [standalone]

#1409772 — [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set

Instead of perf_config(), This function initialize config set
collecting all configs from config files (i.e. user config
~/.perfconfig and system config $(sysconfdir)/perfconfig).

If there are the same config variable both user and system
config file, user config has higher priority than system config.

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/util/config.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index dad7d82..5d01899 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -645,13 +645,61 @@ out_free:
 	return -1;
 }
 
+static int perf_config_set__init(struct perf_config_set *set)
+{
+	int ret = 0, found = 0;
+	const char *home = NULL;
+
+	/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */
+	if (config_exclusive_filename)
+		return perf_config_from_file(collect_config, config_exclusive_filename, set);
+	if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) {
+		ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set);
+		found += 1;
+	}
+
+	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(collect_config, user_config, set);
+		found += 1;
+out_free:
+		free(user_config);
+	}
+out:
+	if (found == 0)
+		return -1;
+	return ret;
+}
+
 struct perf_config_set *perf_config_set__new(void)
 {
 	struct perf_config_set *set = zalloc(sizeof(*set));
 
 	if (set) {
 		INIT_LIST_HEAD(&set->sections);
-		perf_config(collect_config, set);
+		if (perf_config_set__init(set) < 0)
+			return NULL;
 	}
 
 	return set;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1410326 — Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set

Em Tue, May 31, 2016 at 10:13:43AM +0900, Taeung Song escreveu:
> Instead of perf_config(), This function initialize config set
> collecting all configs from config files (i.e. user config
> ~/.perfconfig and system config $(sysconfdir)/perfconfig).
> 
> If there are the same config variable both user and system
> config file, user config has higher priority than system config.
> 
> 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/util/config.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index dad7d82..5d01899 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -645,13 +645,61 @@ out_free:
>  	return -1;
>  }
>  
> +static int perf_config_set__init(struct perf_config_set *set)
> +{
> +	int ret = 0, found = 0;
> +	const char *home = NULL;
> +
> +	/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */
> +	if (config_exclusive_filename)
> +		return perf_config_from_file(collect_config, config_exclusive_filename, set);
> +	if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) {
> +		ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set);
> +		found += 1;
> +	}
> +
> +	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(collect_config, user_config, set);
> +		found += 1;
> +out_free:
> +		free(user_config);
> +	}
> +out:
> +	if (found == 0)
> +		return -1;
> +	return ret;
> +}
> +
>  struct perf_config_set *perf_config_set__new(void)
>  {
>  	struct perf_config_set *set = zalloc(sizeof(*set));
>  
>  	if (set) {
>  		INIT_LIST_HEAD(&set->sections);
> -		perf_config(collect_config, set);
> +		if (perf_config_set__init(set) < 0)
> +			return NULL;

So, the usual pattern is: alloc, init, fail? free, return NULL.

I thought you could've been deviating from that pattern and went to look
at perf_config_set__init() to see if that was doing the freeing in case
of failure, which it shouldn't, it isn't, so I guess this is a leak on
failure, no?

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1410430 — Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set

On 05/31/2016 10:43 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 31, 2016 at 10:13:43AM +0900, Taeung Song escreveu:
>> Instead of perf_config(), This function initialize config set
>> collecting all configs from config files (i.e. user config
>> ~/.perfconfig and system config $(sysconfdir)/perfconfig).
>>
>> If there are the same config variable both user and system
>> config file, user config has higher priority than system config.
>>
>> 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/util/config.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index dad7d82..5d01899 100644
>> --- a/tools/perf/util/config.c
>> +++ b/tools/perf/util/config.c
>> @@ -645,13 +645,61 @@ out_free:
>>   	return -1;
>>   }
>>
>> +static int perf_config_set__init(struct perf_config_set *set)
>> +{
>> +	int ret = 0, found = 0;
>> +	const char *home = NULL;
>> +
>> +	/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */
>> +	if (config_exclusive_filename)
>> +		return perf_config_from_file(collect_config, config_exclusive_filename, set);
>> +	if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) {
>> +		ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set);
>> +		found += 1;
>> +	}
>> +
>> +	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(collect_config, user_config, set);
>> +		found += 1;
>> +out_free:
>> +		free(user_config);
>> +	}
>> +out:
>> +	if (found == 0)
>> +		return -1;
>> +	return ret;
>> +}
>> +
>>   struct perf_config_set *perf_config_set__new(void)
>>   {
>>   	struct perf_config_set *set = zalloc(sizeof(*set));
>>
>>   	if (set) {
>>   		INIT_LIST_HEAD(&set->sections);
>> -		perf_config(collect_config, set);
>> +		if (perf_config_set__init(set) < 0)
>> +			return NULL;
>
> So, the usual pattern is: alloc, init, fail? free, return NULL.
>
> I thought you could've been deviating from that pattern and went to look
> at perf_config_set__init() to see if that was doing the freeing in case
> of failure, which it shouldn't, it isn't, so I guess this is a leak on
> failure, no?
>

You are right. And I found additional problems.

First of all, as you said, if it is failed in perf_config_set__init(),
the config set wouldn't be freed so this is a leak on failure.

Secondly, if it is failed in perf_parse_file(),
perf_parse_file() cannot return because of die()
so perf_config_from_file() and perf_config()
don't also return. I guess this is abnormal termination
without the freeing.
(The important point of this problem is die() at perf_parse_file())

Thirdly, there are problems that are related to collect_config().
If perf_config_from_file(collect_config,..) is failed
the config set will be freed at collect_config() like below.

static int collect_config(const char *var, const char *value,
			  void *perf_config_set)
{

...

out_free:
	free(key);
	perf_config_set__delete(set);
	return -1;
}

And then if calling perf_config_from_file(collect_config,..)
at perf_config_set__init() again,
an error will happen because the config set is NULL at collect_config().
(the error mean NULL pointer exception.)


To conclude,
First of all, I'll send preparatory PATCH set for this patch
to solve the problems i.e.

     1) A problem that perf_config() can't return
        becuase of die() at perf_parse_file()

     2) A problem about the freeing config set at collect_config()

     3) NULL pointer exception at collect_config()

And then I will send changed this patch following above patchset.
(to solve a leak when perf_config_set__init() failed)


Thanks,
Taeung

[toc] | [prev] | [next] | [standalone]

#1414912 — Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set

Hi, Arnaldo :)
Did you have a nice weekend?

I sent this mail for nothing else but to tell the reason of v6 to you.

On 06/01/2016 01:52 AM, Taeung Song wrote:
>
>
> On 05/31/2016 10:43 PM, Arnaldo Carvalho de Melo wrote:
>> Em Tue, May 31, 2016 at 10:13:43AM +0900, Taeung Song escreveu:
>>> Instead of perf_config(), This function initialize config set
>>> collecting all configs from config files (i.e. user config
>>> ~/.perfconfig and system config $(sysconfdir)/perfconfig).
>>>
>>> If there are the same config variable both user and system
>>> config file, user config has higher priority than system config.
>>>
>>> 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/util/config.c | 50
>>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 49 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>>> index dad7d82..5d01899 100644
>>> --- a/tools/perf/util/config.c
>>> +++ b/tools/perf/util/config.c
>>> @@ -645,13 +645,61 @@ out_free:
>>>       return -1;
>>>   }
>>>
>>> +static int perf_config_set__init(struct perf_config_set *set)
>>> +{
>>> +    int ret = 0, found = 0;
>>> +    const char *home = NULL;
>>> +
>>> +    /* Setting $PERF_CONFIG makes perf read _only_ the given config
>>> file. */
>>> +    if (config_exclusive_filename)
>>> +        return perf_config_from_file(collect_config,
>>> config_exclusive_filename, set);
>>> +    if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) {
>>> +        ret += perf_config_from_file(collect_config,
>>> perf_etc_perfconfig(), set);
>>> +        found += 1;
>>> +    }
>>> +
>>> +    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(collect_config, user_config, set);
>>> +        found += 1;
>>> +out_free:
>>> +        free(user_config);
>>> +    }
>>> +out:
>>> +    if (found == 0)
>>> +        return -1;
>>> +    return ret;
>>> +}
>>> +
>>>   struct perf_config_set *perf_config_set__new(void)
>>>   {
>>>       struct perf_config_set *set = zalloc(sizeof(*set));
>>>
>>>       if (set) {
>>>           INIT_LIST_HEAD(&set->sections);
>>> -        perf_config(collect_config, set);
>>> +        if (perf_config_set__init(set) < 0)
>>> +            return NULL;
>>
>> So, the usual pattern is: alloc, init, fail? free, return NULL.
>>
>> I thought you could've been deviating from that pattern and went to look
>> at perf_config_set__init() to see if that was doing the freeing in case
>> of failure, which it shouldn't, it isn't, so I guess this is a leak on
>> failure, no?

I can modify the problem of the leak you said by simple handling
a case of failure at perf_config_set__init().
But I found preexisting small problems so I sent v6 with the three 
[BUGFIX] patches !!

If you can't agree this way to solve the leak,
I'd find other way. :)


Thanks,
Taeung


>
> You are right. And I found additional problems.
>
> First of all, as you said, if it is failed in perf_config_set__init(),
> the config set wouldn't be freed so this is a leak on failure.
>
> Secondly, if it is failed in perf_parse_file(),
> perf_parse_file() cannot return because of die()
> so perf_config_from_file() and perf_config()
> don't also return. I guess this is abnormal termination
> without the freeing.
> (The important point of this problem is die() at perf_parse_file())
>
> Thirdly, there are problems that are related to collect_config().
> If perf_config_from_file(collect_config,..) is failed
> the config set will be freed at collect_config() like below.
>
> static int collect_config(const char *var, const char *value,
>                void *perf_config_set)
> {
>
> ...
>
> out_free:
>      free(key);
>      perf_config_set__delete(set);
>      return -1;
> }
>
> And then if calling perf_config_from_file(collect_config,..)
> at perf_config_set__init() again,
> an error will happen because the config set is NULL at collect_config().
> (the error mean NULL pointer exception.)
>
>
> To conclude,
> First of all, I'll send preparatory PATCH set for this patch
> to solve the problems i.e.
>
>      1) A problem that perf_config() can't return
>         becuase of die() at perf_parse_file()
>
>      2) A problem about the freeing config set at collect_config()
>
>      3) NULL pointer exception at collect_config()
>
> And then I will send changed this patch following above patchset.
> (to solve a leak when perf_config_set__init() failed)
>
>
> Thanks,
> Taeung
>

[toc] | [prev] | [next] | [standalone]

#1409773 — [PATCH v4 6/6] perf config: Reimplement show_config() using perf_config()

Old show_config() directly use config set so
there are many duplicated code with perf_config_set__iter().

So reimplement show_config() using perf_config() that use
perf_config_set__iter() with config set that already
contains all configs.

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 | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 4dab41e..b6ae8ea 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -33,28 +33,13 @@ static struct option config_options[] = {
 	OPT_END()
 };
 
-static int show_config(struct perf_config_set *set)
+static int show_config(const char *key, const char *value,
+		       void *cb __maybe_unused)
 {
-	struct perf_config_section *section;
-	struct perf_config_item *item;
-	struct list_head *sections;
-
-	if (set == NULL)
-		return -1;
-
-	sections = &set->sections;
-	if (list_empty(sections))
-		return -1;
-
-	list_for_each_entry(section, sections, node) {
-		list_for_each_entry(item, &section->items, node) {
-			char *value = item->value;
-
-			if (value)
-				printf("%s.%s=%s\n", section->name,
-				       item->name, value);
-		}
-	}
+	if (value)
+		printf("%s=%s\n", key, value);
+	else
+		printf("%s\n", key);
 
 	return 0;
 }
@@ -96,7 +81,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(config_set);
+			ret = perf_config(show_config, NULL);
 			if (ret < 0) {
 				const char * config_filename = config_exclusive_filename;
 				if (!config_exclusive_filename)
-- 
2.5.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web