Re: [PATCH v10 2/3] perf config: Reimplement perf_config() introducing new perf_config_init() and perf_config_finish()

From: Taeung Song
Date: Thu Jun 23 2016 - 08:41:53 EST

Hi, Arnaldo :)

On 06/23/2016 08:56 PM, Arnaldo Carvalho de Melo wrote:
Em Thu, Jun 23, 2016 at 05:55:18PM +0900, Taeung Song escreveu:
Many sub-commands use perf_config() but
everytime perf_config() is called, perf_config() always read config files.
(i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig')

But it is better 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 static variable 'config_set')

In other words, if new perf_config_init() is called,
only first time 'config_set' is initialized collecting all configs from the config files.
And then we could use new perf_config() like old perf_config().
When a sub-command finished, free the config set by perf_config_finish() at run_builtin().

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

In summary, in order to use features about configuration,
we can call the functions at perf.c and other source files as below.

# initialize a config set
perf_config_init()

# configure actual variables from a config set
perf_config()

# eliminate allocated config set
perf_config_finish()

# destroy existing config set and initialize a new config set.
perf_config_refresh()

Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Wang Nan <wangnan0@xxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
---
tools/perf/builtin-config.c | 4 ++
tools/perf/perf.c | 2 +
tools/perf/util/config.c | 92 +++++++++++++++++++++++----------------------
tools/perf/util/config.h | 29 ++++++++++++++
4 files changed, 82 insertions(+), 45 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index fe1b77f..cfd1036 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
else if (use_user_config)
config_exclusive_filename = user_config;

+ /*
+ * At only 'config' sub-command, individually use the config set
+ * because of reinitializing with options config file location.
+ */
set = perf_config_set__new();
if (!set) {
ret = -1;
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 66772da..280967e 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -355,6 +355,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_finish();
exit_browser(status);
perf_env__exit(&perf_env);
bpf__clear();
@@ -522,6 +523,7 @@ int main(int argc, const char **argv)

srandom(time(NULL));

+ perf_config_init();
perf_config(perf_default_config, NULL);
set_buildid_dir(NULL);

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index d15c592..a16f95d 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -26,6 +26,7 @@ static FILE *config_file;
static const char *config_file_name;
static int config_linenr;
static int config_file_eof;
+static struct perf_config_set *config_set;

const char *config_exclusive_filename;

@@ -478,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)
{
@@ -706,6 +662,52 @@ struct perf_config_set *perf_config_set__new(void)
return set;
}

+int perf_config(config_fn_t fn, void *data)
+{
+ int ret = 0;
+ char key[BUFSIZ];
+ struct perf_config_section *section;
+ struct perf_config_item *item;
+
+ if (config_set == NULL)
+ return -1;
+
+ config_set__for_each(config_set, section, item) {
+ char *value = item->value;
+
+ if (value) {
+ scnprintf(key, sizeof(key), "%s.%s",
+ section->name, item->name);
+ ret = fn(key, value, data);
+ if (ret < 0) {
+ pr_err("Error: wrong config key-value pair %s=%s\n",
+ key, value);
+ break;
+ }
+ }
+ }
+
+ return ret;
+}
+
+void perf_config_init(void)
+{
+ if (config_set == NULL)
+ config_set = perf_config_set__new();
+}
+
+void perf_config_finish(void)
+{
+ perf_config_set__delete(config_set);
+ config_set = NULL;
+}
+
+void perf_config_refresh(void)
+{
+ perf_config_finish();
+ perf_config_init();
+}
+
static void perf_config_item__delete(struct perf_config_item *item)
{
zfree(&item->name);
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 155a441..746c619 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -33,5 +33,34 @@ const char *perf_etc_perfconfig(void);

struct perf_config_set *perf_config_set__new(void);
void perf_config_set__delete(struct perf_config_set *set);
+void perf_config_init(void);
+void perf_config_finish(void);
+void perf_config_refresh(void);

Please use double _ to separate the subsystem/class from its methods,
i.e.:

void perf_config__init(void);
void perf_config__finish(void);
void perf_config__refresh(void);

I got it :)

+
+/**
+ * config_sections__for_each - iterate thru all the sections
+ * @list: list_head instance to iterate
+ * @section: struct perf_config_section iterator
+ */
+#define config_sections__for_each(list, section) \
+ list_for_each_entry(section, list, node)

These macros operate on perf_config_sections, so please name it
accordingly, i.e.:
perf_config_sections__for_each()

+
+/**
+ * config_items__for_each - iterate thru all the items
+ * @list: list_head instance to iterate
+ * @item: struct perf_config_item iterator
+ */
+#define config_items__for_each(list, item) \
+ list_for_each_entry(item, list, node)
+

Ditto

+/**
+ * config_set__for_each - iterate thru all the config section-item pairs
+ * @set: evlist instance to iterate
+ * @section: struct perf_config_section iterator
+ * @item: struct perf_config_item iterator
+ */
+#define config_set__for_each(set, section, item) \
+ config_sections__for_each(&set->sections, section) \
+ config_items__for_each(&section->items, item)

Ditto.


Granted.

I'll send v11 soon with this change!

Thanks,
Taeung