[PATCH v4 1/4] perf config: Introduce perf_config_set class

[PATCH v4 1/4] perf config: Introduce perf_config_set class

[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 | 171 +++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/config.h |  26 +++++++
 2 files changed, 197 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..2dbf47c 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,176 @@ 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 *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 perf_config_set *perf_configs = perf_config_set;
+	struct list_head *sections = &perf_configs->sections;
+
+	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);
+	perf_config_set__delete(perf_configs);
+	return -1;
+}
+
+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);
+
+	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

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Arnaldo Carvalho de Melo]

Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
> 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>

Waiting for ack.
> ---
>  tools/perf/util/config.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/config.h |  26 +++++++
>  2 files changed, 197 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..2dbf47c 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,176 @@ 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 *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 perf_config_set *perf_configs = perf_config_set;
> +	struct list_head *sections = &perf_configs->sections;
> +
> +	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);
> +	perf_config_set__delete(perf_configs);
> +	return -1;
> +}
> +
> +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);
> +
> +	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

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Taeung Song]

Hi, Arnaldo and Namhyung

On 03/30/2016 01:12 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
>> 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>
>
> Waiting for ack.

Namhyung,
The difference between v3 and v4 for this patch as below.
(fill perf_config_set__delete() in collect_config() for state of error)

Can you review this patch, again ?

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 725015f..2cfafff 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 | 171 +++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/config.h |  26 +++++++
>>   2 files changed, 197 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..2dbf47c 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,176 @@ 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 *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 perf_config_set *perf_configs = perf_config_set;
>> +	struct list_head *sections = &perf_configs->sections;
>> +
>> +	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);
>> +	perf_config_set__delete(perf_configs);
>> +	return -1;
>> +}
>> +
>> +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);
>> +
>> +	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

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Namhyung Kim]

On March 30, 2016 8:28:21 AM GMT+09:00, Taeung Song <treeze.taeung@gmail.com> wrote:
>Hi, Arnaldo and Namhyung
>
>On 03/30/2016 01:12 AM, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
>>> 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>
>>
>> Waiting for ack.
>
>Namhyung,
>The difference between v3 and v4 for this patch as below.
>(fill perf_config_set__delete() in collect_config() for state of error)
>
>Can you review this patch, again ?

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks
Namhyung


>diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>index 725015f..2cfafff 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 | 171
>+++++++++++++++++++++++++++++++++++++++++++++++
>>>   tools/perf/util/config.h |  26 +++++++
>>>   2 files changed, 197 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..2dbf47c 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,176 @@ 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 *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 perf_config_set *perf_configs = perf_config_set;
>>> +	struct list_head *sections = &perf_configs->sections;
>>> +
>>> +	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);
>>> +	perf_config_set__delete(perf_configs);
>>> +	return -1;
>>> +}
>>> +
>>> +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);
>>> +
>>> +	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


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Taeung Song]

Hi, Namhyung

On 03/30/2016 11:09 AM, Namhyung Kim wrote:
> On March 30, 2016 8:28:21 AM GMT+09:00, Taeung Song <treeze.taeung@gmail.com> wrote:
>> Hi, Arnaldo and Namhyung
>>
>> On 03/30/2016 01:12 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
>>>> 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>
>>>
>>> Waiting for ack.
>>
>> Namhyung,
>> The difference between v3 and v4 for this patch as below.
>> (fill perf_config_set__delete() in collect_config() for state of error)
>>
>> Can you review this patch, again ?
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
>

Thank you !! :-)

Taeung

>
>
>> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
>> index 725015f..2cfafff 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 | 171
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    tools/perf/util/config.h |  26 +++++++
>>>>    2 files changed, 197 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..2dbf47c 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,176 @@ 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 *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 perf_config_set *perf_configs = perf_config_set;
>>>> +	struct list_head *sections = &perf_configs->sections;
>>>> +
>>>> +	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);
>>>> +	perf_config_set__delete(perf_configs);
>>>> +	return -1;
>>>> +}
>>>> +
>>>> +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);
>>>> +
>>>> +	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
>
>

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Arnaldo Carvalho de Melo]

Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
> 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 | 171 +++++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/config.h |  26 +++++++
>  2 files changed, 197 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..2dbf47c 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,176 @@ 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) {

Huh? You're testing the wrong variable.

> +		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 *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 perf_config_set *perf_configs = perf_config_set;
> +	struct list_head *sections = &perf_configs->sections;
> +
> +	key = ptr = strdup(var);
> +	if (!key) {
> +		pr_err("%s: strdup failed\n", __func__);

pr_debug()

> +		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);

This idiom is confusing, why not ditch this 'find_config()' function and
do the searches here? I.e.:

	section = find_section(sections, section_name);
> +	if (!section) {
> +		section = add_section(sections, section_name);
> +		if (!section)
> +			goto out_free;
> +	}

 	config_item = find_config_item(name, section);
> +	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);
> +	perf_config_set__delete(perf_configs);
> +	return -1;
> +}
> +
> +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);
> +
> +	return perf_configs;
> +}

