[RFC][PATCH v3 0/7] perf config: Reimplement perf_config() using perf_config_set__inter()

[RFC][PATCH v3 0/7] perf config: Reimplement perf_config() using perf_config_set__inter()

[Taeung Song]

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

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

Thanks,
Taeung

v3:
- add freeing config set after sub-command work at run_builtin() (Namhyung)
- remove needless code about 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 (7):
  perf config: Use new perf_config_set__init() to initialize config set
  perf config: Add global variable 'config_set'
  perf config: Modify perf_config_set__delete() using global variable
    'config_set'
  perf config: Reimplement perf_config() using perf_config_set__iter()
  perf config: Reimplement show_config() using perf_config()
  perf config: Remove needless code about config set at cmd_config()
  perf config: Reset the config set at only 'config' sub-command

 tools/perf/builtin-config.c |  42 ++++--------
 tools/perf/perf.c           |   1 +
 tools/perf/util/cache.h     |   1 +
 tools/perf/util/config.c    | 156 +++++++++++++++++++++++++++++---------------
 tools/perf/util/config.h    |   2 +-
 5 files changed, 118 insertions(+), 84 deletions(-)

-- 
2.5.0

[PATCH v3 1/7] perf config: Use new perf_config_set__init() to initialize config set

[Taeung Song]

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

[PATCH v3 2/7] perf config: Add global variable 'config_set'

[Taeung Song]

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

[PATCH v3 3/7] perf config: Modify perf_config_set__delete() using global variable 'config_set'

[Taeung Song]

This function deleted allocated config set but
the global variable 'config_set' is used all around
so this directly remove 'config_set' instead of using local variable '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    | 8 ++++----
 tools/perf/util/config.h    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index b3bc01a..255015e 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();
 out_err:
 	return ret;
 }
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index adf2bad..68def9a 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -642,7 +642,7 @@ static int collect_config(const char *var, const char *value,
 
 out_free:
 	free(key);
-	perf_config_set__delete(set);
+	perf_config_set__delete();
 	return -1;
 }
 
@@ -740,10 +740,10 @@ 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(void)
 {
-	perf_config_set__purge(set);
-	free(set);
+	perf_config_set__purge(config_set);
+	zfree(&config_set);
 }
 
 /*
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index ea157a4..e9b47b3 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(void);
 
 #endif /* __PERF_CONFIG_H */
-- 
2.5.0

[PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter()

[Taeung Song]

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 | 97 ++++++++++++++++++++++++------------------------
 3 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 15982ce..32e54d9 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();
 	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 68def9a..abfe1b2 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,55 @@ struct perf_config_set *perf_config_set__new(void)
 	return set;
 }
 
+static int perf_config_set__check(void)
+{
+	if (config_set != NULL)
+		return 0;
+
+	config_set = perf_config_set__new();
+	if (!config_set)
+		return -1;
+
+	return 0;
+}
+
+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 (perf_config_set__check() < 0)
+		return -1;
+	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

[PATCH v3 5/7] perf config: Reimplement show_config() using perf_config()

[Taeung Song]

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 255015e..cf6c396 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;
 }
@@ -91,7 +76,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

[PATCH v3 6/7] perf config: Remove needless code about config set at cmd_config()

[Taeung Song]

show_config() was reimplemented using perf_config()
so it isn't needed to use perf_config_set__new() at cmd_config().
And perf_config_set__delete() isn't needed at cmd_config() because of
calling the function at run_builtin() when a sub-command finished.

And it isn't also needed to declare 'config_set' as extern variable
because the variable is only handled at util/config.c from now on.

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 | 8 --------
 tools/perf/util/config.h    | 2 --
 2 files changed, 10 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index cf6c396..412c725 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -64,12 +64,6 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	else if (use_user_config)
 		config_exclusive_filename = user_config;
 
-	config_set = perf_config_set__new();
-	if (!config_set) {
-		ret = -1;
-		goto out_err;
-	}
-
 	switch (actions) {
 	case ACTION_LIST:
 		if (argc) {
@@ -90,7 +84,5 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 		usage_with_options(config_usage, config_options);
 	}
 
-	perf_config_set__delete();
-out_err:
 	return ret;
 }
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index e9b47b3..be4e603 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -20,8 +20,6 @@ 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(void);
 
-- 
2.5.0

[PATCH v3 7/7] perf config: Reset the config set at only 'config' sub-command

[Taeung Song]

When first calling perf_config(), 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 have '--user' or '--system' options.
The options is to select a particular config file location
so the config set should be reset before 'config' sub-command work.

User config file:

    # cat ~/.perfconfig
    [annotate]
        hide_src_code = false
    [tui]
        report = on

