linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/4] Infrastructure code for perf-config
@ 2016-04-04  9:17 Taeung Song
  2016-04-04  9:17 ` [PATCH v6 1/4] perf config: Introduce perf_config_set class Taeung Song
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Taeung Song @ 2016-04-04  9:17 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Masami Hiramatsu, Taeung Song

Hi,

We can use the config files (i.e user wide ~/.perfconfig
and system wide $(sysconfdir)/perfconfig)
to configure perf tools. perf-config help user
manage the config files, not manually look into or edit them.

Introduce new infrastructure code for config
management features of perf-config subcommand.

This pathset contains basic code for various purposes of configuration management
showing current configs, in the near future,
showing all configs with default value,
getting current configs from the config files
or writing configs that user type on the config files, etc.

IMHO, I think this infrastructure code is needed
to add new funcationalities for config management of perf-config.

If anyone reviews this, I'd appreciate it.

Thanks,
Taeung

v6:
- don't use goto in add_config_item() (Masami)

v5:
- departmentalize perf_config_set__delete() (Arnaldo)
- remove confusing find_config() (Arnaldo)
- use pr_debug() instead of pr_err() (Arnaldo)
- use zfree() instead of free() (Arnaldo)
- more compact in perf_config_set__new() (Arnaldo)
- rename variables 'perf_configs', 'config_items', etc. (Arnaldo)

v4:
- fill perf_config_set__delete() in collect_config() for state of error
- fill the code setting is_custom value in add_config_item() (Namhyung)

v3:
- use the section list that contains configs each section
  instead of the single config list (Namhyung)
- exclude a patch for '--list-all' option from this patchset

v2:
- remove perf_config_kind (user, system or both config files)
  and needless at this time, etc. (Namhyung)
- separate this patch as several patches (Namhyung)
- fix typing errors, etc.

Taeung Song (4):
  perf config: Introduce perf_config_set class
  perf config: Let show_config() work with perf_config_set
  perf config: Prepare all default configs
  perf config: Initialize perf_config_set with all default configs

 tools/perf/builtin-config.c |  38 +++++-
 tools/perf/util/config.c    | 306 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/config.h    |  91 +++++++++++++
 3 files changed, 428 insertions(+), 7 deletions(-)
 create mode 100644 tools/perf/util/config.h

-- 
2.5.0

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH v6 1/4] perf config: Introduce perf_config_set class
  2016-04-04  9:17 [PATCH v6 0/4] Infrastructure code for perf-config Taeung Song
@ 2016-04-04  9:17 ` Taeung Song
  2016-04-04 12:41   ` Masami Hiramatsu
  2016-04-04  9:17 ` [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set Taeung Song
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Taeung Song @ 2016-04-04  9:17 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Masami Hiramatsu, Taeung Song

This infrastructure code was designed for
upcoming features of perf-config.

That collect config key-value pairs from user and
system config files (i.e. user wide ~/.perfconfig
and system wide $(sysconfdir)/perfconfig)
to manage perf's configs.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/config.h |  26 +++++++
 2 files changed, 199 insertions(+)
 create mode 100644 tools/perf/util/config.h

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 5c20d78..53e756e 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -13,6 +13,7 @@
 #include <subcmd/exec-cmd.h>
 #include "util/hist.h"  /* perf_hist_config */
 #include "util/llvm-utils.h"   /* perf_llvm_config */
+#include "config.h"
 
 #define MAXNAME (256)
 
@@ -524,6 +525,178 @@ out:
 	return ret;
 }
 
