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

[Namhyung Kim <namhyung@kernel.org>]

Hi Taeung,

On Mon, Jul 27, 2015 at 12:58:27AM +0900, Taeung Song wrote:
> The perf configuration file contains many variables which can make
> the perf command's action more effective.
> 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 '--list' option and a document for it.
> 
>     perf config [options]
> 
>     display current perf config variables.
>     # perf config
>     or
>     # perf config -l | --list
> 
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---

[SNIP]
> +kmem.*::
> +	kmem.default::
> +		This option can decide which allocator is analyzed between 'slab' and 'page'
> +		without using options '--slab' and '--page'.
> +		Default value is 'page'.

The default is 'slab'.


> +
> +SEE ALSO
> +--------
> +linkperf:perf[1], linkperf:perf-report[1]
> +
> +[kmem]
> +	default = page

Ditto.


> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> new file mode 100644
> index 0000000..f0541b8
> --- /dev/null
> +++ b/tools/perf/builtin-config.c
> @@ -0,0 +1,76 @@
> +/*
> + * 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 int actions;
> +
> +static const char * const config_usage[] = {
> +	"perf config [options]",
> +	NULL
> +};
> +
> +#define ACTION_LIST (1<<0)
> +
> +static const struct option config_options[] = {
> +	OPT_GROUP("Action"),
> +	OPT_BIT('l', "list", &actions,
> +		"show current config variables", ACTION_LIST),
> +	OPT_END()
> +};
> +
> +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;
> +	int origin_argc = argc - 1;
> +	bool has_option;

Why are these needed?


> +
> +	argc = parse_options(argc, argv, config_options, config_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (origin_argc > argc)
> +		has_option = true;
> +	else
> +		has_option = false;
> +
> +	switch (actions) {
> +	case ACTION_LIST:
> +		if (argc == 0)
> +			ret = perf_config(show_config, NULL);
> +		else
> +			goto out_err;
> +		goto out;
> +	default:
> +		if (!has_option && argc == 0) {
> +			ret = perf_config(show_config, NULL);
> +			goto out;
> +		} else
> +			goto out_err;
> +	}

Why not simply doing below instead?

	case ACTION_LIST:
	default:
		if (argc)
			error ...;
		perf_config(show_config, ...);
		break;

Thanks,
Namhyung


> +
> +out_err:
> +	pr_err("Error: Unknown argument\n");
> +	usage_with_options(config_usage, config_options);
> +out:
> +	return ret;
> +}