Re: [PATCH 4/6] perf config: Add support for writing configs to a config file

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Nov 04, 2016 at 03:44:20PM +0900, Taeung Song escreveu:
> 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

This works, but has some problems, see below:
 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-config.c | 66 ++++++++++++++++++++++++++++++++++++++++-----
>  tools/perf/util/config.c    |  6 +++++
>  tools/perf/util/config.h    |  2 ++
>  3 files changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index fe253f3..5313702 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,37 @@ 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 = fopen(file_name, "w");
> +
> +	if (!fp)
> +		return -1;
> +	if (set == NULL)
> +		return -1;

So, here fp is left open? I'm fixing this...

> +	perf_config_set__collect(set, var, value);
> +	fprintf(fp, "%s\n", first_line);
> +
> +	/* overwrite configvariables */
                         missing space?
> +	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 +113,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 +130,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 +198,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 +207,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 18dae74..c8fb65d 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 6f813d4..0fcdb8c 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);
> -- 
> 2.7.4