[PATCH 0/7] perf config: Bugfixes & Refactoring

[PATCH 0/7] perf config: Bugfixes & Refactoring

[Taeung Song]

Hi all, :)

This is simple patchset for perf-config
to fix small bugs and refactor code.

I'd appreciate some feedback on this patchset.

The code is also avaiable at 'config/refactoring' branch on

  git://github.com/taeung/linux-perf.git


Thanks,
Taeung

Taeung Song (7):
  perf config: Refactor a duplicated code for config file name
  perf config: Check list empty before showing configs
  perf config: Use none_err for all cases that nothing configured
  perf config: Invert if statements to reduce nesting in cmd_config()
  perf config: Correctly check whether it is from system config
  perf config: Finally write changed configs on config file at a time
  perf config: No free config set when it's initialization failed

 tools/perf/builtin-config.c | 92 +++++++++++++++++++++++++--------------------
 tools/perf/util/config.c    |  7 +---
 2 files changed, 54 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCH 1/7] perf config: Refactor a duplicated code for config file name

[Taeung Song]

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 55f04f8..80668fa 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -159,6 +159,7 @@ int cmd_config(int argc, const char **argv)
 	int i, ret = 0;
 	struct perf_config_set *set;
 	char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
+	const char *config_filename;
 
 	argc = parse_options(argc, argv, config_options, config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -175,6 +176,11 @@ int cmd_config(int argc, const char **argv)
 	else if (use_user_config)
 		config_exclusive_filename = user_config;
 
+	if (!config_exclusive_filename)
+		config_filename = user_config;
+	else
+		config_filename = config_exclusive_filename;
+
 	/*
 	 * At only 'config' sub-command, individually use the config set
 	 * because of reinitializing with options config file location.
@@ -192,13 +198,9 @@ int cmd_config(int argc, const char **argv)
 			parse_options_usage(config_usage, config_options, "l", 1);
 		} else {
 			ret = show_config(set);
-			if (ret < 0) {
-				const char * config_filename = config_exclusive_filename;
-				if (!config_exclusive_filename)
-					config_filename = user_config;
+			if (ret < 0)
 				pr_err("Nothing configured, "
 				       "please check your %s \n", config_filename);
-			}
 		}
 		break;
 	default:
@@ -221,13 +223,8 @@ int cmd_config(int argc, const char **argv)
 
 				if (value == NULL)
 					ret = show_spec_config(set, var);
-				else {
-					const char *config_filename = config_exclusive_filename;
-
-					if (!config_exclusive_filename)
-						config_filename = user_config;
+				else
 					ret = set_config(set, config_filename, var, value);
-				}
 				free(arg);
 			}
 		} else
-- 
2.7.4

[PATCH 2/7] perf config: Check list empty before showing configs

[Taeung Song]

If existent config files contains nothing,
the sections list in config_set can be empty.

So check not only NULL pointer of config_set but
also the list in config_set.

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 80668fa..9ec8664 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -75,7 +75,7 @@ static int show_spec_config(struct perf_config_set *set, const char *var)
 	struct perf_config_section *section;
 	struct perf_config_item *item;
 
-	if (set == NULL)
+	if (set == NULL || list_empty(&set->sections))
 		return -1;
 
 	perf_config_items__for_each_entry(&set->sections, section) {
@@ -105,7 +105,7 @@ static int show_config(struct perf_config_set *set)
 	struct perf_config_section *section;
 	struct perf_config_item *item;
 
-	if (set == NULL)
+	if (set == NULL || list_empty(&set->sections))
 		return -1;
 
 	perf_config_set__for_each_entry(set, section, item) {
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 8d724f0..492c862 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -707,7 +707,7 @@ int perf_config(config_fn_t fn, void *data)
 	struct perf_config_section *section;
 	struct perf_config_item *item;
 
-	if (config_set == NULL)
+	if (config_set == NULL || list_empty(&config_set->sections))
 		return -1;
 
 	perf_config_set__for_each_entry(config_set, section, item) {
-- 
2.7.4

[PATCH 3/7] perf config: Use none_err for all cases that nothing configured

[Taeung Song]

Currently there's only one error message for nothing configured.
Before:

  $ perf config -l
  Nothing configured, please check your /home/taeung/.perfconfig

  $ perf config report.queue-size

So use none_err to handle all cases when nothing configured,
and also use it instead of out_err that skip the error message.

After:

  $ perf config -l
  Nothing configured, please check your /home/taeung/.perfconfig

  $ perf config report.queue-size
  Nothing configured, please check your /home/taeung/.perfconfig

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 9ec8664..ad0a112 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -188,7 +188,7 @@ int cmd_config(int argc, const char **argv)
 	set = perf_config_set__new();
 	if (!set) {
 		ret = -1;
-		goto out_err;
+		goto none_err;
 	}
 
 	switch (actions) {
@@ -199,8 +199,7 @@ int cmd_config(int argc, const char **argv)
 		} else {
 			ret = show_config(set);
 			if (ret < 0)
-				pr_err("Nothing configured, "
-				       "please check your %s \n", config_filename);
+				goto none_err;
 		}
 		break;
 	default:
@@ -221,9 +220,11 @@ int cmd_config(int argc, const char **argv)
 					break;
 				}
 
-				if (value == NULL)
+				if (value == NULL) {
 					ret = show_spec_config(set, var);
-				else
+					if (ret < 0)
+						goto none_err;
+				} else
 					ret = set_config(set, config_filename, var, value);
 				free(arg);
 			}
@@ -231,7 +232,11 @@ int cmd_config(int argc, const char **argv)
 			usage_with_options(config_usage, config_options);
 	}
 
+none_err:
+	if (ret < 0 && (!set || list_empty(&set->sections)))
+		pr_err("Nothing configured, "
+		       "please check your %s \n", config_filename);
+
 	perf_config_set__delete(set);
-out_err:
 	return ret;
 }
-- 
2.7.4

[PATCH 4/7] perf config: Invert if statements to reduce nesting in cmd_config()

[Taeung Song]

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index ad0a112..f4596ef 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -203,33 +203,37 @@ int cmd_config(int argc, const char **argv)
 		}
 		break;
 	default:
-		if (argc) {
-			for (i = 0; argv[i]; i++) {
-				char *var, *value;
-				char *arg = strdup(argv[i]);
-
-				if (!arg) {
-					pr_err("%s: strdup failed\n", __func__);
-					ret = -1;
-					break;
-				}
+		if (!argc) {
+			usage_with_options(config_usage, config_options);
+			break;
+		}
 
-				if (parse_config_arg(arg, &var, &value) < 0) {
-					free(arg);
-					ret = -1;
-					break;
-				}
+		for (i = 0; argv[i]; i++) {
+			char *var, *value;
+			char *arg = strdup(argv[i]);
+
+			if (!arg) {
+				pr_err("%s: strdup failed\n", __func__);
+				ret = -1;
+				break;
+			}
 
-				if (value == NULL) {
-					ret = show_spec_config(set, var);
-					if (ret < 0)
-						goto none_err;
-				} else
-					ret = set_config(set, config_filename, var, value);
+			if (parse_config_arg(arg, &var, &value) < 0) {
 				free(arg);
+				ret = -1;
+				break;
 			}
-		} else
-			usage_with_options(config_usage, config_options);
+
+			if (value) {
+				ret = set_config(set, config_filename, var, value);
+				continue;
+			}
+			ret = show_spec_config(set, var);
+			if (ret < 0)
+				goto none_err;
+
+			free(arg);
+		}
 	}
 
 none_err:
-- 
2.7.4

[PATCH 5/7] perf config: Correctly check whether it is from system config

[Taeung Song]

Currently no bugs in the checking code.
But adjust it to correctly check item->from_system_config,
not section's from_system_config.

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index f4596ef..c76aacf 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -58,7 +58,7 @@ static int set_config(struct perf_config_set *set, const char *file_name,
 		fprintf(fp, "[%s]\n", section->name);
 
 		perf_config_items__for_each_entry(&section->items, item) {
-			if (!use_system_config && section->from_system_config)
+			if (!use_system_config && item->from_system_config)
 				continue;
 			if (item->value)
 				fprintf(fp, "\t%s = %s\n",
-- 
2.7.4

[PATCH 6/7] perf config: Finally write changed configs on config file at a time

[Taeung Song]

Currently set_config() can be repeatedly called for each
input config on the below case:

  $ perf config kmem.default=slab report.children=false ...

But it's a waste, so finally write changed configs at a time.

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c76aacf..793a729 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -33,8 +33,7 @@ static struct option config_options[] = {
 	OPT_END()
 };
 
-static int set_config(struct perf_config_set *set, const char *file_name,
-		      const char *var, const char *value)
+static int set_config(struct perf_config_set *set, const char *file_name)
 {
 	struct perf_config_section *section = NULL;
 	struct perf_config_item *item = NULL;
@@ -48,7 +47,6 @@ static int set_config(struct perf_config_set *set, const char *file_name,
 	if (!fp)
 		return -1;
 
-	perf_config_set__collect(set, file_name, var, value);
 	fprintf(fp, "%s\n", first_line);
 
 	/* overwrite configvariables */
@@ -160,6 +158,7 @@ int cmd_config(int argc, const char **argv)
 	struct perf_config_set *set;
 	char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
 	const char *config_filename;
+	bool changed = false;
 
 	argc = parse_options(argc, argv, config_options, config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -225,7 +224,11 @@ int cmd_config(int argc, const char **argv)
 			}
 
 			if (value) {
-				ret = set_config(set, config_filename, var, value);
+				ret = perf_config_set__collect(set, config_filename,
+							       var, value);
+				if (ret < 0)
+					break;
+				changed = true;
 				continue;
 			}
 			ret = show_spec_config(set, var);
@@ -234,6 +237,9 @@ int cmd_config(int argc, const char **argv)
 
 			free(arg);
 		}