+static struct perf_config_section *find_section(struct list_head *sections,
+						const char *section_name)
+{
+	struct perf_config_section *section;
+
+	list_for_each_entry(section, sections, node)
+		if (!strcmp(section->name, section_name))
+			return section;
+
+	return NULL;
+}
+
+static struct perf_config_item *find_config_item(const char *name,
+						 struct perf_config_section *section)
+{
+	struct perf_config_item *item;
+
+	list_for_each_entry(item, &section->items, node)
+		if (!strcmp(item->name, name))
+			return item;
+
+	return NULL;
+}
+
+static struct perf_config_section *add_section(struct list_head *sections,
+					       const char *section_name)
+{
+	struct perf_config_section *section = zalloc(sizeof(*section));
+
+	if (!section)
+		return NULL;
+
+	INIT_LIST_HEAD(&section->items);
+	section->name = strdup(section_name);
+	if (!section->name) {
+		pr_debug("%s: strdup failed\n", __func__);
+		free(section);
+		return NULL;
+	}
+
+	list_add_tail(&section->node, sections);
+	return section;
+}
+
+static struct perf_config_item *add_config_item(struct perf_config_section *section,
+						const char *name)
+{
+	struct perf_config_item *item = zalloc(sizeof(*item));
+
+	if (!item)
+		return NULL;
+
+	item->name = strdup(name);
+	if (!item->name) {
+		pr_debug("%s: strdup failed\n", __func__);
+		free(item);
+		return NULL;
+	}
+
+	list_add_tail(&item->node, &section->items);
+	return item;
+}
+
+static int set_value(struct perf_config_item *item, const char *value)
+{
+	char *val = strdup(value);
+
+	if (!val)
+		return -1;
+
+	free(item->value);
+	item->value = val;
+	return 0;
+}
+
+static int collect_config(const char *var, const char *value,
+			  void *perf_config_set)
+{
+	int ret = -1;
+	char *ptr, *key;
+	char *section_name, *name;
+	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;
+
+	key = ptr = strdup(var);
+	if (!key) {
+		pr_debug("%s: strdup failed\n", __func__);
+		return -1;
+	}
+
+	section_name = strsep(&ptr, ".");
+	name = ptr;
+	if (name == NULL || value == NULL)
+		goto out_free;
+
+	section = find_section(sections, section_name);
+	if (!section) {
+		section = add_section(sections, section_name);
+		if (!section)
+			goto out_free;
+	}
+
+	item = find_config_item(name, section);
+	if (!item) {
+		item = add_config_item(section, name);
+		if (!item)
+			goto out_free;
+	}
+
+	ret = set_value(item, value);
+	return ret;
+
+out_free:
+	free(key);
+	perf_config_set__delete(set);
+	return -1;
+}
+
+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);
+	}
+
+	return set;
+}
+
+static void perf_config_item__delete(struct perf_config_item *item)
+{
+	zfree(&item->name);
+	zfree(&item->value);
+	free(item);
+}
+
+static void perf_config_section__purge(struct perf_config_section *section)
+{
+	struct perf_config_item *item, *tmp;
+
+	list_for_each_entry_safe(item, tmp, &section->items, node) {
+		list_del_init(&item->node);
+		perf_config_item__delete(item);
+	}
+}
+
+static void perf_config_section__delete(struct perf_config_section *section)
+{
+	perf_config_section__purge(section);
+	zfree(&section->name);
+	free(section);
+}
+
+static void perf_config_set__purge(struct perf_config_set *set)
+{
+	struct perf_config_section *section, *tmp;
+
+	list_for_each_entry_safe(section, tmp, &set->sections, node) {
+		list_del_init(&section->node);
+		perf_config_section__delete(section);
+	}
+}
+
+void perf_config_set__delete(struct perf_config_set *set)
+{
+	perf_config_set__purge(set);
+	free(set);
+}
+
 /*
  * Call this to report error for your variable that should not
  * get a boolean value (i.e. "[my] var" means "true").
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
new file mode 100644
index 0000000..22ec626
--- /dev/null
+++ b/tools/perf/util/config.h
@@ -0,0 +1,26 @@
+#ifndef __PERF_CONFIG_H
+#define __PERF_CONFIG_H
+
+#include <stdbool.h>
+#include <linux/list.h>
+
+struct perf_config_item {
+	char *name;
+	char *value;
+	struct list_head node;
+};
+
+struct perf_config_section {
+	char *name;
+	struct list_head items;
+	struct list_head node;
+};
+
+struct perf_config_set {
+	struct list_head sections;
+};
+
+struct perf_config_set *perf_config_set__new(void);
+void perf_config_set__delete(struct perf_config_set *set);
+
+#endif /* __PERF_CONFIG_H */
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set
  2016-04-04  9:17 [PATCH v6 0/4] Infrastructure code for perf-config Taeung Song
  2016-04-04  9:17 ` [PATCH v6 1/4] perf config: Introduce perf_config_set class Taeung Song
@ 2016-04-04  9:17 ` Taeung Song
  2016-04-04 12:49   ` Masami Hiramatsu
  2016-04-04  9:17 ` [PATCH v6 3/4] perf config: Prepare all default configs Taeung Song
  2016-04-04  9:17 ` [PATCH v6 4/4] perf config: Initialize perf_config_set with " Taeung Song
  3 siblings, 1 reply; 14+ messages in thread
From: Taeung Song @ 2016-04-04  9:17 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Masami Hiramatsu, Taeung Song

Current show_config() has a problem when user or
system config files have same config variables i.e.

    # cat ~/.perfconfig
        [top]
            children = false

    when $(sysconfdir) is /usr/local/etc
    # cat /usr/local/etc/perfconfig
        [top]
            children = true

Before:
    # perf config --user --list
    top.children=false

    # perf config --system --list
    top.children=true

    # perf config --list
    top.children=true
    top.children=false

Because perf_config() can call show_config()
each the config file (user and system).
So fix it.

After:
    # perf config --user --list
    top.children=false

    # perf config --system --list
    top.children=true

    # perf config --list
    top.children=false

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c42448e..1133224 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -12,6 +12,7 @@
 #include <subcmd/parse-options.h>
 #include "util/util.h"
 #include "util/debug.h"
+#include "util/config.h"
 
 static bool use_system_config, use_user_config;
 
@@ -32,13 +33,24 @@ static struct option config_options[] = {
 	OPT_END()
 };
 
-static int show_config(const char *key, const char *value,
-		       void *cb __maybe_unused)
+static int show_config(struct perf_config_set *set)
 {
-	if (value)
-		printf("%s=%s\n", key, value);
-	else
-		printf("%s\n", key);
+	struct perf_config_section *section;
+	struct perf_config_item *item;
+	struct list_head *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);
+		}
+	}
 
 	return 0;
 }
@@ -46,6 +58,7 @@ static int show_config(const char *key, const char *value,
 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,
@@ -63,13 +76,19 @@ 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) {
+		ret = -1;
+		goto out_err;
+	}
+
 	switch (actions) {
 	case ACTION_LIST:
 		if (argc) {
 			pr_err("Error: takes no arguments\n");
 			parse_options_usage(config_usage, config_options, "l", 1);
 		} else {
-			ret = perf_config(show_config, NULL);
+			ret = show_config(set);
 			if (ret < 0) {
 				const char * config_filename = config_exclusive_filename;
 				if (!config_exclusive_filename)
@@ -83,5 +102,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);
+out_err:
 	return ret;
 }
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH v6 3/4] perf config: Prepare all default configs
  2016-04-04  9:17 [PATCH v6 0/4] Infrastructure code for perf-config Taeung Song
  2016-04-04  9:17 ` [PATCH v6 1/4] perf config: Introduce perf_config_set class Taeung Song
  2016-04-04  9:17 ` [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set Taeung Song
@ 2016-04-04  9:17 ` Taeung Song
  2016-04-04  9:17 ` [PATCH v6 4/4] perf config: Initialize perf_config_set with " Taeung Song
  3 siblings, 0 replies; 14+ messages in thread
From: Taeung Song @ 2016-04-04  9:17 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Masami Hiramatsu, Taeung Song

To precisely manage configs,
prepare all default perf's configs that contain
default section name, variable name, value
and correct type, not string type.

In the near future, this will be used when
checking type of config variable or showing
all configs with default values, etc.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/config.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/config.h |  62 +++++++++++++++++++++++++-
 2 files changed, 168 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 53e756e..f937124 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -15,6 +15,7 @@
 #include "util/llvm-utils.h"   /* perf_llvm_config */
 #include "config.h"
 
+#define MAX_CONFIGS 64
 #define MAXNAME (256)
 
 #define DEBUG_CACHE_DIR ".debug"
@@ -29,6 +30,111 @@ static int config_file_eof;
 
 const char *config_exclusive_filename;
 
+struct perf_config_section default_sections[] = {
+	{ .name = "colors" },
+	{ .name = "tui" },
+	{ .name = "buildid" },
+	{ .name = "annotate" },
+	{ .name = "gtk" },
+	{ .name = "pager" },
+	{ .name = "help" },
+	{ .name = "hist" },
+	{ .name = "ui" },
+	{ .name = "call-graph" },
+	{ .name = "report" },
+	{ .name = "top" },
+	{ .name = "man" },
+	{ .name = "kmem" }
+};
+
+struct perf_config_item default_config_items[][MAX_CONFIGS] = {
+	[CONFIG_COLORS] = {
+		CONF_STR_VAR("top", "red, default"),
+		CONF_STR_VAR("medium", "green, default"),
+		CONF_STR_VAR("normal", "lightgray, default"),
+		CONF_STR_VAR("selected", "white, lightgray"),
+		CONF_STR_VAR("jump_arrows", "blue, default"),
+		CONF_STR_VAR("addr", "magenta, default"),
+		CONF_STR_VAR("root", "white, blue"),
+		CONF_END()
+	},
+	[CONFIG_TUI] = {
+		CONF_BOOL_VAR("report", true),
+		CONF_BOOL_VAR("annotate", true),
+		CONF_BOOL_VAR("top", true),
+		CONF_END()
+	},
+	[CONFIG_BUILDID] = {
+		CONF_STR_VAR("dir", "~/.debug"),
+		CONF_END()
+	},
+	[CONFIG_ANNOTATE] = {
+		CONF_BOOL_VAR("hide_src_code", false),
+		CONF_BOOL_VAR("use_offset", true),
+		CONF_BOOL_VAR("jump_arrows", true),
+		CONF_BOOL_VAR("show_nr_jumps", false),
+		CONF_BOOL_VAR("show_linenr", false),
+		CONF_BOOL_VAR("show_total_period", false),
+		CONF_END()
+	},
+	[CONFIG_GTK] = {
+		CONF_BOOL_VAR("annotate", false),
+		CONF_BOOL_VAR("report", false),
+		CONF_BOOL_VAR("top", false),
+		CONF_END()
+	},
+	[CONFIG_PAGER] = {
+		CONF_BOOL_VAR("cmd", true),
+		CONF_BOOL_VAR("report", true),
+		CONF_BOOL_VAR("annotate", true),
+		CONF_BOOL_VAR("top", true),
+		CONF_BOOL_VAR("diff", true),
+		CONF_END()
+	},
+	[CONFIG_HELP] = {
+		CONF_STR_VAR("format", "man"),
+		CONF_INT_VAR("autocorrect", 0),
+		CONF_END()
+	},
+	[CONFIG_HIST] = {
+		CONF_STR_VAR("percentage", "absolute"),
+		CONF_END()
+	},
+	[CONFIG_UI] = {
+		CONF_BOOL_VAR("show-headers", true),
+		CONF_END()
+	},
+	[CONFIG_CALL_GRAPH] = {
+		CONF_STR_VAR("record-mode", "fp"),
+		CONF_LONG_VAR("dump-size", 8192),
+		CONF_STR_VAR("print-type", "graph"),
+		CONF_STR_VAR("order", "callee"),
+		CONF_STR_VAR("sort-key", "function"),
+		CONF_DOUBLE_VAR("threshold", 0.5),
+		CONF_LONG_VAR("print-limit", 0),
+		CONF_END()
+	},
+	[CONFIG_REPORT] = {
+		CONF_BOOL_VAR("group", true),
+		CONF_BOOL_VAR("children", true),
+		CONF_FLOAT_VAR("percent-limit", 0),
+		CONF_U64_VAR("queue-size", 0),
+		CONF_END()
+	},
+	[CONFIG_TOP] = {
+		CONF_BOOL_VAR("children", true),
+		CONF_END()
+	},
+	[CONFIG_MAN] = {
+		CONF_STR_VAR("viewer", "man"),
+		CONF_END()
+	},
+	[CONFIG_KMEM] = {
+		CONF_STR_VAR("default", "slab"),
+		CONF_END()
+	}
+};
+
 static int get_next_char(void)
 {
 	int c;
@@ -659,7 +765,7 @@ struct perf_config_set *perf_config_set__new(void)
 
 static void perf_config_item__delete(struct perf_config_item *item)
 {
-	zfree(&item->name);
+	zfree((char **)&item->name);
 	zfree(&item->value);
 	free(item);
 }
@@ -677,7 +783,7 @@ static void perf_config_section__purge(struct perf_config_section *section)
 static void perf_config_section__delete(struct perf_config_section *section)
 {
 	perf_config_section__purge(section);
-	zfree(&section->name);
+	zfree((char **)&section->name);
 	free(section);
 }
 
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 22ec626..84dcc1d 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -4,14 +4,34 @@
 #include <stdbool.h>
 #include <linux/list.h>
 
+enum perf_config_type {
+	CONFIG_TYPE_BOOL,
+	CONFIG_TYPE_INT,
+	CONFIG_TYPE_LONG,
+	CONFIG_TYPE_U64,
+	CONFIG_TYPE_FLOAT,
+	CONFIG_TYPE_DOUBLE,
+	CONFIG_TYPE_STRING
+};
+
 struct perf_config_item {
-	char *name;
+	const char *name;
 	char *value;
+	union {
+		bool b;
+		int i;
+		u32 l;
+		u64 ll;
+		float f;
+		double d;
+		const char *s;
+	} default_value;
+	enum perf_config_type type;
 	struct list_head node;
 };
 
 struct perf_config_section {
-	char *name;
+	const char *name;
 	struct list_head items;
 	struct list_head node;
 };
@@ -20,6 +40,44 @@ struct perf_config_set {
 	struct list_head sections;
 };
 
+enum perf_config_secion_idx {
+	CONFIG_COLORS,
+	CONFIG_TUI,
+	CONFIG_BUILDID,
+	CONFIG_ANNOTATE,
+	CONFIG_GTK,
+	CONFIG_PAGER,
+	CONFIG_HELP,
+	CONFIG_HIST,
+	CONFIG_UI,
+	CONFIG_CALL_GRAPH,
+	CONFIG_REPORT,
+	CONFIG_TOP,
+	CONFIG_MAN,
+	CONFIG_KMEM,
+	CONFIG_END
+};
+
+#define CONF_VAR(_name, _field, _val, _type)				\
+	{ .name = _name, .default_value._field = _val, .type = _type }
+
+#define CONF_BOOL_VAR(_name, _val)			\
+	CONF_VAR(_name, b, _val, CONFIG_TYPE_BOOL)
+#define CONF_INT_VAR(_name, _val)			\
+	CONF_VAR(_name, i, _val, CONFIG_TYPE_INT)
+#define CONF_LONG_VAR(_name, _val)			\
+	CONF_VAR(_name, l, _val, CONFIG_TYPE_LONG)
+#define CONF_U64_VAR(_name, _val)			\
+	CONF_VAR(_name, ll, _val, CONFIG_TYPE_U64)
+#define CONF_FLOAT_VAR(_name, _val)			\
+	CONF_VAR(_name, f, _val, CONFIG_TYPE_FLOAT)
+#define CONF_DOUBLE_VAR(_name, _val)			\
+	CONF_VAR(_name, d, _val, CONFIG_TYPE_DOUBLE)
+#define CONF_STR_VAR(_name, _val)			\
+	CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
+#define CONF_END()					\
+	{ .name = NULL }
+
 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] 14+ messages in thread
* [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
  2016-04-04  9:17 [PATCH v6 0/4] Infrastructure code for perf-config Taeung Song
                   ` (2 preceding siblings ...)
  2016-04-04  9:17 ` [PATCH v6 3/4] perf config: Prepare all default configs Taeung Song
@ 2016-04-04  9:17 ` Taeung Song
  2016-04-04 13:28   ` Masami Hiramatsu
  3 siblings, 1 reply; 14+ messages in thread
From: Taeung Song @ 2016-04-04  9:17 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, Jiri Olsa, Namhyung Kim, Ingo Molnar,
	Peter Zijlstra, Masami Hiramatsu, Taeung Song

To avoid duplicated config variables and
use perf_config_set classifying between standard
perf config variables and unknown or new config
variables other than them, initialize perf_config_set
with all default configs.

And this will be needed when showing all configs with
default value or checking correct type of a config
variable in the near future.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-config.c | 11 +++++++----
 tools/perf/util/config.c    | 33 ++++++++++++++++++++++++++++++---
 tools/perf/util/config.h    |  7 +++++++
 3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 1133224..66a269e 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -35,23 +35,26 @@ static struct option config_options[] = {
 
 static int show_config(struct perf_config_set *set)
 {
+	bool has_value = false;
 	struct perf_config_section *section;
 	struct perf_config_item *item;
 	struct list_head *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)
+			if (value) {
 				printf("%s.%s=%s\n", section->name,
 				       item->name, value);
+				has_value = true;
+			}
 		}
 	}
 
+	if (!has_value)
+		return -1;
+
 	return 0;
 }
 
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index f937124..1855560 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
 	if (!section)
 		return NULL;
 
+	section->is_custom = true;
 	INIT_LIST_HEAD(&section->items);
 	section->name = strdup(section_name);
 	if (!section->name) {
@@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
 	if (!item)
 		return NULL;
 
+	item->is_custom = true;
 	item->name = strdup(name);
 	if (!item->name) {
 		pr_debug("%s: strdup failed\n", __func__);
@@ -751,12 +753,35 @@ out_free:
 	return -1;
 }
 
+static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
+{
+	int i, j;
+	struct perf_config_section *section;
+	struct perf_config_item *items;
+	struct list_head *sections = &set->sections;
+
+	INIT_LIST_HEAD(&set->sections);
+
+	for (i = 0; i != CONFIG_END; i++) {
+		section = &default_sections[i];
+		INIT_LIST_HEAD(&section->items);
+
+		items = default_config_items[i];
+		for (j = 0; items[j].name != NULL; j++)
+			list_add_tail(&items[j].node, &section->items);
+
+		list_add_tail(&section->node, sections);
+	}
+
+	return set;
+}
+
 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_set__init(set);
 		perf_config(collect_config, set);
 	}
 
@@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
 
 	list_for_each_entry_safe(item, tmp, &section->items, node) {
 		list_del_init(&item->node);
-		perf_config_item__delete(item);
+		if (item->is_custom)
+			perf_config_item__delete(item);
 	}
 }
 
@@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
 
 	list_for_each_entry_safe(section, tmp, &set->sections, node) {
 		list_del_init(&section->node);
-		perf_config_section__delete(section);
+		if (section->is_custom)
+			perf_config_section__delete(section);
 	}
 }
 
diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
index 84dcc1d..1df96b7 100644
--- a/tools/perf/util/config.h
+++ b/tools/perf/util/config.h
@@ -14,6 +14,11 @@ enum perf_config_type {
 	CONFIG_TYPE_STRING
 };
 
+/**
+ * struct perf_config_item - element of perf's configs
+ *
+ * @is_custom: unknown or new config other than default config
+ */
 struct perf_config_item {
 	const char *name;
 	char *value;
@@ -27,11 +32,13 @@ struct perf_config_item {
 		const char *s;
 	} default_value;
 	enum perf_config_type type;
+	bool is_custom;
 	struct list_head node;
 };
 
 struct perf_config_section {
 	const char *name;
+	bool is_custom;
 	struct list_head items;
 	struct list_head node;
 };
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/4] perf config: Introduce perf_config_set class
  2016-04-04  9:17 ` [PATCH v6 1/4] perf config: Introduce perf_config_set class Taeung Song