System config file:

    # cat /usr/local/etc/perfconfig
    [annotate]
        hide_src_code = true

Before:

    # perf config --user --list
    annotate.hide_src_code=false
    ui.report=on

    # perf config --system --list
    annotate.hide_src_code=false
    tui.report=on

After:

    # perf config --user --list
    annotate.hide_src_code=false
    tui.report=on

    # perf config --system --list
    annotate.hide_src_code=true

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 412c725..5615631 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -64,6 +64,12 @@ 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 options config file location.
+	 */
+	perf_config_set__delete();
+
 	switch (actions) {
 	case ACTION_LIST:
 		if (argc) {
-- 
2.5.0

Re: [PATCH v3 3/7] perf config: Modify perf_config_set__delete() using global variable 'config_set'

[Arnaldo Carvalho de Melo]

Em Tue, May 31, 2016 at 01:44:07AM +0900, Taeung Song escreveu:
> This function deleted allocated config set but
> the global variable 'config_set' is used all around
> so this directly remove 'config_set' instead of using local variable 'set'.

Keep it like it is, i.e. foo__delete() acts on something returned by
foo__new(), for consistency.

We may want to, for instance, have two instances returned by foo__new()
and then call the destructor for each.

- Arnaldo
 
> 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    | 8 ++++----
>  tools/perf/util/config.h    | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index b3bc01a..255015e 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();
>  out_err:
>  	return ret;
>  }
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index adf2bad..68def9a 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -642,7 +642,7 @@ static int collect_config(const char *var, const char *value,
>  
>  out_free:
>  	free(key);
> -	perf_config_set__delete(set);
> +	perf_config_set__delete();
>  	return -1;
>  }
>  
> @@ -740,10 +740,10 @@ 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(void)
>  {
> -	perf_config_set__purge(set);
> -	free(set);
> +	perf_config_set__purge(config_set);
> +	zfree(&config_set);
>  }
>  
>  /*
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index ea157a4..e9b47b3 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(void);
>  
>  #endif /* __PERF_CONFIG_H */
> -- 
> 2.5.0

Re: [PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter()

[Arnaldo Carvalho de Melo]

Em Tue, May 31, 2016 at 01:44:08AM +0900, Taeung Song escreveu:
> +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;
<SNIP>
> +	return 0;
> +}
> +int perf_config(config_fn_t fn, void *data)
> +{
> +	if (perf_config_set__check() < 0)
> +		return -1;
> +	return perf_config_set__iter(config_set, fn, data);
> +}

"check" looks too vague, this is equivalent, no?

int perf_config(config_fn_t, void *data)
{
	if (config_set == NULL)
		config_set = perf_config_set__new();
	
	return perf_config_set__iter(config_set, fn, data);
}

- Arnaldo

Re: [PATCH v3 6/7] perf config: Remove needless code about config set at cmd_config()

[Arnaldo Carvalho de Melo]

Em Tue, May 31, 2016 at 01:44:10AM +0900, Taeung Song escreveu:
> show_config() was reimplemented using perf_config()
> so it isn't needed to use perf_config_set__new() at cmd_config().
> And perf_config_set__delete() isn't needed at cmd_config() because of
> calling the function at run_builtin() when a sub-command finished.
> 
> And it isn't also needed to declare 'config_set' as extern variable
> because the variable is only handled at util/config.c from now on.

But the existing code looks more natural, i.e. before dealing with the
config_set, we try instantiating it, handling errors at constructor
calling time, etc.

Then, finally, calling the destructor when we don't need it anymore.
 
> 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 | 8 --------
>  tools/perf/util/config.h    | 2 --
>  2 files changed, 10 deletions(-)
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index cf6c396..412c725 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -64,12 +64,6 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  	else if (use_user_config)
>  		config_exclusive_filename = user_config;
>  
> -	config_set = perf_config_set__new();
> -	if (!config_set) {
> -		ret = -1;
> -		goto out_err;
> -	}
> -
>  	switch (actions) {
>  	case ACTION_LIST:
>  		if (argc) {
> @@ -90,7 +84,5 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  		usage_with_options(config_usage, config_options);
>  	}
>  
> -	perf_config_set__delete();
> -out_err:
>  	return ret;
>  }
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index e9b47b3..be4e603 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -20,8 +20,6 @@ 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(void);
>  
> -- 
> 2.5.0

Re: [PATCH v3 3/7] perf config: Modify perf_config_set__delete() using global variable 'config_set'

[Taeung Song]

Hi, Arnaldo :-D

On 05/31/2016 04:29 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 31, 2016 at 01:44:07AM +0900, Taeung Song escreveu:
>> This function deleted allocated config set but
>> the global variable 'config_set' is used all around
>> so this directly remove 'config_set' instead of using local variable 'set'.
>
> Keep it like it is, i.e. foo__delete() acts on something returned by
> foo__new(), for consistency.
>
> We may want to, for instance, have two instances returned by foo__new()
> and then call the destructor for each.
>

I understood it.
I'll just keep the function as it is
i.e. void perf_config_set__delete(struct perf_config_set *set)
, for consistency.

And at [PATCH v3 4/7] perf config: Reimplement perf_config() using 
perf_config_set__iter()
when calling the function at run_builtin(),
I'll modify it like below.

@@ -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();


Thanks,
Taeung

>> 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    | 8 ++++----
>>   tools/perf/util/config.h    | 2 +-
>>   3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index b3bc01a..255015e 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();
>>   out_err:
>>   	return ret;
>>   }
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index adf2bad..68def9a 100644
>> --- a/tools/perf/util/config.c
>> +++ b/tools/perf/util/config.c
>> @@ -642,7 +642,7 @@ static int collect_config(const char *var, const char *value,
>>
>>   out_free:
>>   	free(key);
>> -	perf_config_set__delete(set);
>> +	perf_config_set__delete();
>>   	return -1;
>>   }
>>
>> @@ -740,10 +740,10 @@ 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(void)
>>   {
>> -	perf_config_set__purge(set);
>> -	free(set);
>> +	perf_config_set__purge(config_set);
>> +	zfree(&config_set);
>>   }
>>
>>   /*
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index ea157a4..e9b47b3 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(void);
>>
>>   #endif /* __PERF_CONFIG_H */
>> --
>> 2.5.0

