Re: [PATCH 09/15] perf config: Add support setting variables in a config file

[Taeung Song <treeze.taeung@gmail.com>]

Hi, Arnaldo :)

On 11/15/2016 10:38 AM, Arnaldo Carvalho de Melo wrote:
> From: Taeung Song <treeze.taeung@gmail.com>
>
> Add setting feature that can add config variables with their values to a
> config file (i.e. user or system config file) or modify config key-value
> pairs in a config file.  For the syntax examples:
>
>     perf config [<file-option>] [section.name[=value] ...]
>
> e.g. You can set the ui.show-headers to false with
>
>     # perf config ui.show-headers=false
>
> If you want to add or modify several config items, you can do like
>
>     # perf config annotate.show_nr_jumps=false kmem.default=slab
>
> Committer notes:
>
> Testing it:
>
>   $ perf config -l
>   top.children=true
>   report.children=false
>   $
>   $ perf config top.children=false
>   $ perf config -l
>   top.children=false
>   report.children=false
>   $
>   $ perf config kmem.default=slab
>   $ perf config -l
>   top.children=false
>   report.children=false
>   kmem.default=slab
>   $
>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Nambong Ha <over3025@gmail.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: Wookje Kwon <aweee0@gmail.com>
> Link: http://lkml.kernel.org/r/1478241862-31230-5-git-send-email-treeze.taeung@gmail.com
> [ Combined patch with docs update with this one ]
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/Documentation/perf-config.txt | 19 ++++++++-
>  tools/perf/builtin-config.c              | 68 +++++++++++++++++++++++++++++---
>  tools/perf/util/config.c                 |  6 +++
>  tools/perf/util/config.h                 |  2 +
>  4 files changed, 88 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index 1714b0c8c8e1..9365b75fd04f 100644
> --- a/tools/perf/Documentation/perf-config.txt
> +++ b/tools/perf/Documentation/perf-config.txt
> @@ -8,7 +8,7 @@ perf-config - Get and set variables in a configuration file.
>  SYNOPSIS
>  --------
>  [verse]
> -'perf config' [<file-option>] [section.name ...]
> +'perf config' [<file-option>] [section.name[=value] ...]
>  or
>  'perf config' [<file-option>] -l | --list
>
> @@ -120,6 +120,23 @@ Given a $HOME/.perfconfig like this:
>  		children = true
>  		group = true
>
> +You can hide source code of annotate feature setting the config to false with
> +
> +	% perf config annotate.hide_src_code=true
> +
> +If you want to add or modify several config items, you can do like
> +
> +	% perf config ui.show-headers=false kmem.default=slab
> +
> +To modify the sort order of report functionality in user config file(i.e. `~/.perfconfig`), do
> +
> +	% perf config --user report sort-order=srcline
> +
> +To change colors of selected line to other foreground and background colors
> +in system config file (i.e. `$(sysconf)/perfconfig`), do
> +
> +	% perf config --system colors.selected=yellow,green
> +
>  To query the record mode of call graph, do
>
>  	% perf config call-graph.record-mode
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index 88a43fe4963c..7c861b54f3a6 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -17,7 +17,7 @@
>  static bool use_system_config, use_user_config;
>
>  static const char * const config_usage[] = {
> -	"perf config [<file-option>] [options] [section.name ...]",
> +	"perf config [<file-option>] [options] [section.name[=value] ...]",
>  	NULL
>  };
>
> @@ -33,6 +33,39 @@ 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)
> +{
> +	struct perf_config_section *section = NULL;
> +	struct perf_config_item *item = NULL;
> +	const char *first_line = "# this file is auto-generated.";
> +	FILE *fp;
> +
> +	if (set == NULL)
> +		return -1;
> +
> +	fp = fopen(file_name, "w");
> +	if (!fp)
> +		return -1;
> +
> +	perf_config_set__collect(set, var, value);
> +	fprintf(fp, "%s\n", first_line);
> +
> +	/* overwrite configvariables */

We need to add a space between two word as you said..
But it is so minor part, is it ok ?

Thanks,
Taeung

> +	perf_config_items__for_each_entry(&set->sections, section) {
> +		fprintf(fp, "[%s]\n", section->name);
> +
> +		perf_config_items__for_each_entry(&section->items, item) {
> +			if (item->value)
> +				fprintf(fp, "\t%s = %s\n",
> +					item->name, item->value);
> +		}
> +	}
> +	fclose(fp);
> +
> +	return 0;
> +}
> +
>  static int show_spec_config(struct perf_config_set *set, const char *var)
>  {
>  	struct perf_config_section *section;
> @@ -82,7 +115,7 @@ static int show_config(struct perf_config_set *set)
>  	return 0;
>  }
>
> -static int parse_config_arg(char *arg, char **var)
> +static int parse_config_arg(char *arg, char **var, char **value)
>  {
>  	const char *last_dot = strchr(arg, '.');
>
> @@ -99,7 +132,21 @@ static int parse_config_arg(char *arg, char **var)
>  		return -1;
>  	}
>
> -	*var = arg;
> +	*value = strchr(arg, '=');
> +	if (*value == NULL)
> +		*var = arg;
> +	else if (!strcmp(*value, "=")) {
> +		pr_err("The config variable does not contain a value: %s\n", arg);
> +		return -1;
> +	} else {
> +		*value = *value + 1; /* excluding a first character '=' */
> +		*var = strsep(&arg, "=");
> +		if (*var[0] == '\0') {
> +			pr_err("invalid config variable: %s\n", arg);
> +			return -1;
> +		}
> +	}
> +
>  	return 0;
>  }
>
> @@ -153,7 +200,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  	default:
>  		if (argc) {
>  			for (i = 0; argv[i]; i++) {
> -				char *var, *arg = strdup(argv[i]);
> +				char *var, *value;
> +				char *arg = strdup(argv[i]);
>
>  				if (!arg) {
>  					pr_err("%s: strdup failed\n", __func__);
> @@ -161,13 +209,21 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  					break;
>  				}
>
> -				if (parse_config_arg(arg, &var) < 0) {
> +				if (parse_config_arg(arg, &var, &value) < 0) {
>  					free(arg);
>  					ret = -1;
>  					break;
>  				}
>
> -				ret = show_spec_config(set, var);
> +				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;
> +					ret = set_config(set, config_filename, var, value);
> +				}
>  				free(arg);
>  			}
>  		} else
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 18dae745034f..c8fb65d923cb 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -602,6 +602,12 @@ static int collect_config(const char *var, const char *value,
>  	return -1;
>  }
>
> +int perf_config_set__collect(struct perf_config_set *set,
> +			     const char *var, const char *value)
> +{
> +	return collect_config(var, value, set);
> +}
> +
>  static int perf_config_set__init(struct perf_config_set *set)
>  {
>  	int ret = -1;
> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
> index 6f813d46045e..0fcdb8c594b0 100644
> --- a/tools/perf/util/config.h
> +++ b/tools/perf/util/config.h
> @@ -33,6 +33,8 @@ const char *perf_etc_perfconfig(void);
>
>  struct perf_config_set *perf_config_set__new(void);
>  void perf_config_set__delete(struct perf_config_set *set);
> +int perf_config_set__collect(struct perf_config_set *set,
> +			     const char *var, const char *value);
>  void perf_config__init(void);
>  void perf_config__exit(void);
>  void perf_config__refresh(void);
>