@ 2016-04-04 12:41   ` Masami Hiramatsu
  2016-04-04 12:42     ` Taeung Song
  0 siblings, 1 reply; 14+ messages in thread
From: Masami Hiramatsu @ 2016-04-04 12:41 UTC (permalink / raw)
  To: Taeung Song
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra, Masami Hiramatsu

On Mon,  4 Apr 2016 18:17:04 +0900
Taeung Song <treeze.taeung@gmail.com> wrote:

> This infrastructure code was designed for
> upcoming features of perf-config.
> 
> That collect config key-value pairs from user and
> system config files (i.e. user wide ~/.perfconfig
> and system wide $(sysconfdir)/perfconfig)
> to manage perf's configs.
> 
Looks good to me:)

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/config.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/config.h |  26 +++++++
>  2 files changed, 199 insertions(+)
>  create mode 100644 tools/perf/util/config.h
> 
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 5c20d78..53e756e 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -13,6 +13,7 @@
>  #include <subcmd/exec-cmd.h>
>  #include "util/hist.h"  /* perf_hist_config */
>  #include "util/llvm-utils.h"   /* perf_llvm_config */
> +#include "config.h"
>  
>  #define MAXNAME (256)
>  
> @@ -524,6 +525,178 @@ out:
>  	return ret;
>  }
>  
> +static struct perf_config_section *find_section(struct list_head *sections,
> +						const char *section_name)
> +{
> +	struct perf_config_section *section;
> +
> +	list_for_each_entry(section, sections, node)
> +		if (!strcmp(section->name, section_name))
> +			return section;
> +
> +	return NULL;
> +}
> +
> +static struct perf_config_item *find_config_item(const char *name,
> +						 struct perf_config_section *section)
> +{
> +	struct perf_config_item *item;
> +
> +	list_for_each_entry(item, &section->items, node)
> +		if (!strcmp(item->name, name))
> +			return item;
> +
> +	return NULL;
> +}
> +
> +static struct perf_config_section *add_section(struct list_head *sections,
> +					       const char *section_name)
> +{
> +	struct perf_config_section *section = zalloc(sizeof(*section));
> +
> +	if (!section)
> +		return NULL;
> +
> +	INIT_LIST_HEAD(&section->items);
> +	section->name = strdup(section_name);
> +	if (!section->name) {
> +		pr_debug("%s: strdup failed\n", __func__);
> +		free(section);
> +		return NULL;
> +	}
> +
> +	list_add_tail(&section->node, sections);
> +	return section;
> +}
> +
> +static struct perf_config_item *add_config_item(struct perf_config_section *section,
> +						const char *name)
> +{
> +	struct perf_config_item *item = zalloc(sizeof(*item));
> +
> +	if (!item)
> +		return NULL;
> +
> +	item->name = strdup(name);
> +	if (!item->name) {
> +		pr_debug("%s: strdup failed\n", __func__);
> +		free(item);
> +		return NULL;
> +	}
> +
> +	list_add_tail(&item->node, &section->items);
> +	return item;
> +}
> +
> +static int set_value(struct perf_config_item *item, const char *value)
> +{
> +	char *val = strdup(value);
> +
> +	if (!val)
> +		return -1;
> +
> +	free(item->value);
> +	item->value = val;
> +	return 0;
> +}
> +
> +static int collect_config(const char *var, const char *value,
> +			  void *perf_config_set)
> +{
> +	int ret = -1;
> +	char *ptr, *key;
> +	char *section_name, *name;
> +	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;
> +
> +	key = ptr = strdup(var);
> +	if (!key) {
> +		pr_debug("%s: strdup failed\n", __func__);
> +		return -1;
> +	}
> +
> +	section_name = strsep(&ptr, ".");
> +	name = ptr;
> +	if (name == NULL || value == NULL)
> +		goto out_free;
> +
> +	section = find_section(sections, section_name);
> +	if (!section) {
> +		section = add_section(sections, section_name);
> +		if (!section)
> +			goto out_free;
> +	}
> +
> +	item = find_config_item(name, section);
> +	if (!item) {
> +		item = add_config_item(section, name);
> +		if (!item)
> +			goto out_free;
> +	}
> +
> +	ret = set_value(item, value);
> +	return ret;
> +
> +out_free:
> +	free(key);
> +	perf_config_set__delete(set);
> +	return -1;
> +}
> +
> +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);
> +	}
> +
> +	return set;
> +}
> +
> +static void perf_config_item__delete(struct perf_config_item *item)
> +{
> +	zfree(&item->name);
> +	zfree(&item->value);
> +	free(item);
> +}
> +
> +static void perf_config_section__purge(struct perf_config_section *section)
> +{
> +	struct perf_config_item *item, *tmp;
> +
> +	list_for_each_entry_safe(item, tmp, &section->items, node) {
> +		list_del_init(&item->node);
> +		perf_config_item__delete(item);
> +	}
> +}
> +
> +static void perf_config_section__delete(struct perf_config_section *section)
> +{
> +	perf_config_section__purge(section);
> +	zfree(&section->name);
> +	free(section);
> +}
> +
> +static void perf_config_set__purge(struct perf_config_set *set)
> +{
> +	struct perf_config_section *section, *tmp;
> +
> +	list_for_each_entry_safe(section, tmp, &set->sections, node) {
> +		list_del_init(&section->node);
> +		perf_config_section__delete(section);
> +	}
> +}
> +
> +void perf_config_set__delete(struct perf_config_set *set)
> +{
> +	perf_config_set__purge(set);
> +	free(set);
> +}
> +
>  /*
>   * Call this to report error for your variable that should not
>   * get a boolean value (i.e. "[my] var" means "true").
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> new file mode 100644
> index 0000000..22ec626
> --- /dev/null
> +++ b/tools/perf/util/config.h
> @@ -0,0 +1,26 @@
> +#ifndef __PERF_CONFIG_H
> +#define __PERF_CONFIG_H
> +
> +#include <stdbool.h>
> +#include <linux/list.h>
> +
> +struct perf_config_item {
> +	char *name;
> +	char *value;
> +	struct list_head node;
> +};
> +
> +struct perf_config_section {
> +	char *name;
> +	struct list_head items;
> +	struct list_head node;
> +};
> +
> +struct perf_config_set {
> +	struct list_head sections;
> +};
> +
> +struct perf_config_set *perf_config_set__new(void);
> +void perf_config_set__delete(struct perf_config_set *set);
> +
> +#endif /* __PERF_CONFIG_H */
> -- 
> 2.5.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/4] perf config: Introduce perf_config_set class
  2016-04-04 12:41   ` Masami Hiramatsu
@ 2016-04-04 12:42     ` Taeung Song
  0 siblings, 0 replies; 14+ messages in thread
From: Taeung Song @ 2016-04-04 12:42 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra



On 04/04/2016 09:41 PM, Masami Hiramatsu wrote:
> On Mon,  4 Apr 2016 18:17:04 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> This infrastructure code was designed for
>> upcoming features of perf-config.
>>
>> That collect config key-value pairs from user and
>> system config files (i.e. user wide ~/.perfconfig
>> and system wide $(sysconfdir)/perfconfig)
>> to manage perf's configs.
>>
> Looks good to me:)
>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
>
> Thanks!

Thank you !! :-)

Taeung

