Message-ID: <20160328210629.GH8706@kernel.org>
Date:	Mon, 28 Mar 2016 18:06:29 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Taeung Song <treeze.taeung@...il.com>
Cc:	Namhyung Kim <namhyung@...nel.org>, linux-kernel@...r.kernel.org,
	Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v3 1/4] perf config: Introduce perf_config_set class

Em Thu, Mar 24, 2016 at 03:46:00PM +0900, Taeung Song escreveu:
> Hi, Namhyung
> 
> On 03/24/2016 03:07 PM, Namhyung Kim wrote:
> >Hi Taeung,
> >
> >On Thu, Mar 24, 2016 at 05:16:30AM +0900, Taeung Song 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.
> >>
> >>Cc: Jiri Olsa <jolsa@...nel.org>
> >>Signed-off-by: Taeung Song <treeze.taeung@...il.com>
> >
> >Acked-by: Namhyung Kim <namhyung@...nel.org>
> >
> 
> Thank you!!
> 
> But I'm so sorry for my mistake.
> I missed perf_config_set__delete() out when the error occur in
> collect_config().
> 
> (If returning -1 from collect_config(), perf process die with some error
> message at once
> so it is needed to call perf_config_set__delete() before that moment)
> 
> So, I'll resend this patch after fixing it as below.

Ok, when resubmitting, if no changes are made to a patch that got an
Acked-by, please add that Acked-by line,

Will wait for v4 then, thanks,

