From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752137AbbG0IBn (ORCPT <rfc822;w@1wt.eu>);
	Mon, 27 Jul 2015 04:01:43 -0400
Received: from mail-pa0-f41.google.com ([209.85.220.41]:34878 "EHLO
	mail-pa0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751388AbbG0IBk (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 27 Jul 2015 04:01:40 -0400
Date: Mon, 27 Jul 2015 16:59:12 +0900
From: Namhyung Kim <namhyung@kernel.org>
To: Taeung Song <treeze.taeung@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, linux-kernel@vger.kernel.org,
        jolsa@redhat.com, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH v4 1/5] perf tools: Add 'perf-config' command
Message-ID: <20150727075912.GA22022@danjae.kornet>
References: <1437926311-16226-1-git-send-email-treeze.taeung@gmail.com>
 <1437926311-16226-2-git-send-email-treeze.taeung@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1437926311-16226-2-git-send-email-treeze.taeung@gmail.com>
User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.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;
> +}