>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/util/config.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/config.h |  26 +++++++
>>   2 files changed, 199 insertions(+)
>>   create mode 100644 tools/perf/util/config.h
>>
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index 5c20d78..53e756e 100644
>> --- a/tools/perf/util/config.c
>> +++ b/tools/perf/util/config.c
>> @@ -13,6 +13,7 @@
>>   #include <subcmd/exec-cmd.h>
>>   #include "util/hist.h"  /* perf_hist_config */
>>   #include "util/llvm-utils.h"   /* perf_llvm_config */
>> +#include "config.h"
>>
>>   #define MAXNAME (256)
>>
>> @@ -524,6 +525,178 @@ out:
>>   	return ret;
>>   }
>>
>> +static struct perf_config_section *find_section(struct list_head *sections,
>> +						const char *section_name)
>> +{
>> +	struct perf_config_section *section;
>> +
>> +	list_for_each_entry(section, sections, node)
>> +		if (!strcmp(section->name, section_name))
>> +			return section;
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct perf_config_item *find_config_item(const char *name,
>> +						 struct perf_config_section *section)
>> +{
>> +	struct perf_config_item *item;
>> +
>> +	list_for_each_entry(item, &section->items, node)
>> +		if (!strcmp(item->name, name))
>> +			return item;
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct perf_config_section *add_section(struct list_head *sections,
>> +					       const char *section_name)
>> +{
>> +	struct perf_config_section *section = zalloc(sizeof(*section));
>> +
>> +	if (!section)
>> +		return NULL;
>> +
>> +	INIT_LIST_HEAD(&section->items);
>> +	section->name = strdup(section_name);
>> +	if (!section->name) {
>> +		pr_debug("%s: strdup failed\n", __func__);
>> +		free(section);
>> +		return NULL;
>> +	}
>> +
>> +	list_add_tail(&section->node, sections);
>> +	return section;
>> +}
>> +
>> +static struct perf_config_item *add_config_item(struct perf_config_section *section,
>> +						const char *name)
>> +{
>> +	struct perf_config_item *item = zalloc(sizeof(*item));
>> +
>> +	if (!item)
>> +		return NULL;
>> +
>> +	item->name = strdup(name);
>> +	if (!item->name) {
>> +		pr_debug("%s: strdup failed\n", __func__);
>> +		free(item);
>> +		return NULL;
>> +	}
>> +
>> +	list_add_tail(&item->node, &section->items);
>> +	return item;
>> +}
>> +
>> +static int set_value(struct perf_config_item *item, const char *value)
>> +{
>> +	char *val = strdup(value);
>> +
>> +	if (!val)
>> +		return -1;
>> +
>> +	free(item->value);
>> +	item->value = val;
>> +	return 0;
>> +}
>> +
>> +static int collect_config(const char *var, const char *value,
>> +			  void *perf_config_set)
>> +{
>> +	int ret = -1;
>> +	char *ptr, *key;
>> +	char *section_name, *name;
>> +	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;
>> +
>> +	key = ptr = strdup(var);
>> +	if (!key) {
>> +		pr_debug("%s: strdup failed\n", __func__);
>> +		return -1;
>> +	}
>> +
>> +	section_name = strsep(&ptr, ".");
>> +	name = ptr;
>> +	if (name == NULL || value == NULL)
>> +		goto out_free;
>> +
>> +	section = find_section(sections, section_name);
>> +	if (!section) {
>> +		section = add_section(sections, section_name);
>> +		if (!section)
>> +			goto out_free;
>> +	}
>> +
>> +	item = find_config_item(name, section);
>> +	if (!item) {
>> +		item = add_config_item(section, name);
>> +		if (!item)
>> +			goto out_free;
>> +	}
>> +
>> +	ret = set_value(item, value);
>> +	return ret;
>> +
>> +out_free:
>> +	free(key);
>> +	perf_config_set__delete(set);
>> +	return -1;
>> +}
>> +
>> +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);
>> +	}
>> +
>> +	return set;
>> +}
>> +
>> +static void perf_config_item__delete(struct perf_config_item *item)
>> +{
>> +	zfree(&item->name);
>> +	zfree(&item->value);
>> +	free(item);
>> +}
>> +
>> +static void perf_config_section__purge(struct perf_config_section *section)
>> +{
>> +	struct perf_config_item *item, *tmp;
>> +
>> +	list_for_each_entry_safe(item, tmp, &section->items, node) {
>> +		list_del_init(&item->node);
>> +		perf_config_item__delete(item);
>> +	}
>> +}
>> +
>> +static void perf_config_section__delete(struct perf_config_section *section)
>> +{
>> +	perf_config_section__purge(section);
>> +	zfree(&section->name);
>> +	free(section);
>> +}
>> +
>> +static void perf_config_set__purge(struct perf_config_set *set)
>> +{
>> +	struct perf_config_section *section, *tmp;
>> +
>> +	list_for_each_entry_safe(section, tmp, &set->sections, node) {
>> +		list_del_init(&section->node);
>> +		perf_config_section__delete(section);
>> +	}
>> +}
>> +
>> +void perf_config_set__delete(struct perf_config_set *set)
>> +{
>> +	perf_config_set__purge(set);
>> +	free(set);
>> +}
>> +
>>   /*
>>    * Call this to report error for your variable that should not
>>    * get a boolean value (i.e. "[my] var" means "true").
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> new file mode 100644
>> index 0000000..22ec626
>> --- /dev/null
>> +++ b/tools/perf/util/config.h
>> @@ -0,0 +1,26 @@
>> +#ifndef __PERF_CONFIG_H
>> +#define __PERF_CONFIG_H
>> +
>> +#include <stdbool.h>
>> +#include <linux/list.h>
>> +
>> +struct perf_config_item {
>> +	char *name;
>> +	char *value;
>> +	struct list_head node;
>> +};
>> +
>> +struct perf_config_section {
>> +	char *name;
>> +	struct list_head items;
>> +	struct list_head node;
>> +};
>> +
>> +struct perf_config_set {
>> +	struct list_head sections;
>> +};
>> +
>> +struct perf_config_set *perf_config_set__new(void);
>> +void perf_config_set__delete(struct perf_config_set *set);
>> +
>> +#endif /* __PERF_CONFIG_H */
>> --
>> 2.5.0
>>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set
  2016-04-04  9:17 ` [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set Taeung Song
@ 2016-04-04 12:49   ` Masami Hiramatsu
  2016-04-05  1:54     ` Taeung Song
  0 siblings, 1 reply; 14+ messages in thread
From: Masami Hiramatsu @ 2016-04-04 12:49 UTC (permalink / raw)
  To: Taeung Song
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra, Masami Hiramatsu

On Mon,  4 Apr 2016 18:17:05 +0900
Taeung Song <treeze.taeung@gmail.com> wrote:

> Current show_config() has a problem when user or
> system config files have same config variables i.e.
> 
>     # cat ~/.perfconfig
>         [top]
>             children = false
> 
>     when $(sysconfdir) is /usr/local/etc
>     # cat /usr/local/etc/perfconfig
>         [top]
>             children = true
> 
> Before:
>     # perf config --user --list
>     top.children=false
> 
>     # perf config --system --list
>     top.children=true
> 
>     # perf config --list
>     top.children=true
>     top.children=false
> 
> Because perf_config() can call show_config()
> each the config file (user and system).
> So fix it.
> 
> After:
>     # perf config --user --list
>     top.children=false
> 
>     # perf config --system --list
>     top.children=true
> 
>     # perf config --list
>     top.children=false
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-config.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index c42448e..1133224 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -12,6 +12,7 @@
>  #include <subcmd/parse-options.h>
>  #include "util/util.h"
>  #include "util/debug.h"
> +#include "util/config.h"
>  
>  static bool use_system_config, use_user_config;
>  
> @@ -32,13 +33,24 @@ static struct option config_options[] = {
>  	OPT_END()
>  };
>  
> -static int show_config(const char *key, const char *value,
> -		       void *cb __maybe_unused)
> +static int show_config(struct perf_config_set *set)
>  {
> -	if (value)
> -		printf("%s=%s\n", key, value);
> -	else
> -		printf("%s\n", key);
> +	struct perf_config_section *section;
> +	struct perf_config_item *item;
> +	struct list_head *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);
> +		}
> +	}
>  
>  	return 0;
>  }
> @@ -46,6 +58,7 @@ static int show_config(const char *key, const char *value,
>  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,
> @@ -63,13 +76,19 @@ 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) {
> +		ret = -1;
> +		goto out_err;
> +	}
> +
>  	switch (actions) {
>  	case ACTION_LIST:
>  		if (argc) {
>  			pr_err("Error: takes no arguments\n");
>  			parse_options_usage(config_usage, config_options, "l", 1);
>  		} else {
> -			ret = perf_config(show_config, NULL);
> +			ret = show_config(set);
>  			if (ret < 0) {
>  				const char * config_filename = config_exclusive_filename;
>  				if (!config_exclusive_filename)
> @@ -83,5 +102,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);
> +out_err:
>  	return ret;
>  }
> -- 
> 2.5.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
  2016-04-04  9:17 ` [PATCH v6 4/4] perf config: Initialize perf_config_set with " Taeung Song
@ 2016-04-04 13:28   ` Masami Hiramatsu
  2016-04-05  6:06     ` Taeung Song
  0 siblings, 1 reply; 14+ messages in thread
From: Masami Hiramatsu @ 2016-04-04 13:28 UTC (permalink / raw)
  To: Taeung Song
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra, Masami Hiramatsu

On Mon,  4 Apr 2016 18:17:07 +0900
Taeung Song <treeze.taeung@gmail.com> wrote:

> To avoid duplicated config variables and
> use perf_config_set classifying between standard
> perf config variables and unknown or new config
> variables other than them, initialize perf_config_set
> with all default configs.
> 
> And this will be needed when showing all configs with
> default value or checking correct type of a config
> variable in the near future.
> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-config.c | 11 +++++++----
>  tools/perf/util/config.c    | 33 ++++++++++++++++++++++++++++++---
>  tools/perf/util/config.h    |  7 +++++++
>  3 files changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index 1133224..66a269e 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -35,23 +35,26 @@ static struct option config_options[] = {
>  
>  static int show_config(struct perf_config_set *set)
>  {
> +	bool has_value = false;
>  	struct perf_config_section *section;
>  	struct perf_config_item *item;
>  	struct list_head *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)
> +			if (value) {
>  				printf("%s.%s=%s\n", section->name,
>  				       item->name, value);
> +				has_value = true;
> +			}
>  		}
>  	}
>  
> +	if (!has_value)
> +		return -1;

Could this path be really executed? It seems that the set is
always initialized with default values. This means not empty.
If it is for asserting bugs, we also have to check set != NULL
beforehand. 

> +
>  	return 0;
>  }
>  
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index f937124..1855560 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
>  	if (!section)
>  		return NULL;
>  
> +	section->is_custom = true;
>  	INIT_LIST_HEAD(&section->items);
>  	section->name = strdup(section_name);
>  	if (!section->name) {
> @@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
>  	if (!item)
>  		return NULL;
>  
> +	item->is_custom = true;
>  	item->name = strdup(name);
>  	if (!item->name) {
>  		pr_debug("%s: strdup failed\n", __func__);
> @@ -751,12 +753,35 @@ out_free:
>  	return -1;
>  }
>  
> +static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
> +{
> +	int i, j;
> +	struct perf_config_section *section;
> +	struct perf_config_item *items;
> +	struct list_head *sections = &set->sections;
> +
> +	INIT_LIST_HEAD(&set->sections);
> +
> +	for (i = 0; i != CONFIG_END; i++) {
> +		section = &default_sections[i];
> +		INIT_LIST_HEAD(&section->items);
> +
> +		items = default_config_items[i];
> +		for (j = 0; items[j].name != NULL; j++)
> +			list_add_tail(&items[j].node, &section->items);
> +
> +		list_add_tail(&section->node, sections);
> +	}
> +
> +	return set;
> +}
> +
>  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_set__init(set);
>  		perf_config(collect_config, set);
>  	}
>  
> @@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
>  
>  	list_for_each_entry_safe(item, tmp, &section->items, node) {
>  		list_del_init(&item->node);
> -		perf_config_item__delete(item);
> +		if (item->is_custom)
> +			perf_config_item__delete(item);
>  	}
>  }
>  
> @@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
>  
>  	list_for_each_entry_safe(section, tmp, &set->sections, node) {
>  		list_del_init(&section->node);
> -		perf_config_section__delete(section);
> +		if (section->is_custom)
> +			perf_config_section__delete(section);

Here, unless perf_config_section__delete() is called, the items
under the section is never be checked. However, if a user changes
existing item value, its section->is_custom is false, but the
item itself is_custom be true.
This means that such custom item values may not be freed.
So, the section->is_custom flag should be checked in *__delete
method as below,

static void perf_config_section__delete(struct perf_config_section *section)
{
	perf_config_section__purge(section);
	if (section->is_custom) {
		zfree(&section->name);
		free(section);
	}
}

Or, you can just call perf_config_section__purge(section) in
perf_config_set__purge().

Also, I like ->allocated instead of ->is_custom.

Thank you,

>  	}
>  }
>  
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index 84dcc1d..1df96b7 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -14,6 +14,11 @@ enum perf_config_type {
>  	CONFIG_TYPE_STRING
>  };
>  
> +/**
> + * struct perf_config_item - element of perf's configs
> + *
> + * @is_custom: unknown or new config other than default config
> + */
>  struct perf_config_item {
>  	const char *name;
>  	char *value;
> @@ -27,11 +32,13 @@ struct perf_config_item {
>  		const char *s;
>  	} default_value;
>  	enum perf_config_type type;
> +	bool is_custom;
>  	struct list_head node;
>  };
>  
>  struct perf_config_section {
>  	const char *name;
> +	bool is_custom;
>  	struct list_head items;
>  	struct list_head node;
>  };
> -- 
> 2.5.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set
  2016-04-04 12:49   ` Masami Hiramatsu
@ 2016-04-05  1:54     ` Taeung Song
  0 siblings, 0 replies; 14+ messages in thread
