linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter()
@ 2016-05-31 17:26 Taeung Song
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 1/9] perf config: Let perf_config() return in case of failure Taeung Song
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song

Hello, :)

This patchset is to reimplement perf_config() for efficient config management.

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 v5 7/9] perf config: Reimplement
perf_config() using perf_config_set__iter()" and PATCH 1/9 ~ 6/9 are preparation for it.

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

Thanks,
Taeung

v5:
- solve a leak when perf_config_set__init() failed (Arnaldo)
- bugfix) let perf_config() return in case of failure
- bugfix) finally free the config set after collect_config() worked
- bugfix) handle NULL pointer exception of 'set' at collect_config()

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 (9):
  perf config: Let perf_config() return in case of failure
  perf config: Finally free a config set after collect_config() worked
  perf config: Handle an error when config set is NULL at
    collect_config()
  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    | 156 ++++++++++++++++++++++++++++----------------
 tools/perf/util/config.h    |   4 +-
 5 files changed, 121 insertions(+), 82 deletions(-)

-- 
2.5.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [BUGFIX][PATCH v5 1/9] perf config: Let perf_config() return in case of failure
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 2/9] perf config: Finally free a config set after collect_config() worked Taeung Song
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

If a config file has wrong key-value pairs,
perf will be forcibly terminated by die() at perf_parse_file()
so terminal setting can be crushed because of abnormal termination.

For example,

If user config file has a wrong value 'red;default'
instead of a normal value like 'red, default' for a key 'colors.top',

    # cat ~/.perfconfig
    [colors]
        medium = red;default # wrong value

and if running sub-command 'top'.

    # perf top

perf process is terminated by force and terminal setting is broken
with a messge like below.

    Fatal: bad config file line 2 in /root/.perfconfig

So fix it.
If perf_config() can return on failure without calling die()
at perf_parse_file(), this problem is solved.

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

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index dad7d82..b500737 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -275,7 +275,8 @@ static int perf_parse_file(config_fn_t fn, void *data)
 			break;
 		}
 	}
-	die("bad config file line %d in %s", config_linenr, config_file_name);
+	pr_err("bad config file line %d in %s\n", config_linenr, config_file_name);
+	return -1;
 }
 
 static int parse_unit_factor(const char *end, unsigned long *val)
@@ -479,16 +480,15 @@ static int perf_config_global(void)
 
 int perf_config(config_fn_t fn, void *data)
 {
-	int ret = 0, found = 0;
+	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)) {
-		ret += perf_config_from_file(fn, perf_etc_perfconfig(),
-					    data);
-		found += 1;
+		if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0)
+			goto out;
 	}
 
 	home = getenv("HOME");
@@ -514,14 +514,12 @@ int perf_config(config_fn_t fn, void *data)
 		if (!st.st_size)
 			goto out_free;
 
-		ret += perf_config_from_file(fn, user_config, data);
-		found += 1;
+		ret = perf_config_from_file(fn, user_config, data);
+
 out_free:
 		free(user_config);
 	}
 out:
-	if (found == 0)
-		return -1;
 	return ret;
 }
 
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [BUGFIX][PATCH v5 2/9] perf config: Finally free a config set after collect_config() worked
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 1/9] perf config: Let perf_config() return in case of failure Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 3/9] perf config: Handle an error when config set is NULL at collect_config() Taeung Song
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

Current colllect_config() free a config set on failure.
But it is needed to finally free it at perf_config_set__new()
after collect_config() worked.

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

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index b500737..d013f90 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -639,7 +639,6 @@ static int collect_config(const char *var, const char *value,
 
 out_free:
 	free(key);
-	perf_config_set__delete(set);
 	return -1;
 }
 
@@ -649,7 +648,8 @@ struct perf_config_set *perf_config_set__new(void)
 
 	if (set) {
 		INIT_LIST_HEAD(&set->sections);
-		perf_config(collect_config, set);
+		if (perf_config(collect_config, set) < 0)
+			perf_config_set__delete(set);
 	}
 
 	return set;
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [BUGFIX][PATCH v5 3/9] perf config: Handle an error when config set is NULL at collect_config()
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 1/9] perf config: Let perf_config() return in case of failure Taeung Song
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 2/9] perf config: Finally free a config set after collect_config() worked Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [PATCH v5 4/9] perf config: Use new perf_config_set__init() to initialize config set Taeung Song
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

collect_config() collect all config key-value pairs
from a config files and put each config in config set.
But if config set (i.e. 'set' variable at collect_config())
is NULL, this is an error so return -1.

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

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index d013f90..062eeb8 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -607,8 +607,12 @@ static int collect_config(const char *var, const char *value,
 	struct perf_config_section *section = NULL;
 	struct perf_config_item *item = NULL;
 	struct perf_config_set *set = perf_config_set;
-	struct list_head *sections = &set->sections;
+	struct list_head *sections;
 
+	if (set == NULL)
+		return -1;
+
+	sections = &set->sections;
 	key = ptr = strdup(var);
 	if (!key) {
 		pr_debug("%s: strdup failed\n", __func__);
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 4/9] perf config: Use new perf_config_set__init() to initialize config set
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
                   ` (2 preceding siblings ...)
  2016-05-31 17:26 ` [BUGFIX][PATCH v5 3/9] perf config: Handle an error when config set is NULL at collect_config() Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [PATCH v5 5/9] perf config: Add global variable 'config_set' Taeung Song
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

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

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 062eeb8..6bfa112 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -646,14 +646,61 @@ out_free:
 	return -1;
 }
 
