Re: [PATCH 1/2] perf tools: Add 'perf-config' command

[Namhyung Kim <namhyung@kernel.org>]

Hi Taeung,

On Sun, Apr 12, 2015 at 11:44:48PM +0900, Taeung Song wrote:
> The perf configuration file contains many variables which can make
> the perf command's action more effective and more skilful.
> But looking through state of configuration is difficult and
> there's no knowing what kind of other variables except variables in perfconfig.example exist.
> So This patch adds 'perf-config' command with '--all' option and a document for it.
> 
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---

Thanks for your work!

I think the documentation updates need to be shared with tools
manpages, or at least they can point to this document for details.
This can be further work though. ;-)


[SNIP]
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> new file mode 100644
> index 0000000..7cf6d03
> --- /dev/null
> +++ b/tools/perf/builtin-config.c
> @@ -0,0 +1,71 @@
> +/*
> + * builtin-config.c
> + *
> + * Copyright (C) 2015, Taeung Song <treeze.taeung@gmail.com>
> + *
> + */
> +#include "builtin.h"
> +
> +#include "perf.h"
> +
> +#include "util/cache.h"
> +#include "util/parse-options.h"
> +#include "util/util.h"
> +#include "util/debug.h"
> +
> +static struct {
> +	bool all_action;
> +} params;
> +
> +static const char * const config_usage[] = {
> +	"perf config [options]",
> +	NULL
> +};
> +static const struct option config_options[] = {
> +	OPT_GROUP("Action"),
> +	OPT_BOOLEAN('a', "all", &params.all_action, "print all configurations"),
> +	OPT_END()
> +};
> +
> +static void check_argc(int argc, int limit)
> +{
> +	if (argc >= limit && argc <= limit)
> +		return;
> +	error("wrong number of arguments");
> +	usage_with_options(config_usage, config_options);
> +}

I don't know why this is needed.  The -a option won't need this and we
can support to get/set any number of config items IMHO.


> +
> +static int show_config(const char *key, const char *value,
> +			   void *cb __maybe_unused)
> +{
> +	if (value)
> +		printf("%s=%s\n", key, value);
> +	else
> +		printf("%s\n", key);
> +
> +	return 0;
> +}
> +
> +int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
> +{
> +	int ret = 0;
> +
> +	argc = parse_options(argc, argv, config_options, config_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (argc > 0) {
> +		if (strcmp(argv[0], "-") == 0) {
> +			pr_warning("  Error: '-' is not supported.\n");

I don't understand this.  Why is the "-" so special?

And I think current behavior don't allow any argument.  So what about this?

	argc = parse_options(...);
	if (argc)
		usage_with_options(...);

	if (params.all_action)
		...


> +			usage_with_options(config_usage, config_options);
> +		}
> +	}
> +
> +	if (argc == 0 || params.all_action) {
> +		check_argc(argc, 0);
> +		ret = perf_config(show_config, NULL);

But doesn't it just print currently set items in a config file?  I
guess -a/--all option should show this *AND* all possible config items
with default values.  Current behavior can be done by -l/--list option
like git-config..

Thanks,
Namhyung


> +	} else {
> +		pr_warning("Error: Unknown argument.\n");
> +		usage_with_options(config_usage, config_options);
> +	}
> +
> +	return ret;
> +}
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index 3688ad2..3f871b5 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -17,6 +17,7 @@ extern int cmd_annotate(int argc, const char **argv, const char *prefix);
>  extern int cmd_bench(int argc, const char **argv, const char *prefix);
>  extern int cmd_buildid_cache(int argc, const char **argv, const char *prefix);
>  extern int cmd_buildid_list(int argc, const char **argv, const char *prefix);
> +extern int cmd_config(int argc, const char **argv, const char *prefix);
>  extern int cmd_diff(int argc, const char **argv, const char *prefix);
>  extern int cmd_evlist(int argc, const char **argv, const char *prefix);
>  extern int cmd_help(int argc, const char **argv, const char *prefix);
> diff --git a/tools/perf/command-list.txt b/tools/perf/command-list.txt
> index 00fcaf8..acc3ea7 100644
> --- a/tools/perf/command-list.txt
> +++ b/tools/perf/command-list.txt
> @@ -9,6 +9,7 @@ perf-buildid-cache		mainporcelain common
>  perf-buildid-list		mainporcelain common
>  perf-data			mainporcelain common
>  perf-diff			mainporcelain common
> +perf-config			mainporcelain common
>  perf-evlist			mainporcelain common
>  perf-inject			mainporcelain common
>  perf-kmem			mainporcelain common
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index b857fcb..604fa4a 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -37,6 +37,7 @@ struct cmd_struct {
>  static struct cmd_struct commands[] = {
>  	{ "buildid-cache", cmd_buildid_cache, 0 },
>  	{ "buildid-list", cmd_buildid_list, 0 },
> +	{ "config",	cmd_config,	0 },
>  	{ "diff",	cmd_diff,	0 },
>  	{ "evlist",	cmd_evlist,	0 },
>  	{ "help",	cmd_help,	0 },
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/