Usually for these short functions we could do it more compactly as:

struct perf_config_set *perf_config_set__new(void)
{
	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));

	if (perf_configs) {
		INIT_LIST_HEAD(&perf_configs->sections);
		perf_config(collect_config, perf_configs);
	}

	return perf_configs;
}

But I'm not dwelling on this...

> +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);
> +}

What is the problem with having perf_config_item__delete() and
perf_config_section__delete() and then have it like below, also please
rename those foo->list to foo->node.

void perf_config_item__delete(struct perf_config_item *item)
{
	zfree(&item->name);
	zfree(&item->value);
	free(item);
}

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);
	}
}

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

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);
}

Using zfree() and list_del_init to NULL or poison the freed pointers
helps with debugging, please use them.

> +
>  /*
>   * 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;

s/list/node/g

> +};
> +
> +struct perf_config_section {
> +	char *name;
> +	struct list_head config_items;

s/config_items/items/g

> +	struct list_head list;

s/list/node/g
> +};
> +
> +struct perf_config_set {
> +	struct list_head sections;

See? Here you did it right, no point in having it as "config_sections"

> +};
> +
> +struct perf_config_set *perf_config_set__new(void);
> +void perf_config_set__delete(struct perf_config_set *perf_configs);

   void perf_config_set__delete(struct perf_config_set *set);

> +
> +#endif /* __PERF_CONFIG_H */
> -- 
> 2.5.0

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Taeung Song]

Hi, Arnaldo

Thank you for your review.

On 04/01/2016 02:31 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
>> 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 | 171 +++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/config.h |  26 +++++++
>>   2 files changed, 197 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..2dbf47c 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,176 @@ 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) {
>
> Huh? You're testing the wrong variable.
>

Sorry, my stupid mistake..

>> +		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 *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 perf_config_set *perf_configs = perf_config_set;
>> +	struct list_head *sections = &perf_configs->sections;
>> +
>> +	key = ptr = strdup(var);
>> +	if (!key) {
>> +		pr_err("%s: strdup failed\n", __func__);
>
> pr_debug()
>

I'll change pr_err to pr_debug.
But why do use pr_debug at only this part ?

>> +		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);
>
> This idiom is confusing, why not ditch this 'find_config()' function and
> do the searches here? I.e.:
>
> 	section = find_section(sections, section_name);

I got it.
I think it is needed to remove needless find_config() function as you said.

>> +	if (!section) {
>> +		section = add_section(sections, section_name);
>> +		if (!section)
>> +			goto out_free;
>> +	}
>
>   	config_item = find_config_item(name, section);

ok.

>> +	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);
>> +	perf_config_set__delete(perf_configs);
>> +	return -1;
>> +}
>> +
>> +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);
>> +
>> +	return perf_configs;
>> +}
>
> Usually for these short functions we could do it more compactly as:
>
> struct perf_config_set *perf_config_set__new(void)
> {
> 	struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs));
>
> 	if (perf_configs) {
> 		INIT_LIST_HEAD(&perf_configs->sections);
> 		perf_config(collect_config, perf_configs);
> 	}
>
> 	return perf_configs;
> }
>
> But I'm not dwelling on this...
>

I got it, too!

>> +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);
>> +}
>
> What is the problem with having perf_config_item__delete() and
> perf_config_section__delete() and then have it like below, also please
> rename those foo->list to foo->node.
>

No problem!
OK, I'll rename it.

> void perf_config_item__delete(struct perf_config_item *item)
> {
> 	zfree(&item->name);
> 	zfree(&item->value);
> 	free(item);
> }
>
> 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);
> 	}
> }
>
> void perf_config_section__delete(struct perf_config_section *section)
> {
> 	perf_config_section__purge(section);
> 	zfree(&section->name);
> 	free(section);
> }
>
> 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);
> }
>
> Using zfree() and list_del_init to NULL or poison the freed pointers
> helps with debugging, please use them.