- Arnaldo
 
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 725015f..3831733 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -705,14 +706,15 @@ static int set_value(struct perf_config_item
> *config_item, const char *value)
>  }
> 
>  static int collect_config(const char *var, const char *value,
> -                          void *section_list)
> +                          void *perf_config_set)
>  {
>          int ret = -1;
>          char *ptr, *key;
>          char *section_name, *name;
>          struct perf_config_section *section = NULL;
>          struct perf_config_item *config_item = NULL;
> -        struct list_head *sections = section_list;
> +        struct perf_config_set *perf_configs = perf_config_set;
> +        struct list_head *sections = &perf_configs->sections;
> 
>          key = ptr = strdup(var);
>          if (!key) {
> @@ -743,7 +745,8 @@ static int collect_config(const char *var, const char
> *value,
> 
>  out_free:
>          free(key);
> -        return ret;
> +        perf_config_set__delete(perf_configs);
> +        return -1;
>  }
> 
>  static struct perf_config_set *perf_config_set__init(struct perf_config_set
> *perf_configs)
> @@ -777,7 +780,7 @@ struct perf_config_set *perf_config_set__new(void)
>                  return NULL;
> 
>          perf_config_set__init(perf_configs);
> -        perf_config(collect_config, &perf_configs->sections);
> +        perf_config(collect_config, perf_configs);
> 
>          return perf_configs;
>  }
> 
> 
> Thanks,
> Taeung
> 
> >
> >
> >>---
> >>  tools/perf/util/config.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  tools/perf/util/config.h |  26 ++++++++
> >>  2 files changed, 195 insertions(+)
> >>  create mode 100644 tools/perf/util/config.h
> >>
> >>diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> >>index 4e72763..3beeceb 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)
> >>
> >>@@ -506,6 +507,174 @@ 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, list)
> >>+		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 *config_item;
> >>+
> >>+	list_for_each_entry(config_item, &section->config_items, list)
> >>+		if (!strcmp(config_item->name, name))
> >>+			return config_item;
> >>+
> >>+	return NULL;
> >>+}
> >>+
> >>+static void find_config(struct list_head *sections,
> >>+			struct perf_config_section **section,
> >>+			struct perf_config_item **config_item,
> >>+			const char *section_name, const char *name)
> >>+{
> >>+	*section = find_section(sections, section_name);
> >>+
> >>+	if (*section != NULL)
> >>+		*config_item = find_config_item(name, *section);
> >>+	else
> >>+		*config_item = 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->config_items);
> >>+	section->name = strdup(section_name);
> >>+	if (!section->name) {
> >>+		pr_err("%s: strdup failed\n", __func__);
> >>+		free(section);
> >>+		return NULL;
> >>+	}
> >>+
> >>+	list_add_tail(&section->list, sections);
> >>+	return section;
> >>+}
> >>+
> >>+static struct perf_config_item *add_config_item(struct perf_config_section *section,
> >>+						const char *name)
> >>+{
> >>+	struct perf_config_item *config_item = zalloc(sizeof(*config_item));
> >>+
> >>+	if (!config_item)
> >>+		return NULL;
> >>+
> >>+	config_item->name = strdup(name);
> >>+	if (!name) {
> >>+		pr_err("%s: strdup failed\n", __func__);
> >>+		goto out_err;
> >>+	}
> >>+
> >>+	list_add_tail(&config_item->list, &section->config_items);
> >>+	return config_item;
> >>+
> >>+out_err:
> >>+	free(config_item);
> >>+	return NULL;
> >>+}
> >>+
> >>+static int set_value(struct perf_config_item *config_item, const char *value)
> >>+{
> >>+	char *val = strdup(value);
> >>+
> >>+	if (!val)
> >>+		return -1;
> >>+
> >>+	free(config_item->value);
> >>+	config_item->value = val;
> >>+	return 0;
> >>+}
> >>+
> >>+static int collect_config(const char *var, const char *value,
> >>+			  void *section_list)
> >>+{
> >>+	int ret = -1;
> >>+	char *ptr, *key;
> >>+	char *section_name, *name;
> >>+	struct perf_config_section *section = NULL;
> >>+	struct perf_config_item *config_item = NULL;
> >>+	struct list_head *sections = section_list;
> >>+
> >>+	key = ptr = strdup(var);
> >>+	if (!key) {
> >>+		pr_err("%s: strdup failed\n", __func__);
> >>+		return -1;
> >>+	}
> >>+
> >>+	section_name = strsep(&ptr, ".");
> >>+	name = ptr;
> >>+	if (name == NULL || value == NULL)
> >>+		goto out_free;
> >>+
> >>+	find_config(sections, &section, &config_item, section_name, name);
> >>+
> >>+	if (!section) {
> >>+		section = add_section(sections, section_name);
> >>+		if (!section)
> >>+			goto out_free;
> >>+	}
> >>+	if (!config_item) {
> >>+		config_item = add_config_item(section, name);
> >>+		if (!config_item)
> >>+			goto out_free;
> >>+	}
> >>+
> >>+	ret = set_value(config_item, value);
> >>+	return ret;
> >>+
> >>+out_free:
> >>+	free(key);
> >>+	return ret;
> >>+}
> >>+
> >>+struct perf_config_set *perf_config_set__new(void)
> >>+{
> >>+	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
> >>+
> >>+	if (!perf_configs)
> >>+		return NULL;
> >>+
> >>+	INIT_LIST_HEAD(&perf_configs->sections);
> >>+	perf_config(collect_config, &perf_configs->sections);
> >>+
> >>+	return perf_configs;
> >>+}
> >>+
> >>+void perf_config_set__delete(struct perf_config_set *perf_configs)
> >>+{
> >>+	struct perf_config_section *section, *section_tmp;
> >>+	struct perf_config_item *config_item, *item_tmp;
> >>+
> >>+	list_for_each_entry_safe(section, section_tmp,
> >>+				 &perf_configs->sections, list) {
> >>+		list_for_each_entry_safe(config_item, item_tmp,
> >>+					 &section->config_items, list) {
> >>+			list_del(&config_item->list);
> >>+			free(config_item->name);
> >>+			free(config_item->value);
> >>+			free(config_item);
> >>+		}
> >>+		list_del(&section->list);
> >>+		free(section->name);
> >>+		free(section);
> >>+	}
> >>+
> >>+	free(perf_configs);
> >>+}
> >>+
> >>  /*
> >>   * 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..e270e51
> >>--- /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 list;
> >>+};
> >>+
> >>+struct perf_config_section {
> >>+	char *name;
> >>+	struct list_head config_items;
> >>+	struct list_head list;
> >>+};
> >>+
> >>+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 *perf_configs);
> >>+
> >>+#endif /* __PERF_CONFIG_H */
> >>--
> >>2.5.0
> >>

Powered by blists - more mailing lists