Re: [PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter()

[Taeung Song]

On 05/31/2016 04:32 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 31, 2016 at 01:44:08AM +0900, Taeung Song escreveu:
>> +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;
> <SNIP>
>> +	return 0;
>> +}
>> +int perf_config(config_fn_t fn, void *data)
>> +{
>> +	if (perf_config_set__check() < 0)
>> +		return -1;
>> +	return perf_config_set__iter(config_set, fn, data);
>> +}
>
> "check" looks too vague, this is equivalent, no?
>
> int perf_config(config_fn_t, void *data)
> {
> 	if (config_set == NULL)
> 		config_set = perf_config_set__new();
> 	
> 	return perf_config_set__iter(config_set, fn, data);
> }
>

I understood it! I thought __check() function is needed for readability.
But I'll remove __check() because it would seem that the function isn't 
needed as you said.

Thanks,
Taeung

Re: [PATCH v3 6/7] perf config: Remove needless code about config set at cmd_config()

[Taeung Song]

On 05/31/2016 04:35 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 31, 2016 at 01:44:10AM +0900, Taeung Song escreveu:
>> show_config() was reimplemented using perf_config()
>> so it isn't needed to use perf_config_set__new() at cmd_config().
>> And perf_config_set__delete() isn't needed at cmd_config() because of
>> calling the function at run_builtin() when a sub-command finished.
>>
>> And it isn't also needed to declare 'config_set' as extern variable
>> because the variable is only handled at util/config.c from now on.
>
> But the existing code looks more natural, i.e. before dealing with the
> config_set, we try instantiating it, handling errors at constructor
> calling time, etc.
>
> Then, finally, calling the destructor when we don't need it anymore.

I understood it.
The important thing is when it is instantiated or isn't needed,
not where it is handled (i.e. at util/config.c). right ?

So we need to use the config set as extern variable
and it would be better to keep the existing code at cmd_config() as you 
said.

I'll send v4 with modified code soon ! :-)

Thanks,
Taeung

>> 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 | 8 --------
>>   tools/perf/util/config.h    | 2 --
>>   2 files changed, 10 deletions(-)
>>
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index cf6c396..412c725 100644
>> --- a/tools/perf/builtin-config.c
>> +++ b/tools/perf/builtin-config.c
>> @@ -64,12 +64,6 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>>   	else if (use_user_config)
>>   		config_exclusive_filename = user_config;
>>
>> -	config_set = perf_config_set__new();
>> -	if (!config_set) {
>> -		ret = -1;
>> -		goto out_err;
>> -	}
>> -
>>   	switch (actions) {
>>   	case ACTION_LIST:
>>   		if (argc) {
>> @@ -90,7 +84,5 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>>   		usage_with_options(config_usage, config_options);
>>   	}
>>
>> -	perf_config_set__delete();
>> -out_err:
>>   	return ret;
>>   }
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index e9b47b3..be4e603 100644
>> --- a/tools/perf/util/config.h
>> +++ b/tools/perf/util/config.h
>> @@ -20,8 +20,6 @@ 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(void);
>>
>> --
>> 2.5.0