+
+		if (changed)
+			ret = set_config(set, config_filename);
 	}
 
 none_err:
-- 
2.7.4

[PATCH 7/7] perf config: No free config set when it's initialization failed

[Taeung Song]

Currently if perf_config_set__init() failed in perf_config_set__new(),
config_set will be freed.

However, if we do, config setting feature can't work sometimes
when  user or system config files are nonexistent.
So let the config set be empty, not freed totally.
(it'll be freed at the tail end)

Before:

  $ cat ~/.perfconfig
  cat: /root/.perfconfig: No such file or directory

  $ perf config --user report.children=false
  Nothing configured, please check your /root/.perfconfig

After:

  $ cat ~/.perfconfig
  cat: /root/.perfconfig: No such file or directory

  $ perf config --user report.children=false

  $ cat ~/.perfconfig
  # this file is auto-generated.
  [report]
	  children = false

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

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 492c862..3c89d74 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -691,10 +691,7 @@ struct perf_config_set *perf_config_set__new(void)
 
 	if (set) {
 		INIT_LIST_HEAD(&set->sections);
-		if (perf_config_set__init(set) < 0) {
-			perf_config_set__delete(set);
-			set = NULL;
-		}
+		perf_config_set__init(set);
 	}
 
 	return set;
-- 
2.7.4

Re: [PATCH 2/7] perf config: Check list empty before showing configs

[Arnaldo Carvalho de Melo]

Em Wed, Apr 26, 2017 at 09:21:03PM +0900, Taeung Song escreveu:
> If existent config files contains nothing,
> the sections list in config_set can be empty.
> 
> So check not only NULL pointer of config_set but
> also the list in config_set.
<SNIP> 
> +++ b/tools/perf/builtin-config.c
> @@ -75,7 +75,7 @@ static int show_spec_config(struct perf_config_set *set, const char *var)
>  	struct perf_config_section *section;
>  	struct perf_config_item *item;
>  
> -	if (set == NULL)
> +	if (set == NULL || list_empty(&set->sections))
>  		return -1;

But should we consider an error to have an empty config file? I don't
think so :-\

- Arnaldo
>  
>  	perf_config_items__for_each_entry(&set->sections, section) {
> @@ -105,7 +105,7 @@ static int show_config(struct perf_config_set *set)
>  	struct perf_config_section *section;
>  	struct perf_config_item *item;
>  
> -	if (set == NULL)
> +	if (set == NULL || list_empty(&set->sections))
>  		return -1;
>  
>  	perf_config_set__for_each_entry(set, section, item) {
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 8d724f0..492c862 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -707,7 +707,7 @@ int perf_config(config_fn_t fn, void *data)
>  	struct perf_config_section *section;
>  	struct perf_config_item *item;
>  
> -	if (config_set == NULL)
> +	if (config_set == NULL || list_empty(&config_set->sections))
>  		return -1;
>  
>  	perf_config_set__for_each_entry(config_set, section, item) {
> -- 
> 2.7.4

Re: [PATCH 2/7] perf config: Check list empty before showing configs

[Taeung Song]

Hi Arnaldo,

On 05/03/2017 12:12 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 26, 2017 at 09:21:03PM +0900, Taeung Song escreveu:
>> If existent config files contains nothing,
>> the sections list in config_set can be empty.
>>
>> So check not only NULL pointer of config_set but
>> also the list in config_set.
> <SNIP>
>> +++ b/tools/perf/builtin-config.c
>> @@ -75,7 +75,7 @@ static int show_spec_config(struct perf_config_set *set, const char *var)
>>  	struct perf_config_section *section;
>>  	struct perf_config_item *item;
>>
>> -	if (set == NULL)
>> +	if (set == NULL || list_empty(&set->sections))
>>  		return -1;
>
> But should we consider an error to have an empty config file? I don't
> think so :-\
>
> - Arnaldo

I think if we do, when a config file is not only not exist but also empty,
user can see the error message
(e.g. "Nothing configured, please check your ~/.perfconfig").
And IMHO, it seems better.

But if you don't think so, I got it.

Thanks,
Taeung

[tip:perf/urgent] perf config: Refactor a duplicated code for obtaining config file name

[tip-bot for Taeung Song]

Commit-ID:  4341ec6b3db4c3e903d6c44958722918baec1e59
Gitweb:     http://git.kernel.org/tip/4341ec6b3db4c3e903d6c44958722918baec1e59
Author:     Taeung Song <treeze.taeung@gmail.com>
AuthorDate: Wed, 26 Apr 2017 21:21:02 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 2 May 2017 18:23:12 -0300

perf config: Refactor a duplicated code for obtaining config file name

We were doing the same sequence to figure out what is the config
pathname to use, fix it by doing it before those two uses.

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/1493209268-5543-2-git-send-email-treeze.taeung@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-config.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 55f04f8..80668fa 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -159,6 +159,7 @@ int cmd_config(int argc, const char **argv)
 	int i, ret = 0;
 	struct perf_config_set *set;
 	char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
+	const char *config_filename;
 
 	argc = parse_options(argc, argv, config_options, config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -175,6 +176,11 @@ int cmd_config(int argc, const char **argv)
 	else if (use_user_config)
 		config_exclusive_filename = user_config;
 
+	if (!config_exclusive_filename)
+		config_filename = user_config;
+	else
+		config_filename = config_exclusive_filename;
+
 	/*
 	 * At only 'config' sub-command, individually use the config set
 	 * because of reinitializing with options config file location.
@@ -192,13 +198,9 @@ int cmd_config(int argc, const char **argv)
 			parse_options_usage(config_usage, config_options, "l", 1);
 		} else {
 			ret = show_config(set);
-			if (ret < 0) {
-				const char * config_filename = config_exclusive_filename;
-				if (!config_exclusive_filename)
-					config_filename = user_config;
+			if (ret < 0)
 				pr_err("Nothing configured, "
 				       "please check your %s \n", config_filename);
-			}
 		}
 		break;
 	default:
@@ -221,13 +223,8 @@ int cmd_config(int argc, const char **argv)
 
 				if (value == NULL)
 					ret = show_spec_config(set, var);
-				else {
-					const char *config_filename = config_exclusive_filename;
-
-					if (!config_exclusive_filename)
-						config_filename = user_config;
+				else
 					ret = set_config(set, config_filename, var, value);
-				}
 				free(arg);
 			}
 		} else

[PATCH 1/7] perf config: Refactor a duplicated code for config file name

[Taeung Song]

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

diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 55f04f8..80668fa 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -159,6 +159,7 @@ int cmd_config(int argc, const char **argv)
 	int i, ret = 0;
 	struct perf_config_set *set;
 	char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
+	const char *config_filename;
 
 	argc = parse_options(argc, argv, config_options, config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -175,6 +176,11 @@ int cmd_config(int argc, const char **argv)
 	else if (use_user_config)
 		config_exclusive_filename = user_config;
 
+	if (!config_exclusive_filename)
+		config_filename = user_config;
+	else
+		config_filename = config_exclusive_filename;
+
 	/*
 	 * At only 'config' sub-command, individually use the config set
 	 * because of reinitializing with options config file location.
@@ -192,13 +198,9 @@ int cmd_config(int argc, const char **argv)
 			parse_options_usage(config_usage, config_options, "l", 1);
 		} else {
 			ret = show_config(set);
-			if (ret < 0) {
-				const char * config_filename = config_exclusive_filename;
-				if (!config_exclusive_filename)
-					config_filename = user_config;
+			if (ret < 0)
 				pr_err("Nothing configured, "
 				       "please check your %s \n", config_filename);
-			}
 		}
 		break;
 	default:
@@ -221,13 +223,8 @@ int cmd_config(int argc, const char **argv)
 
 				if (value == NULL)
 					ret = show_spec_config(set, var);
-				else {
-					const char *config_filename = config_exclusive_filename;
-
-					if (!config_exclusive_filename)
-						config_filename = user_config;
+				else
 					ret = set_config(set, config_filename, var, value);
-				}
 				free(arg);
 			}
 		} else
-- 
2.7.4