From: Taeung Song @ 2016-04-05  1:54 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra



On 04/04/2016 09:49 PM, Masami Hiramatsu wrote:
> On Mon,  4 Apr 2016 18:17:05 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> Current show_config() has a problem when user or
>> system config files have same config variables i.e.
>>
>>      # cat ~/.perfconfig
>>          [top]
>>              children = false
>>
>>      when $(sysconfdir) is /usr/local/etc
>>      # cat /usr/local/etc/perfconfig
>>          [top]
>>              children = true
>>
>> Before:
>>      # perf config --user --list
>>      top.children=false
>>
>>      # perf config --system --list
>>      top.children=true
>>
>>      # perf config --list
>>      top.children=true
>>      top.children=false
>>
>> Because perf_config() can call show_config()
>> each the config file (user and system).
>> So fix it.
>>
>> After:
>>      # perf config --user --list
>>      top.children=false
>>
>>      # perf config --system --list
>>      top.children=true
>>
>>      # perf config --list
>>      top.children=false
>>
>
> Looks good to me.
>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
>

Thank you !! :-)

Taeung

>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/builtin-config.c | 35 ++++++++++++++++++++++++++++-------
>>   1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index c42448e..1133224 100644
>> --- a/tools/perf/builtin-config.c
>> +++ b/tools/perf/builtin-config.c
>> @@ -12,6 +12,7 @@
>>   #include <subcmd/parse-options.h>
>>   #include "util/util.h"
>>   #include "util/debug.h"
>> +#include "util/config.h"
>>
>>   static bool use_system_config, use_user_config;
>>
>> @@ -32,13 +33,24 @@ static struct option config_options[] = {
>>   	OPT_END()
>>   };
>>
>> -static int show_config(const char *key, const char *value,
>> -		       void *cb __maybe_unused)
>> +static int show_config(struct perf_config_set *set)
>>   {
>> -	if (value)
>> -		printf("%s=%s\n", key, value);
>> -	else
>> -		printf("%s\n", key);
>> +	struct perf_config_section *section;
>> +	struct perf_config_item *item;
>> +	struct list_head *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);
>> +		}
>> +	}
>>
>>   	return 0;
>>   }
>> @@ -46,6 +58,7 @@ static int show_config(const char *key, const char *value,
>>   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,
>> @@ -63,13 +76,19 @@ 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) {
>> +		ret = -1;
>> +		goto out_err;
>> +	}
>> +
>>   	switch (actions) {
>>   	case ACTION_LIST:
>>   		if (argc) {
>>   			pr_err("Error: takes no arguments\n");
>>   			parse_options_usage(config_usage, config_options, "l", 1);
>>   		} else {
>> -			ret = perf_config(show_config, NULL);
>> +			ret = show_config(set);
>>   			if (ret < 0) {
>>   				const char * config_filename = config_exclusive_filename;
>>   				if (!config_exclusive_filename)
>> @@ -83,5 +102,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);
>> +out_err:
>>   	return ret;
>>   }
>> --
>> 2.5.0
>>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
  2016-04-04 13:28   ` Masami Hiramatsu
@ 2016-04-05  6:06     ` Taeung Song
  2016-04-05 11:23       ` Masami Hiramatsu
  0 siblings, 1 reply; 14+ messages in thread
From: Taeung Song @ 2016-04-05  6:06 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra

Hi, Masami

On 04/04/2016 10:28 PM, Masami Hiramatsu wrote:
> On Mon,  4 Apr 2016 18:17:07 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> To avoid duplicated config variables and
>> use perf_config_set classifying between standard
>> perf config variables and unknown or new config
>> variables other than them, initialize perf_config_set
>> with all default configs.
>>
>> And this will be needed when showing all configs with
>> default value or checking correct type of a config
>> variable in the near future.
>>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/builtin-config.c | 11 +++++++----
>>   tools/perf/util/config.c    | 33 ++++++++++++++++++++++++++++++---
>>   tools/perf/util/config.h    |  7 +++++++
>>   3 files changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index 1133224..66a269e 100644
>> --- a/tools/perf/builtin-config.c
>> +++ b/tools/perf/builtin-config.c
>> @@ -35,23 +35,26 @@ static struct option config_options[] = {
>>
>>   static int show_config(struct perf_config_set *set)
>>   {
>> +	bool has_value = false;
>>   	struct perf_config_section *section;
>>   	struct perf_config_item *item;
>>   	struct list_head *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)
>> +			if (value) {
>>   				printf("%s.%s=%s\n", section->name,
>>   				       item->name, value);
>> +				has_value = true;
>> +			}
>>   		}
>>   	}
>>
>> +	if (!has_value)
>> +		return -1;
>
> Could this path be really executed? It seems that the set is
> always initialized with default values. This means not empty.
> If it is for asserting bugs, we also have to check set != NULL
> beforehand.

I got it.
But before calling show_config(set), the set is checked as below

... (omitted) ...
	set = perf_config_set__new();
	if (!set) {
		ret = -1;
		goto out_err;
	}

	switch (actions) {
	case ACTION_LIST:
		if (argc) {
			pr_err("Error: takes no arguments\n");
			parse_options_usage(config_usage, config_options, "l", 1);
		} else {
			ret = show_config(set);

...(omitted)...

I thought that the set can never be NULL in show_config().
Is it wrong ?
(I'm afraid that there are things I misunderstood.)

>> +
>>   	return 0;
>>   }
>>
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index f937124..1855560 100644
>> --- a/tools/perf/util/config.c
>> +++ b/tools/perf/util/config.c
>> @@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
>>   	if (!section)
>>   		return NULL;
>>
>> +	section->is_custom = true;
>>   	INIT_LIST_HEAD(&section->items);
>>   	section->name = strdup(section_name);
>>   	if (!section->name) {
>> @@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
>>   	if (!item)
>>   		return NULL;
>>
>> +	item->is_custom = true;
>>   	item->name = strdup(name);
>>   	if (!item->name) {
>>   		pr_debug("%s: strdup failed\n", __func__);
>> @@ -751,12 +753,35 @@ out_free:
>>   	return -1;
>>   }
>>
>> +static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
>> +{
>> +	int i, j;
>> +	struct perf_config_section *section;
>> +	struct perf_config_item *items;
>> +	struct list_head *sections = &set->sections;
>> +
>> +	INIT_LIST_HEAD(&set->sections);
>> +
>> +	for (i = 0; i != CONFIG_END; i++) {
>> +		section = &default_sections[i];
>> +		INIT_LIST_HEAD(&section->items);
>> +
>> +		items = default_config_items[i];
>> +		for (j = 0; items[j].name != NULL; j++)
>> +			list_add_tail(&items[j].node, &section->items);
>> +
>> +		list_add_tail(&section->node, sections);
>> +	}
>> +
>> +	return set;
>> +}
>> +
>>   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_set__init(set);
>>   		perf_config(collect_config, set);
>>   	}
>>
>> @@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
>>
>>   	list_for_each_entry_safe(item, tmp, &section->items, node) {
>>   		list_del_init(&item->node);
>> -		perf_config_item__delete(item);
>> +		if (item->is_custom)
>> +			perf_config_item__delete(item);
>>   	}
>>   }
>>
>> @@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
>>
>>   	list_for_each_entry_safe(section, tmp, &set->sections, node) {
>>   		list_del_init(&section->node);
>> -		perf_config_section__delete(section);
>> +		if (section->is_custom)
>> +			perf_config_section__delete(section);
>
> Here, unless perf_config_section__delete() is called, the items
> under the section is never be checked.

It is my stupid mistake..

> However, if a user changes
> existing item value, its section->is_custom is false, but the
> item itself is_custom be true.
> This means that such custom item values may not be freed.

And there is one more what I missed.
I didn't consider that when existing item's 'value' is set by set_value(),
it can not be freed. So fix it as below.

  static void perf_config_item__delete(struct perf_config_item *item)
  {
-        zfree((char **)&item->name);
          zfree(&item->value);
-        free(item);
+        if (item->is_allocated) {
+                zfree((char **)&item->name);
+                free(item);
+        }
  }

  static void perf_config_section__purge(struct perf_config_section 
*section)
  {
          struct perf_config_item *item, *tmp;

          list_for_each_entry_safe(item, tmp, &section->items, node) {
                  list_del_init(&item->node);
-                if (item->is_custom)
-                        perf_config_item__delete(item);
+                perf_config_item__delete(item);
          }
  }


> So, the section->is_custom flag should be checked in *__delete
> method as below,
> static void perf_config_section__delete(struct perf_config_section *section)
> {
> 	perf_config_section__purge(section);
> 	if (section->is_custom) {
> 		zfree(&section->name);
> 		free(section);
> 	}
> }
>
> Or, you can just call perf_config_section__purge(section) in
> perf_config_set__purge().
>

I'll modify this code by the former that you said like below.


  static void perf_config_section__delete(struct perf_config_section 
*section)
  {
          perf_config_section__purge(section);
-        zfree((char **)&section->name);
-        free(section);
+        if (section->is_allocated) {
+                zfree((char **)&section->name);
+                free(section);
+        }
  }

  static void perf_config_set__purge(struct perf_config_set *set)
@@ -819,8 +822,7 @@ static void perf_config_set__purge(struct 
perf_config_set *set)

          list_for_each_entry_safe(section, tmp, &set->sections, node) {
                  list_del_init(&section->node);
-                if (section->is_custom)
-                        perf_config_section__delete(section);
+                perf_config_section__delete(section);
          }
  }



> Also, I like ->allocated instead of ->is_custom.
>

What you said is right.
Because the variable means whether a config section or item is allocated,
not customized by a user.
I'll rename it.


Thanks,
Taeung

>
>>   	}
>>   }
>>
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index 84dcc1d..1df96b7 100644
>> --- a/tools/perf/util/config.h
>> +++ b/tools/perf/util/config.h
>> @@ -14,6 +14,11 @@ enum perf_config_type {
>>   	CONFIG_TYPE_STRING
>>   };
>>
>> +/**
>> + * struct perf_config_item - element of perf's configs
>> + *
>> + * @is_custom: unknown or new config other than default config
>> + */
>>   struct perf_config_item {
>>   	const char *name;
>>   	char *value;
>> @@ -27,11 +32,13 @@ struct perf_config_item {
>>   		const char *s;
>>   	} default_value;
>>   	enum perf_config_type type;
>> +	bool is_custom;
>>   	struct list_head node;
>>   };
>>
>>   struct perf_config_section {
>>   	const char *name;
>> +	bool is_custom;
>>   	struct list_head items;
>>   	struct list_head node;
>>   };
>> --
>> 2.5.0
>>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
  2016-04-05  6:06     ` Taeung Song
@ 2016-04-05 11:23       ` Masami Hiramatsu
  2016-04-05 11:47         ` Taeung Song
  2016-04-07 10:35         ` Taeung Song
  0 siblings, 2 replies; 14+ messages in thread
From: Masami Hiramatsu @ 2016-04-05 11:23 UTC (permalink / raw)
  To: Taeung Song
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra

On Tue, 5 Apr 2016 15:06:47 +0900
Taeung Song <treeze.taeung@gmail.com> wrote:

> Hi, Masami
> 
> On 04/04/2016 10:28 PM, Masami Hiramatsu wrote:
> > On Mon,  4 Apr 2016 18:17:07 +0900
> > Taeung Song <treeze.taeung@gmail.com> wrote:
> >
> >> To avoid duplicated config variables and
> >> use perf_config_set classifying between standard
> >> perf config variables and unknown or new config
> >> variables other than them, initialize perf_config_set
> >> with all default configs.
> >>
> >> And this will be needed when showing all configs with
> >> default value or checking correct type of a config
> >> variable in the near future.
> >>
> >> Cc: Namhyung Kim <namhyung@kernel.org>
> >> Cc: Jiri Olsa <jolsa@kernel.org>
> >> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> >> ---
> >>   tools/perf/builtin-config.c | 11 +++++++----
> >>   tools/perf/util/config.c    | 33 ++++++++++++++++++++++++++++++---
> >>   tools/perf/util/config.h    |  7 +++++++
> >>   3 files changed, 44 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> >> index 1133224..66a269e 100644
> >> --- a/tools/perf/builtin-config.c
> >> +++ b/tools/perf/builtin-config.c
> >> @@ -35,23 +35,26 @@ static struct option config_options[] = {
> >>
> >>   static int show_config(struct perf_config_set *set)
> >>   {
> >> +	bool has_value = false;
> >>   	struct perf_config_section *section;
> >>   	struct perf_config_item *item;
> >>   	struct list_head *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)
> >> +			if (value) {
> >>   				printf("%s.%s=%s\n", section->name,
> >>   				       item->name, value);
> >> +				has_value = true;
> >> +			}
> >>   		}
> >>   	}
> >>
> >> +	if (!has_value)
> >> +		return -1;
> >
> > Could this path be really executed? It seems that the set is
> > always initialized with default values. This means not empty.
> > If it is for asserting bugs, we also have to check set != NULL
> > beforehand.
> 
> I got it.
> But before calling show_config(set), the set is checked as below
> 
> ... (omitted) ...
> 	set = perf_config_set__new();
> 	if (!set) {
> 		ret = -1;
> 		goto out_err;
> 	}
> 
> 	switch (actions) {
> 	case ACTION_LIST:
> 		if (argc) {
> 			pr_err("Error: takes no arguments\n");
> 			parse_options_usage(config_usage, config_options, "l", 1);
> 		} else {
> 			ret = show_config(set);
> 
> ...(omitted)...
> 
> I thought that the set can never be NULL in show_config().
> Is it wrong ?
> (I'm afraid that there are things I misunderstood.)

Hmm, my main point was has_value is always true because
perf_config_set__init() initializes the set with default value.
If so, has_value can be removed. But even though, if has_value
is intended to detect unexpected bugs, also introducing (set != NULL)
check is more natural.

> >> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> >> index f937124..1855560 100644
> >> --- a/tools/perf/util/config.c
> >> +++ b/tools/perf/util/config.c
> >> @@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
> >>   	if (!section)
> >>   		return NULL;
> >>
> >> +	section->is_custom = true;
> >>   	INIT_LIST_HEAD(&section->items);
> >>   	section->name = strdup(section_name);
> >>   	if (!section->name) {
> >> @@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
> >>   	if (!item)
> >>   		return NULL;
> >>
> >> +	item->is_custom = true;
> >>   	item->name = strdup(name);
> >>   	if (!item->name) {
> >>   		pr_debug("%s: strdup failed\n", __func__);
> >> @@ -751,12 +753,35 @@ out_free:
> >>   	return -1;
> >>   }
> >>
> >> +static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
> >> +{
> >> +	int i, j;
> >> +	struct perf_config_section *section;
> >> +	struct perf_config_item *items;
> >> +	struct list_head *sections = &set->sections;
> >> +
> >> +	INIT_LIST_HEAD(&set->sections);
> >> +
> >> +	for (i = 0; i != CONFIG_END; i++) {
> >> +		section = &default_sections[i];
> >> +		INIT_LIST_HEAD(&section->items);
> >> +
> >> +		items = default_config_items[i];
> >> +		for (j = 0; items[j].name != NULL; j++)
> >> +			list_add_tail(&items[j].node, &section->items);
> >> +
> >> +		list_add_tail(&section->node, sections);
> >> +	}
> >> +
> >> +	return set;
> >> +}
> >> +
> >>   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_set__init(set);
> >>   		perf_config(collect_config, set);
> >>   	}
> >>
> >> @@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
> >>
> >>   	list_for_each_entry_safe(item, tmp, &section->items, node) {
> >>   		list_del_init(&item->node);
> >> -		perf_config_item__delete(item);
> >> +		if (item->is_custom)
> >> +			perf_config_item__delete(item);
> >>   	}
> >>   }
> >>
> >> @@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
> >>
> >>   	list_for_each_entry_safe(section, tmp, &set->sections, node) {
> >>   		list_del_init(&section->node);
> >> -		perf_config_section__delete(section);
> >> +		if (section->is_custom)
> >> +			perf_config_section__delete(section);
> >
> > Here, unless perf_config_section__delete() is called, the items
> > under the section is never be checked.
> 
> It is my stupid mistake..
> 
> > However, if a user changes
> > existing item value, its section->is_custom is false, but the
> > item itself is_custom be true.
> > This means that such custom item values may not be freed.
> 
> And there is one more what I missed.
> I didn't consider that when existing item's 'value' is set by set_value(),
> it can not be freed. So fix it as below.
> 
>   static void perf_config_item__delete(struct perf_config_item *item)
>   {
> -        zfree((char **)&item->name);
>           zfree(&item->value);
> -        free(item);
> +        if (item->is_allocated) {
> +                zfree((char **)&item->name);
> +                free(item);
> +        }
>   }
> 
>   static void perf_config_section__purge(struct perf_config_section 
> *section)
>   {
>           struct perf_config_item *item, *tmp;
> 
>           list_for_each_entry_safe(item, tmp, &section->items, node) {
>                   list_del_init(&item->node);
> -                if (item->is_custom)
> -                        perf_config_item__delete(item);
> +                perf_config_item__delete(item);
>           }
>   }

Good catch! it also should be done :)

> 
> 
> > So, the section->is_custom flag should be checked in *__delete
> > method as below,
> > static void perf_config_section__delete(struct perf_config_section *section)
> > {
> > 	perf_config_section__purge(section);
> > 	if (section->is_custom) {
> > 		zfree(&section->name);
> > 		free(section);
> > 	}
> > }
> >
> > Or, you can just call perf_config_section__purge(section) in
> > perf_config_set__purge().
> >
> 
> I'll modify this code by the former that you said like below.
> 
> 
>   static void perf_config_section__delete(struct perf_config_section 
> *section)
>   {
>           perf_config_section__purge(section);
> -        zfree((char **)&section->name);
> -        free(section);
> +        if (section->is_allocated) {
> +                zfree((char **)&section->name);
> +                free(section);
> +        }
>   }

Looks good :)

> 
>   static void perf_config_set__purge(struct perf_config_set *set)
> @@ -819,8 +822,7 @@ static void perf_config_set__purge(struct 
> perf_config_set *set)
> 
>           list_for_each_entry_safe(section, tmp, &set->sections, node) {
>                   list_del_init(&section->node);
> -                if (section->is_custom)
> -                        perf_config_section__delete(section);
> +                perf_config_section__delete(section);
>           }
>   }
> 
> 
> 
> > Also, I like ->allocated instead of ->is_custom.
> >
> 
> What you said is right.
> Because the variable means whether a config section or item is allocated,
> not customized by a user.
> I'll rename it.

Thank you!

> 
> 
> Thanks,
> Taeung
> 
> >
> >>   	}
> >>   }
> >>
> >> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> >> index 84dcc1d..1df96b7 100644
> >> --- a/tools/perf/util/config.h
> >> +++ b/tools/perf/util/config.h
> >> @@ -14,6 +14,11 @@ enum perf_config_type {
> >>   	CONFIG_TYPE_STRING
> >>   };
> >>
> >> +/**
> >> + * struct perf_config_item - element of perf's configs
> >> + *
> >> + * @is_custom: unknown or new config other than default config
> >> + */
> >>   struct perf_config_item {
> >>   	const char *name;
> >>   	char *value;
> >> @@ -27,11 +32,13 @@ struct perf_config_item {
> >>   		const char *s;
> >>   	} default_value;
> >>   	enum perf_config_type type;
> >> +	bool is_custom;
> >>   	struct list_head node;
> >>   };
> >>
> >>   struct perf_config_section {
> >>   	const char *name;
> >> +	bool is_custom;
> >>   	struct list_head items;
> >>   	struct list_head node;
> >>   };
> >> --
> >> 2.5.0
> >>
> >
> >


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
  2016-04-05 11:23       ` Masami Hiramatsu
@ 2016-04-05 11:47         ` Taeung Song
  2016-04-07 10:35         ` Taeung Song
  1 sibling, 0 replies; 14+ messages in thread
From: Taeung Song @ 2016-04-05 11:47 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra



On 04/05/2016 08:23 PM, Masami Hiramatsu wrote:
> On Tue, 5 Apr 2016 15:06:47 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> Hi, Masami
>>
>> On 04/04/2016 10:28 PM, Masami Hiramatsu wrote:
>>> On Mon,  4 Apr 2016 18:17:07 +0900
>>> Taeung Song <treeze.taeung@gmail.com> wrote:
>>>
>>>> To avoid duplicated config variables and
>>>> use perf_config_set classifying between standard
>>>> perf config variables and unknown or new config
>>>> variables other than them, initialize perf_config_set
>>>> with all default configs.
>>>>
>>>> And this will be needed when showing all configs with
>>>> default value or checking correct type of a config
>>>> variable in the near future.
>>>>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>>> ---
>>>>    tools/perf/builtin-config.c | 11 +++++++----
>>>>    tools/perf/util/config.c    | 33 ++++++++++++++++++++++++++++++---
>>>>    tools/perf/util/config.h    |  7 +++++++
>>>>    3 files changed, 44 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>>>> index 1133224..66a269e 100644
>>>> --- a/tools/perf/builtin-config.c
>>>> +++ b/tools/perf/builtin-config.c
>>>> @@ -35,23 +35,26 @@ static struct option config_options[] = {
>>>>
>>>>    static int show_config(struct perf_config_set *set)
>>>>    {
>>>> +	bool has_value = false;
>>>>    	struct perf_config_section *section;
>>>>    	struct perf_config_item *item;
>>>>    	struct list_head *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)
>>>> +			if (value) {
>>>>    				printf("%s.%s=%s\n", section->name,
>>>>    				       item->name, value);
>>>> +				has_value = true;
>>>> +			}
>>>>    		}
>>>>    	}
>>>>
>>>> +	if (!has_value)
>>>> +		return -1;
>>>
>>> Could this path be really executed? It seems that the set is
>>> always initialized with default values. This means not empty.
>>> If it is for asserting bugs, we also have to check set != NULL
>>> beforehand.
>>
>> I got it.
>> But before calling show_config(set), the set is checked as below
>>
>> ... (omitted) ...
>> 	set = perf_config_set__new();
>> 	if (!set) {
>> 		ret = -1;
>> 		goto out_err;
>> 	}
>>
>> 	switch (actions) {
>> 	case ACTION_LIST:
>> 		if (argc) {
>> 			pr_err("Error: takes no arguments\n");
>> 			parse_options_usage(config_usage, config_options, "l", 1);
>> 		} else {
>> 			ret = show_config(set);
>>
>> ...(omitted)...
>>
>> I thought that the set can never be NULL in show_config().
>> Is it wrong ?
>> (I'm afraid that there are things I misunderstood.)
>
> Hmm, my main point was has_value is always true because
> perf_config_set__init() initializes the set with default value.
> If so, has_value can be removed.

'value' isn't used in perf_config_set__init() as below.

     1) util/config.c

...(omitted)...
struct perf_config_item default_config_items[][MAX_CONFIGS] = {
	[CONFIG_COLORS] = {
		CONF_STR_VAR("top", "red, default"),
...(omitted)...

     2) util/config.h

...(omitted)...
#define CONF_VAR(_name, _field, _val, _type)				\
	{ .name = _name, .default_value._field = _val, .type = _type }

...(omitted)...
#define CONF_STR_VAR(_name, _val)			\
	CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING)
...(omitted)...


config_item has both 'default_value' and 'value'
because of usability for upcoming functionality e.g (setting, getting,
--list-all, --remove, etc).

New config item or modified existing config item only use 'value'.
IMHO, I think has_value haven't to be removed.

 > But even though, if has_value
> is intended to detect unexpected bugs, also introducing (set != NULL)
> check is more natural.

I got it.
I'll check set!=NULL at the very beginning in show_config().

Thanks,
Taeung


>
>>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>>>> index f937124..1855560 100644
>>>> --- a/tools/perf/util/config.c
>>>> +++ b/tools/perf/util/config.c
>>>> @@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
>>>>    	if (!section)
>>>>    		return NULL;
>>>>
>>>> +	section->is_custom = true;
>>>>    	INIT_LIST_HEAD(&section->items);
>>>>    	section->name = strdup(section_name);
>>>>    	if (!section->name) {
>>>> @@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
>>>>    	if (!item)
>>>>    		return NULL;
>>>>
>>>> +	item->is_custom = true;
>>>>    	item->name = strdup(name);
>>>>    	if (!item->name) {
>>>>    		pr_debug("%s: strdup failed\n", __func__);
>>>> @@ -751,12 +753,35 @@ out_free:
>>>>    	return -1;
>>>>    }
>>>>
>>>> +static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
>>>> +{
>>>> +	int i, j;
>>>> +	struct perf_config_section *section;
>>>> +	struct perf_config_item *items;
>>>> +	struct list_head *sections = &set->sections;
>>>> +
>>>> +	INIT_LIST_HEAD(&set->sections);
>>>> +
>>>> +	for (i = 0; i != CONFIG_END; i++) {
>>>> +		section = &default_sections[i];
>>>> +		INIT_LIST_HEAD(&section->items);
>>>> +
>>>> +		items = default_config_items[i];
>>>> +		for (j = 0; items[j].name != NULL; j++)
>>>> +			list_add_tail(&items[j].node, &section->items);
>>>> +
>>>> +		list_add_tail(&section->node, sections);
>>>> +	}
>>>> +
>>>> +	return set;
>>>> +}
>>>> +
>>>>    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_set__init(set);
>>>>    		perf_config(collect_config, set);
>>>>    	}
>>>>
>>>> @@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
>>>>
>>>>    	list_for_each_entry_safe(item, tmp, &section->items, node) {
>>>>    		list_del_init(&item->node);
>>>> -		perf_config_item__delete(item);
>>>> +		if (item->is_custom)
>>>> +			perf_config_item__delete(item);
>>>>    	}
>>>>    }
>>>>
>>>> @@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
>>>>
>>>>    	list_for_each_entry_safe(section, tmp, &set->sections, node) {
>>>>    		list_del_init(&section->node);
>>>> -		perf_config_section__delete(section);
>>>> +		if (section->is_custom)
>>>> +			perf_config_section__delete(section);
>>>
>>> Here, unless perf_config_section__delete() is called, the items
>>> under the section is never be checked.
>>
>> It is my stupid mistake..
>>
>>> However, if a user changes
>>> existing item value, its section->is_custom is false, but the
>>> item itself is_custom be true.
>>> This means that such custom item values may not be freed.
>>
>> And there is one more what I missed.
>> I didn't consider that when existing item's 'value' is set by set_value(),
>> it can not be freed. So fix it as below.
>>
>>    static void perf_config_item__delete(struct perf_config_item *item)
>>    {
>> -        zfree((char **)&item->name);
>>            zfree(&item->value);
>> -        free(item);
>> +        if (item->is_allocated) {
>> +                zfree((char **)&item->name);
>> +                free(item);
>> +        }
>>    }
>>
>>    static void perf_config_section__purge(struct perf_config_section
>> *section)
>>    {
>>            struct perf_config_item *item, *tmp;
>>
>>            list_for_each_entry_safe(item, tmp, &section->items, node) {
>>                    list_del_init(&item->node);
>> -                if (item->is_custom)
>> -                        perf_config_item__delete(item);
>> +                perf_config_item__delete(item);
>>            }
>>    }
>
> Good catch! it also should be done :)

Thanks you!

>
>>
>>
>>> So, the section->is_custom flag should be checked in *__delete
>>> method as below,
>>> static void perf_config_section__delete(struct perf_config_section *section)
>>> {
>>> 	perf_config_section__purge(section);
>>> 	if (section->is_custom) {
>>> 		zfree(&section->name);
>>> 		free(section);
>>> 	}
>>> }
>>>
>>> Or, you can just call perf_config_section__purge(section) in
>>> perf_config_set__purge().
>>>
>>
>> I'll modify this code by the former that you said like below.
>>
>>
>>    static void perf_config_section__delete(struct perf_config_section
>> *section)
>>    {
>>            perf_config_section__purge(section);
>> -        zfree((char **)&section->name);
>> -        free(section);
>> +        if (section->is_allocated) {
>> +                zfree((char **)&section->name);
>> +                free(section);
>> +        }
>>    }
>
> Looks good :)