ok.

>
>> +
>>   /*
>>    * 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;
>
> s/list/node/g
>
>> +};
>> +
>> +struct perf_config_section {
>> +	char *name;
>> +	struct list_head config_items;
>
> s/config_items/items/g
>
>> +	struct list_head list;
>
> s/list/node/g
>> +};
>> +
>> +struct perf_config_set {
>> +	struct list_head sections;
>
> See? Here you did it right, no point in having it as "config_sections"
>

I'll rename it to 'config_sections'.

>> +};
>> +
>> +struct perf_config_set *perf_config_set__new(void);
>> +void perf_config_set__delete(struct perf_config_set *perf_configs);
>
>     void perf_config_set__delete(struct perf_config_set *set);
>

OK, I'll use 'set' variable name instead of perf_configs on this source 
file.

I'll resend this patch after modifying what you said. :-)

Thanks,
Taeung

>> +
>> +#endif /* __PERF_CONFIG_H */
>> --
>> 2.5.0

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Arnaldo Carvalho de Melo]

Em Fri, Apr 01, 2016 at 07:27:22PM +0900, Taeung Song escreveu:
> On 04/01/2016 02:31 AM, Arnaldo Carvalho de Melo wrote:
> >Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
> >>+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 *config_item = NULL;
> >>+	struct perf_config_set *perf_configs = perf_config_set;
> >>+	struct list_head *sections = &perf_configs->sections;

> >>+	key = ptr = strdup(var);
> >>+	if (!key) {
> >>+		pr_err("%s: strdup failed\n", __func__);

> >pr_debug()
 
> I'll change pr_err to pr_debug.
> But why do use pr_debug at only this part ?

Well, ideally one would propagate the errors from library level code to
the code in the tool, i.e. closer to builtin-foo.c, where it would
decide how to present it to the user.

For extra messages, that may help a more advanced user or to be sent to
the tool developers, use pr_debug().

<SNIP>
 
> >>+struct perf_config_section {
> >>+	char *name;
> >>+	struct list_head config_items;

> >s/config_items/items/g

> >>+	struct list_head list;

> >s/list/node/g
> >>+};
> >>+
> >>+struct perf_config_set {
> >>+	struct list_head sections;

> >See? Here you did it right, no point in having it as "config_sections"

> I'll rename it to 'config_sections'.

I meant to keep this one like it is, i.e. perf_config_set->sections, and
use the same principle: shorter names, removing useless stuff like
"config_", that in this case can be implied by the name of the class,
and apply it to the perf_config_section case, making it
perf_config_section->items, ok?

- Arnaldo

Re: [PATCH v4 1/4] perf config: Introduce perf_config_set class

[Taeung Song]

On 04/01/2016 11:35 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 01, 2016 at 07:27:22PM +0900, Taeung Song escreveu:
>> On 04/01/2016 02:31 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Mar 29, 2016 at 09:43:13AM +0900, Taeung Song escreveu:
>>>> +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 *config_item = NULL;
>>>> +	struct perf_config_set *perf_configs = perf_config_set;
>>>> +	struct list_head *sections = &perf_configs->sections;
>
>>>> +	key = ptr = strdup(var);
>>>> +	if (!key) {
>>>> +		pr_err("%s: strdup failed\n", __func__);
>
>>> pr_debug()
>
>> I'll change pr_err to pr_debug.
>> But why do use pr_debug at only this part ?
>
> Well, ideally one would propagate the errors from library level code to
> the code in the tool, i.e. closer to builtin-foo.c, where it would
> decide how to present it to the user.
>
> For extra messages, that may help a more advanced user or to be sent to
> the tool developers, use pr_debug().

I understood it!

>
>>>> +struct perf_config_section {
>>>> +	char *name;
>>>> +	struct list_head config_items;
>
>>> s/config_items/items/g
>
>>>> +	struct list_head list;
>
>>> s/list/node/g
>>>> +};
>>>> +
>>>> +struct perf_config_set {
>>>> +	struct list_head sections;
>
>>> See? Here you did it right, no point in having it as "config_sections"
>
>> I'll rename it to 'config_sections'.
>
> I meant to keep this one like it is, i.e. perf_config_set->sections, and
> use the same principle: shorter names, removing useless stuff like
> "config_", that in this case can be implied by the name of the class,
> and apply it to the perf_config_section case, making it
> perf_config_section->items, ok?

I got it. :-)

I'll resend v5

Thanks,
Taeung