+static int perf_config_set__init(struct perf_config_set *set)
+{
+	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(collect_config, config_exclusive_filename, set);
+	if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) {
+		if (perf_config_from_file(collect_config, perf_etc_perfconfig(), set) < 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(collect_config, user_config, set);
+
+out_free:
+		free(user_config);
+	}
+out:
+	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);
-		if (perf_config(collect_config, set) < 0)
+		if (perf_config_set__init(set) < 0) {
 			perf_config_set__delete(set);
+			return NULL;
+		}
 	}
 
 	return set;
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 5/9] perf config: Add global variable 'config_set'
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
                   ` (3 preceding siblings ...)
  2016-05-31 17:26 ` [PATCH v5 4/9] perf config: Use new perf_config_set__init() to initialize config set Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [PATCH v5 6/9] perf config: Use zfree() instead of free() at perf_config_set__delete() Taeung Song
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

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 6bfa112..21b7ca8 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 6/9] perf config: Use zfree() instead of free() at perf_config_set__delete()
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
                   ` (4 preceding siblings ...)
  2016-05-31 17:26 ` [PATCH v5 5/9] perf config: Add global variable 'config_set' Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter() Taeung Song
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

perf_config_set__delete() delete allocated the config set
but the global variable 'config_set' is used all around.

So purge and zfree by an address of the global variable
, i.e. 'struct perf_config_set **' type
instead of using local variable 'set' of which type
is 'struct perf_config_set *'.

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 |  2 +-
 tools/perf/util/config.c    | 11 +++++++----
 tools/perf/util/config.h    |  2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index b3bc01a..f23fe52 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -105,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(config_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 21b7ca8..2cae413 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -699,7 +699,7 @@ struct perf_config_set *perf_config_set__new(void)
 	if (set) {
 		INIT_LIST_HEAD(&set->sections);
 		if (perf_config_set__init(set) < 0) {
-			perf_config_set__delete(set);
+			perf_config_set__delete(&set);
 			return NULL;
 		}
 	}
@@ -741,10 +741,13 @@ static void perf_config_set__purge(struct perf_config_set *set)
 	}
 }
 
-void perf_config_set__delete(struct perf_config_set *set)
+void perf_config_set__delete(struct perf_config_set **set)
 {
-	perf_config_set__purge(set);
-	free(set);
+	if (*set == NULL)
+		return;
+
+	perf_config_set__purge(*set);
+	zfree(set);
 }
 
 /*
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index ea157a4..271b429 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -23,6 +23,6 @@ struct perf_config_set {
 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);
+void perf_config_set__delete(struct perf_config_set **set);
 
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter()
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
                   ` (5 preceding siblings ...)
  2016-05-31 17:26 ` [PATCH v5 6/9] perf config: Use zfree() instead of free() at perf_config_set__delete() Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [PATCH v5 8/9] perf config: Reset the config set at only 'config' sub-command Taeung Song
  2016-05-31 17:26 ` [PATCH v5 9/9] perf config: Reimplement show_config() using perf_config() Taeung Song
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa, Wang Nan, Ingo Molnar

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 | 83 ++++++++++++++++++++++--------------------------
 3 files changed, 40 insertions(+), 45 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 2cae413..d4d204a 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -479,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)
 {
@@ -707,6 +662,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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 8/9] perf config: Reset the config set at only 'config' sub-command
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
                   ` (6 preceding siblings ...)
  2016-05-31 17:26 ` [PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter() Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  2016-05-31 17:26 ` [PATCH v5 9/9] perf config: Reimplement show_config() using perf_config() Taeung Song
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 9/9] perf config: Reimplement show_config() using perf_config()
  2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
                   ` (7 preceding siblings ...)
  2016-05-31 17:26 ` [PATCH v5 8/9] perf config: Reset the config set at only 'config' sub-command Taeung Song
@ 2016-05-31 17:26 ` Taeung Song
  8 siblings, 0 replies; 10+ messages in thread
From: Taeung Song @ 2016-05-31 17:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Alexander Shishkin, Masami Hiramatsu,
	Taeung Song, Jiri Olsa

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-05-31 17:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-31 17:26 [RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter() Taeung Song
2016-05-31 17:26 ` [BUGFIX][PATCH v5 1/9] perf config: Let perf_config() return in case of failure Taeung Song
2016-05-31 17:26 ` [BUGFIX][PATCH v5 2/9] perf config: Finally free a config set after collect_config() worked Taeung Song
2016-05-31 17:26 ` [BUGFIX][PATCH v5 3/9] perf config: Handle an error when config set is NULL at collect_config() Taeung Song
2016-05-31 17:26 ` [PATCH v5 4/9] perf config: Use new perf_config_set__init() to initialize config set Taeung Song
2016-05-31 17:26 ` [PATCH v5 5/9] perf config: Add global variable 'config_set' Taeung Song
2016-05-31 17:26 ` [PATCH v5 6/9] perf config: Use zfree() instead of free() at perf_config_set__delete() Taeung Song
2016-05-31 17:26 ` [PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter() Taeung Song
2016-05-31 17:26 ` [PATCH v5 8/9] perf config: Reset the config set at only 'config' sub-command Taeung Song
2016-05-31 17:26 ` [PATCH v5 9/9] perf config: Reimplement show_config() using perf_config() Taeung Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).