Thanks you :-)

>>
>>    static void perf_config_set__purge(struct perf_config_set *set)
>> @@ -819,8 +822,7 @@ static void perf_config_set__purge(struct
>> perf_config_set *set)
>>
>>            list_for_each_entry_safe(section, tmp, &set->sections, node) {
>>                    list_del_init(&section->node);
>> -                if (section->is_custom)
>> -                        perf_config_section__delete(section);
>> +                perf_config_section__delete(section);
>>            }
>>    }
>>
>>
>>
>>> Also, I like ->allocated instead of ->is_custom.
>>>
>>
>> What you said is right.
>> Because the variable means whether a config section or item is allocated,
>> not customized by a user.
>> I'll rename it.
>
> Thank you!
>
>>
>>
>> Thanks,
>> Taeung
>>
>>>
>>>>    	}
>>>>    }
>>>>
>>>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>>>> index 84dcc1d..1df96b7 100644
>>>> --- a/tools/perf/util/config.h
>>>> +++ b/tools/perf/util/config.h
>>>> @@ -14,6 +14,11 @@ enum perf_config_type {
>>>>    	CONFIG_TYPE_STRING
>>>>    };
>>>>
>>>> +/**
>>>> + * struct perf_config_item - element of perf's configs
>>>> + *
>>>> + * @is_custom: unknown or new config other than default config
>>>> + */
>>>>    struct perf_config_item {
>>>>    	const char *name;
>>>>    	char *value;
>>>> @@ -27,11 +32,13 @@ struct perf_config_item {
>>>>    		const char *s;
>>>>    	} default_value;
>>>>    	enum perf_config_type type;
>>>> +	bool is_custom;
>>>>    	struct list_head node;
>>>>    };
>>>>
>>>>    struct perf_config_section {
>>>>    	const char *name;
>>>> +	bool is_custom;
>>>>    	struct list_head items;
>>>>    	struct list_head node;
>>>>    };
>>>> --
>>>> 2.5.0
>>>>
>>>
>>>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v6 4/4] perf config: Initialize perf_config_set with all default configs
  2016-04-05 11:23       ` Masami Hiramatsu
  2016-04-05 11:47         ` Taeung Song
@ 2016-04-07 10:35         ` Taeung Song
  1 sibling, 0 replies; 14+ messages in thread
From: Taeung Song @ 2016-04-07 10:35 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim,
	Ingo Molnar, Peter Zijlstra

Hi, Masami

Because of my unclear response,
I resend this email after submitting v7 this patchset. :)

On 04/05/2016 08:23 PM, Masami Hiramatsu wrote:
> On Tue, 5 Apr 2016 15:06:47 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> Hi, Masami
>>
>> On 04/04/2016 10:28 PM, Masami Hiramatsu wrote:
>>> On Mon,  4 Apr 2016 18:17:07 +0900
>>> Taeung Song <treeze.taeung@gmail.com> wrote:
>>>
>>>> To avoid duplicated config variables and
>>>> use perf_config_set classifying between standard
>>>> perf config variables and unknown or new config
>>>> variables other than them, initialize perf_config_set
>>>> with all default configs.
>>>>
>>>> And this will be needed when showing all configs with
>>>> default value or checking correct type of a config
>>>> variable in the near future.
>>>>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>>> ---
>>>>    tools/perf/builtin-config.c | 11 +++++++----
>>>>    tools/perf/util/config.c    | 33 ++++++++++++++++++++++++++++++---
>>>>    tools/perf/util/config.h    |  7 +++++++
>>>>    3 files changed, 44 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>>>> index 1133224..66a269e 100644
>>>> --- a/tools/perf/builtin-config.c
>>>> +++ b/tools/perf/builtin-config.c
>>>> @@ -35,23 +35,26 @@ static struct option config_options[] = {
>>>>
>>>>    static int show_config(struct perf_config_set *set)
>>>>    {
>>>> +	bool has_value = false;
>>>>    	struct perf_config_section *section;
>>>>    	struct perf_config_item *item;
>>>>    	struct list_head *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)
>>>> +			if (value) {
>>>>    				printf("%s.%s=%s\n", section->name,
>>>>    				       item->name, value);
>>>> +				has_value = true;
>>>> +			}
>>>>    		}
>>>>    	}
>>>>
>>>> +	if (!has_value)
>>>> +		return -1;
>>>
>>> Could this path be really executed? It seems that the set is
>>> always initialized with default values. This means not empty.
>>> If it is for asserting bugs, we also have to check set != NULL
>>> beforehand.
>>
>> I got it.
>> But before calling show_config(set), the set is checked as below
>>
>> ... (omitted) ...
>> 	set = perf_config_set__new();
>> 	if (!set) {
>> 		ret = -1;
>> 		goto out_err;
>> 	}
>>
>> 	switch (actions) {
>> 	case ACTION_LIST:
>> 		if (argc) {
>> 			pr_err("Error: takes no arguments\n");
>> 			parse_options_usage(config_usage, config_options, "l", 1);
>> 		} else {
>> 			ret = show_config(set);
>>
>> ...(omitted)...
>>
>> I thought that the set can never be NULL in show_config().
>> Is it wrong ?
>> (I'm afraid that there are things I misunderstood.)
>
> Hmm, my main point was has_value is always true because
> perf_config_set__init() initializes the set with default value.
> If so, has_value can be removed.

I tested multiple cases (e.g after removing ~/.perfconfig
or with noting configured config files(but there are config files).

But has_value isn't always true because config_item has both 'value'
and 'default_value' and perf_config_set__init() don't handle 'value'.

Even though perf_config_set__init() initializes the set with default values,
the function don't use 'value' and handle 'default_value' variable
of each config_item.

(The reason that config_item has both 'value' and 'default' is
for upcoming features (e.g. --list-all, getting, setting, --remove, etc.))

And I used has_value to know whether config files have config 
information or not in show_config().


 > But even though, if has_value
> is intended to detect unexpected bugs, also introducing (set != NULL)
> check is more natural.

I added it at '[PATCH v7 2/4] perf config: Let show_config() work with 
perf_config_set'.

Thanks,
Taeung

>
>>>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>>>> index f937124..1855560 100644
>>>> --- a/tools/perf/util/config.c
>>>> +++ b/tools/perf/util/config.c
>>>> @@ -663,6 +663,7 @@ static struct perf_config_section *add_section(struct list_head *sections,
>>>>    	if (!section)
>>>>    		return NULL;
>>>>
>>>> +	section->is_custom = true;
>>>>    	INIT_LIST_HEAD(&section->items);
>>>>    	section->name = strdup(section_name);
>>>>    	if (!section->name) {
>>>> @@ -683,6 +684,7 @@ static struct perf_config_item *add_config_item(struct perf_config_section *sect
>>>>    	if (!item)
>>>>    		return NULL;
>>>>
>>>> +	item->is_custom = true;
>>>>    	item->name = strdup(name);
>>>>    	if (!item->name) {
>>>>    		pr_debug("%s: strdup failed\n", __func__);
>>>> @@ -751,12 +753,35 @@ out_free:
>>>>    	return -1;
>>>>    }
>>>>
>>>> +static struct perf_config_set *perf_config_set__init(struct perf_config_set *set)
>>>> +{
>>>> +	int i, j;
>>>> +	struct perf_config_section *section;
>>>> +	struct perf_config_item *items;
>>>> +	struct list_head *sections = &set->sections;
>>>> +
>>>> +	INIT_LIST_HEAD(&set->sections);
>>>> +
>>>> +	for (i = 0; i != CONFIG_END; i++) {
>>>> +		section = &default_sections[i];
>>>> +		INIT_LIST_HEAD(&section->items);
>>>> +
>>>> +		items = default_config_items[i];
>>>> +		for (j = 0; items[j].name != NULL; j++)
>>>> +			list_add_tail(&items[j].node, &section->items);
>>>> +
>>>> +		list_add_tail(&section->node, sections);
>>>> +	}
>>>> +
>>>> +	return set;
>>>> +}
>>>> +
>>>>    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_set__init(set);
>>>>    		perf_config(collect_config, set);
>>>>    	}
>>>>
>>>> @@ -776,7 +801,8 @@ static void perf_config_section__purge(struct perf_config_section *section)
>>>>
>>>>    	list_for_each_entry_safe(item, tmp, &section->items, node) {
>>>>    		list_del_init(&item->node);
>>>> -		perf_config_item__delete(item);
>>>> +		if (item->is_custom)
>>>> +			perf_config_item__delete(item);
>>>>    	}
>>>>    }
>>>>
>>>> @@ -793,7 +819,8 @@ static void perf_config_set__purge(struct perf_config_set *set)
>>>>
>>>>    	list_for_each_entry_safe(section, tmp, &set->sections, node) {
>>>>    		list_del_init(&section->node);
>>>> -		perf_config_section__delete(section);
>>>> +		if (section->is_custom)
>>>> +			perf_config_section__delete(section);
>>>
>>> Here, unless perf_config_section__delete() is called, the items
>>> under the section is never be checked.
>>
>> It is my stupid mistake..
>>
>>> However, if a user changes
>>> existing item value, its section->is_custom is false, but the
>>> item itself is_custom be true.
>>> This means that such custom item values may not be freed.
>>
>> And there is one more what I missed.
>> I didn't consider that when existing item's 'value' is set by set_value(),
>> it can not be freed. So fix it as below.
>>
>>    static void perf_config_item__delete(struct perf_config_item *item)
>>    {
>> -        zfree((char **)&item->name);
>>            zfree(&item->value);
>> -        free(item);
>> +        if (item->is_allocated) {
>> +                zfree((char **)&item->name);
>> +                free(item);
>> +        }
>>    }
>>
>>    static void perf_config_section__purge(struct perf_config_section
>> *section)
>>    {
>>            struct perf_config_item *item, *tmp;
>>
>>            list_for_each_entry_safe(item, tmp, &section->items, node) {
>>                    list_del_init(&item->node);
>> -                if (item->is_custom)
>> -                        perf_config_item__delete(item);
>> +                perf_config_item__delete(item);
>>            }
>>    }
>
> Good catch! it also should be done :)
>
>>
>>
>>> So, the section->is_custom flag should be checked in *__delete
>>> method as below,
>>> static void perf_config_section__delete(struct perf_config_section *section)
>>> {
>>> 	perf_config_section__purge(section);
>>> 	if (section->is_custom) {
>>> 		zfree(&section->name);
>>> 		free(section);
>>> 	}
>>> }
>>>
>>> Or, you can just call perf_config_section__purge(section) in
>>> perf_config_set__purge().
>>>
>>
>> I'll modify this code by the former that you said like below.
>>
>>
>>    static void perf_config_section__delete(struct perf_config_section
>> *section)
>>    {
>>            perf_config_section__purge(section);
>> -        zfree((char **)&section->name);
>> -        free(section);
>> +        if (section->is_allocated) {
>> +                zfree((char **)&section->name);
>> +                free(section);
>> +        }
>>    }
>
> Looks good :)
>
>>
>>    static void perf_config_set__purge(struct perf_config_set *set)
>> @@ -819,8 +822,7 @@ static void perf_config_set__purge(struct
>> perf_config_set *set)
>>
>>            list_for_each_entry_safe(section, tmp, &set->sections, node) {
>>                    list_del_init(&section->node);
>> -                if (section->is_custom)
>> -                        perf_config_section__delete(section);
>> +                perf_config_section__delete(section);
>>            }
>>    }
>>
>>
>>
>>> Also, I like ->allocated instead of ->is_custom.
>>>
>>
>> What you said is right.
>> Because the variable means whether a config section or item is allocated,
>> not customized by a user.
>> I'll rename it.
>
> Thank you!
>
>>
>>
>> Thanks,
>> Taeung
>>
>>>
>>>>    	}
>>>>    }
>>>>
>>>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>>>> index 84dcc1d..1df96b7 100644
>>>> --- a/tools/perf/util/config.h
>>>> +++ b/tools/perf/util/config.h
>>>> @@ -14,6 +14,11 @@ enum perf_config_type {
>>>>    	CONFIG_TYPE_STRING
>>>>    };
>>>>
>>>> +/**
>>>> + * struct perf_config_item - element of perf's configs
>>>> + *
>>>> + * @is_custom: unknown or new config other than default config
>>>> + */
>>>>    struct perf_config_item {
>>>>    	const char *name;
>>>>    	char *value;
>>>> @@ -27,11 +32,13 @@ struct perf_config_item {
>>>>    		const char *s;
>>>>    	} default_value;
>>>>    	enum perf_config_type type;
>>>> +	bool is_custom;
>>>>    	struct list_head node;
>>>>    };
>>>>
>>>>    struct perf_config_section {
>>>>    	const char *name;
>>>> +	bool is_custom;
>>>>    	struct list_head items;
>>>>    	struct list_head node;
>>>>    };
>>>> --
>>>> 2.5.0
>>>>
>>>
>>>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-04-07 10:36 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-04  9:17 [PATCH v6 0/4] Infrastructure code for perf-config Taeung Song
2016-04-04  9:17 ` [PATCH v6 1/4] perf config: Introduce perf_config_set class Taeung Song
2016-04-04 12:41   ` Masami Hiramatsu
2016-04-04 12:42     ` Taeung Song
2016-04-04  9:17 ` [PATCH v6 2/4] perf config: Let show_config() work with perf_config_set Taeung Song
2016-04-04 12:49   ` Masami Hiramatsu
2016-04-05  1:54     ` Taeung Song
2016-04-04  9:17 ` [PATCH v6 3/4] perf config: Prepare all default configs Taeung Song
2016-04-04  9:17 ` [PATCH v6 4/4] perf config: Initialize perf_config_set with " Taeung Song
2016-04-04 13:28   ` Masami Hiramatsu
2016-04-05  6:06     ` Taeung Song
2016-04-05 11:23       ` Masami Hiramatsu
2016-04-05 11:47         ` Taeung Song
2016-04-07 10:35         ` 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).