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

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

[Taeung Song]

So far, it is difficult that the state of perf configs is looked through
and there's no knowing what kind of other variables except variables in perfconfig.example.
Also perf configs can't be changed without manually modifying $HOME/.perfconfig or
$(sysconfdir)/perfconfig file. So I suggest this patchset of the perf-config command
which can list, get, set, remove perf configs or list with default config values as below.

[PATCH v4 1/5] perf tools: Add 'perf-config' command
[PATCH v4 2/5] perf config: Add '--system' and '--global' options to
       	       	    	    select which config file to be used
[PATCH v4 3/5] perf config: Add functions which can get or set perf config variables
[PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config
[PATCH v4 5/5] perf config: Add a option 'remove' to perf-config

Changes in v4:
	- If some config value is default value, notice it is '(default)'
	as suggested by Jirka. (PATCH v4 3/5)

	- If there wasn't any perfconfig file, perf-config malfunctioned like
	Jirka pointed out. So add exception routine with '--global' and '--system'
	option which can select perf config file path. (PATCH v4 2/5)

Changes in v3:
	- Add a config variable 'kmem.default' with a default value as suggested by Namhyung.
	(PATCH v3 2/5, PATCH v4 3/5)

Changes in v2:
	- Change option name of listing all configs as '--list-all' instead of '--all'
	as suggested by Namhyung. (PATCH v2 3/4, PATCH v4 4/5)

	- Correct small infelicities or typing errors in a perf-config documention
	as suggested by Arnaldo. (PATCH v2 1/4, PATCH v4 1/5)

	- Declaration a global variable 'static struct default_configsets' has config variables
	with default values instead of using a 'util/PERFCONFIG-DEFAULT' file.
	(PATCH v2 3/4, PATCH v4 3/5)

	- Add a function to normalize a value and check data type of it.
	(PATCH v2 2/4, PATCH v4 3/5)

	- Simplify parsing arguments as arguments is just divided by '=' and then
	in front of '.' is a section, between '.' and '=' is a name, and behind '=' is a value.
	(PATCH v2 2/4, PATCH v4 3/5)

	- If run perf-config command without any option, perf-config work as '--list'.
	(PATCH v2 1/4, PATCH v4 1/5)

---
 tools/perf/Build                            |   1 +
 tools/perf/Documentation/perf-config.txt    | 407 ++++++++++++++++
 tools/perf/Documentation/perfconfig.example |  73 ++-
 tools/perf/builtin-config.c                 | 718 ++++++++++++++++++++++++++++
 tools/perf/builtin.h                        |   1 +
 tools/perf/command-list.txt                 |   1 +
 tools/perf/perf.c                           |   1 +
 tools/perf/util/cache.h                     |  19 +
 tools/perf/util/config.c                    |  29 +-
 9 files changed, 1236 insertions(+), 14 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-config.txt
 create mode 100644 tools/perf/builtin-config.c

-- 
1.9.1

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

[Taeung Song]

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>
---
 tools/perf/Build                            |   1 +
 tools/perf/Documentation/perf-config.txt    | 381 ++++++++++++++++++++++++++++
 tools/perf/Documentation/perfconfig.example |  73 +++++-
 tools/perf/builtin-config.c                 |  76 ++++++
 tools/perf/builtin.h                        |   1 +
 tools/perf/command-list.txt                 |   1 +
 tools/perf/perf.c                           |   1 +
 7 files changed, 522 insertions(+), 12 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-config.txt
 create mode 100644 tools/perf/builtin-config.c

diff --git a/tools/perf/Build b/tools/perf/Build
index 7223745..2c7aaf2 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -1,5 +1,6 @@
 perf-y += builtin-bench.o
 perf-y += builtin-annotate.o
+perf-y += builtin-config.o
 perf-y += builtin-diff.o
 perf-y += builtin-evlist.o
 perf-y += builtin-help.o
diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
new file mode 100644
index 0000000..1a339ea
--- /dev/null
+++ b/tools/perf/Documentation/perf-config.txt
@@ -0,0 +1,381 @@
+perf-config(1)
+==============
+
+NAME
+----
+perf-config - Get and set variables in a configuration file.
+
+SYNOPSIS
+--------
+[verse]
+'perf config' -l | --list
+
+DESCRIPTION
+-----------
+You can manage variables in a configuration file with this command.
+
+OPTIONS
+-------
+
+-l::
+--list::
+	Show current config variables with key and value into each sections.
+
+CONFIGURATION FILE
+------------------
+
+The Perf configuration file contains many variables which can make
+the perf command's action more effective.
+The '$HOME/.perfconfig' file is used to store a per-user configuration.
+The file '$(sysconfdir)/perfconfig' can be used to
+store a system-wide default configuration.
+
+The variables are divided into sections. In each section, the variables
+can are composed of a key and value.
+
+Syntax
+~~~~~~
+
+The file consists of sections and names. A section begins with
+the name of the section in square brackets and continues until the next
+section begins. Each variable have to belong to some section, which means
+there must be a section header before the first setting of a variable, as below:
+Each variable are in the form 'name = value'.
+
+	[section]
+		name1 = value1
+		name2 = value2
+
+Section names are case sensitive and can contain any characters except
+newline (doublequote `"` and backslash have to be escaped as `\"` and `\\`,
+respectively).  Section headers can't span multiple
+lines.  Variables may belong directly to a section.
+
+Example
+~~~~~~~
+
+Given a $HOME/.perfconfig like this:
+
+#
+# This is the config file, and
+# a '#' and ';' character indicates a comment
+#
+
+[colors]
+	# Color variables
+	top = red, default
+	medium = green, default
+	normal = lightgray, default
+	selected = white, lightgray
+	code = blue, default
+	addr = magenta, default
+	root = white, blue
+
+[tui]
+	# Defaults if linked with libslang
+	report = on
+	annotate = on
+	top = on
+
+[buildid]
+	# Default, disable using /dev/null
+	dir = ~/.debug
+
+[annotate]
+	# Defaults
+	hide_src_code = false
+	use_offset = true
+	jump_arrows = true
+	show_nr_jumps = false
+
+[help]
+	# Format can be man, info, web or html
+	format = man
+	autocorrect = 0
+
+[ui]
+	show-headers= true
+
+[call-graph]
+	# fp (framepointer), dwarf
+	record-mode = fp
+	print-type = graph
+	order = caller
+	sort-key = function
+
+Variables
+~~~~~~~~~
+
+colors.*::
+	Color variables can customize colors of the output which is printed out
+	from ‘report’, ‘top’, ’annotate’ on tui.
+	Color variables are composed of foreground and background
+	and should have two values for them. If you want to keep as colors
+	of your terminal, you should use ‘default’ for the color value.
+	The color names that can be used are:
+	red, green, default, black, blue, white, magenta, lightgray
+
+	colors.top::
+		‘top’ means a overhead percentage which is more than 5%.
+		And values of this variable specify colors of percentage.
+		Basic key values are foreground-color ’red’ and
+		background-color ’default’.
+	colors.medium::
+		‘medium’ means a overhead percentage which has more than 0.5%.
+		Default values are ’green’ and ’default’.
+	colors.normal::
+		‘normal’ means the rest of overhead percentages
+		except ‘top’, ‘medium’, ‘selected’.
+		Default values are ’lightgray’ and ’default’.
+	colors.selected::
+		This selects the colors for the current entry in a list of entries
+		from sub-commands (top,report,annotate).
+		Default values are ’white’ and ’lightgray’.
+	colors.code::
+		Colors for arrows and lines in jumps on  assembly code listings
+		such as ‘jns’,’jmp’,’jane’,etc. Default values are ‘blue’, ‘default’.
+	colors.addr::
+		This selects colors for addresses from ’annotate’.
+		Default values are ‘magenta’, ‘default’.
+	colors.root::
+		Colors for headers in the output of a sub-command ‘top’.
+		Default values are ‘white’, ‘blue’.
+
+tui.*::
+	A boolean value that controls launching TUI browser for each subcommand.
+	By default, TUI is enabled if perf detects the required library during build
+	and this config option can control it.  Available subcommands are 'top',
+	'report' and 'annotate'.
+
+gtk.*::
+	A boolean value that controls launching GTK+2 GUI browser for
+	each subcommand.  By default, TUI is enabled if perf detects the
+	required library during build and this config option can control it.
+	Available subcommands are 'top', 'report' and 'annotate'.
+
+buildid.*::
+	buildid.dir::
+		Each executable or shared library built with each program is assigned
+		a unique identification as build-id. The option means a path where
+		build-id information can be saved.
+		The default is $HOME/.debug
+
+annotate.*::
+	There’re options which work with a ’annotate’ sub-command.
+	This options is in control of addresses, jump function, source code
+	in lines of assembly code from a specific program.
+
+	annotate.hide_src_code::
+		If a program which is analyzed has source code of itself,
+		this option let ‘annotate’ print a list of assembly code with the source code.
+		For example, let’s see a part of a program. There’re four lines.
+		If this option is ‘false’, they can be printed
+		without source code from a program as below.
+
+		│        push   %rbp
+		│        mov    %rsp,%rbp
+		│        sub    $0x10,%rsp
+		│        mov    (%rdi),%rdx
+
+		But if this option is ‘true’, source code of the part
+		can be also printed as below.
+
+		│      struct rb_node *rb_next(const struct rb_node *node)
+		│      {
+		│        push   %rbp
+		│        mov    %rsp,%rbp
+		│        sub    $0x10,%rsp
+		│              struct rb_node *parent;
+		│
+		│              if (RB_EMPTY_NODE(node))
+		│        mov    (%rdi),%rdx
+		│              return n;
+
+        annotate.use_offset::
+		Basing on a first address of a loaded function, offset can be used.
+		Instead of using original addresses of assembly code,
+		addresses subtracted from a base address can be printed.
+		Let’s illustrate a example.
+		If a base address is 0XFFFFFFFF81624d50 as below,
+
+		ffffffff81624d50 <load0>
+
+		a address on assembly code has a specific absolute address as below
+
+		ffffffff816250b8:│  mov    0x8(%r14),%rdi
+
+		but if use_offset is ’true’, a address subtracted from a base address is printed.
+		The default is true.
+
+		             368:│  mov    0x8(%r14),%rdi
+
+	annotate.jump_arrows::
+		There’re jump instruction among assembly code.
+		Depending on a boolean value of jump_arrows,
+		arrows can be printed or not which represent
+		where do the instruction jump into as below.
+
+		│     ┌──jmp    1333
+		│     │  xchg   %ax,%ax
+		│1330:│  mov    %r15,%r10
+		│1333:└─→cmp    %r15,%r14
+
+		If jump_arrow is ‘false’, the arrows isn’t printed as below.
+
+		│      ↓ jmp    1333
+		│        xchg   %ax,%ax
+		│1330:   mov    %r15,%r10
+		│1333:   cmp    %r15,%r14
+
+        annotate.show_nr_jumps::
+		Let’s see a part of assembly code.
+
+		│1382:   movb   $0x1,-0x270(%rbp)
+
+		If use this, the number of branches branching to that address can be printed as below.
+
+		│1 1382:   movb   $0x1,-0x270(%rbp)
+
+help.*::
+	help.format:: = man
+		A format of manual page can be ‘man’, ‘info’, ‘web’ or ‘html’.
+		’man’ is default.
+	help.autocorrect:: = 0
+		Automatically correct and execute mistyped commands after
+		waiting for the given number of deciseconds (0.1 sec).
+		Let's see a example. If a mistyped sub-command is executed like 'perf mistyped-command'
+		and this option is 0, the output is as below.
+
+		perf: 'mistyped-command' is not a perf-command. See 'perf --help’.
+
+		Or if this option is more than 1, the output can be such as.
+
+		WARNING: You called a perf program named 'mistyped-command', which does not exist.
+		Continuing under the assumption that you meant 'with-kcore'
+		in 0.1 seconds automatically...
+		Usage: perf-with-kcore <perf sub-command> <perf.data directory> [<sub-command options> [ -- <workload>]]
+		<perf sub-command> can be record, script, report or inject
+		    or: perf-with-kcore fix_buildid_cache_permissions
+
+hist.*::
+	hist.percentage::
+		This option control a way to calcurate overhead of filtered entries -
+		that means the value of this option is effective only if there's a
+		filter (by comm, dso or symbol name).  Suppose a following example:
+
+		       Overhead  Symbols
+		       ........  .......
+		        33.33%     foo
+		        33.33%     bar
+		        33.33%     baz
+
+	       This is an original overhead and we'll filter out the first 'foo'
+	       entry.  The value of 'relative' would increase the overhead of 'bar'
+	       and 'baz' to 50.00% for each, while 'absolute' would show their
+	       current overhead (33.33%).
+
+ui.*::
+	ui.show-headers::
+		There’re columns as header ‘Overhead’, ‘Children’, ‘Shared Object’, ‘Symbol’, ’self’.
+		If this option is false, they are hiden.
+
+call-graph.*::
+	When sub-commands ‘top’ and ‘report’ work with -g/—-children
+	there’re options in control of call-graph.
+
+	call-graph.record-mode::
+		The record-mode can be ‘fp’ (frame pointer) and ‘dwarf’.
+		The value of 'dwarf' is effective only if perf detect needed library
+		(libunwind or a recent version of libdw).  Also it doesn't *require*
+		the dump-size option since it can use the default value of 8192 if
+		missing.
+
+	call-graph.dump-size::
+		The size of stack to dump in order to do post-unwinding.  Default is 8192 (byte).
+		When using dwarf into record-mode this option should have a value.
+
+	call-graph.print-type::
+		The print-types can be graph (graph absolute), fractal (graph relative), flat.
+		This option controls a way to show overhead for each callchain entry.
+		Suppose a following example.
+
+		Overhead  Symbols
+		........  .......
+		  40.00%  foo
+		      |
+		      --- foo
+		      |
+		      |--50.00%-- bar
+		      |           main
+		      |
+		      --50.00%-- baz
+		                 main
+
+		This output is a default format which is 'fractal'.  The 'foo' came
+		from 'bar' and 'baz' exactly half and half so 'fractal' shows 50.00%
+		for each (meaning that it assumes 100% total overhead of 'foo').
+
+		The 'graph' uses absolute overhead value of 'foo' as total so each of
+		'bar' and 'baz' callchain will have 20.00% of overhead.
+
+	call-graph.order::
+		This option controls print order of callchains.  The default is
+		'callee' which means callee is printed at top and then followed by its
+		caller and so on.  The 'caller' prints it in reverse order.
+
+	call-graph.sort-key::
+		The callchains are merged if they contain same information.
+		The sort-key option determines a way to compare the callchains.
+		A value of 'sort-key' can be 'function' or 'address’.
+		The default is ‘function’.
+
+	call-graph.threshold::
+		When there're many callchains it'd print tons of lines.  So perf omits
+		small callchains under a certain overhead (threshold) and this option
+		control the threashold.  Default is 0.5 (%).
+
+	call-graph.print-limit::
+		This is another way to control the number of callchains printed for a
+		single entry.  Default is 0 which means no limitation.
+
+report.*::
+	report.percent-limit::
+		This one is mostly same as call-graph.threshold but works for
+		histogram entries.  Entries have overhead lower than this percentage
+		will not be printed.  Default is 0.
+		If percent-limit is 70, the output which has percentages of
+		each overhead above 70% can be printed.
+
+	report.queue-size::
+		option to setup the maximum allocation size for session's
+		ordered events queue, if not set there's no default limit.
+
+	report.children::
+		The children means that functions called from another function.
+		If the option is true, accumulate callchain of children and show total overhead.
+		Please refer to the perf-report manual.
+
+top.*::
+	top.children::
+		This option means same as report.children.
+		So it is true, the output of ‘top’ is rearranged by each overhead into children group.
+
+man.*::
+	man.viewer::
+		This option can assign a manual tool with which a subcommand 'help' work.
+		it can used as 'man', 'woman', 'konqueror'. Default value is 'man'.
+
+pager.*::
+	pager.<subcommand>::
+		When a subcommand work as stdio instead of TUI, use pager with it.
+		Default value is 'true'.
+
+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'.
+
+SEE ALSO
+--------
+linkperf:perf[1], linkperf:perf-report[1]
diff --git a/tools/perf/Documentation/perfconfig.example b/tools/perf/Documentation/perfconfig.example
index 767ea24..f6889ce 100644
--- a/tools/perf/Documentation/perfconfig.example
+++ b/tools/perf/Documentation/perfconfig.example
@@ -1,29 +1,78 @@
 [colors]
-
-	# These were the old defaults
-	top = red, lightgray
-	medium = green, lightgray
-	normal = black, lightgray
-	selected = lightgray, magenta
-	code = blue, lightgray
-	addr = magenta, lightgray
+	# There are types of colors which are red,
+	# green, default, black, blue,
+	# white, magenta, lightgray
+	# The default is like below
+	top = red, default
+	medium = green, default
+	normal = lightgray, default
+	selected = white, lightgray
+	code = blue, default
+	addr = magenta, default
+	root = white, blue
 
 [tui]
-
 	# Defaults if linked with libslang
 	report = on
 	annotate = on
 	top = on
 
 [buildid]
-
 	# Default, disable using /dev/null
-	dir = /root/.debug
+	dir = ~/.debug
 
 [annotate]
-
 	# Defaults
 	hide_src_code = false
 	use_offset = true
 	jump_arrows = true
 	show_nr_jumps = false
+
+[gtk]
+	report = off
+	annotate = off
+	#top = off
+
+[pager]
+	# That a 'cmd' is true mean to use "pager or less"
+	cmd = true
+	report = false
+	diff = true
+
+[help]
+	# Format can be man, info, web or html
+	format = man
+	autocorrect = 0
+
+[hist]
+	# a value of 'percentage' can be 'relative' or 'absolute'
+	percentage = absolute
+
+[ui]
+	show-headers= true
+
+[call-graph]
+	# fp (framepointer), dwarf
+	record-mode = fp
+
+	# graph (graph absolute), flat, fractal (graph relative)
+	print-type = fractal
+
+	# caller, callee
+	order = caller
+
+	# function, address
+	sort-key = function
+
+[report]
+	percent-limit = 1
+	children = false
+
+[top]
+	children = true
+
+[man]
+	viewer = man
+
+[kmem]
+	default = page
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;
+
+	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;
+	}
+
+out_err:
+	pr_err("Error: Unknown argument\n");
+	usage_with_options(config_usage, config_options);
+out:
+	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

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

[Namhyung Kim]

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;
> +}

[PATCH v4 2/5] perf config: Add '--system' and '--global' options to select which config file is used

[Taeung Song]

Which config file is used is decided in only perf_config().
And a perf-confg command depend on perf_config()
getting config file path. So add '--system' and '--global' options
to select which config file to be used without perf_config().
If file-options isn't used, default config file path is
$HOME/.perfconfig.

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/Documentation/perf-config.txt | 14 +++++++++++++-
 tools/perf/builtin-config.c              | 21 ++++++++++++++++++---
 tools/perf/util/cache.h                  |  2 ++
 tools/perf/util/config.c                 |  4 ++--
 4 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index 1a339ea..f1e50b3 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' -l | --list
+'perf config' [<file-option>] -l | --list
 
 DESCRIPTION
 -----------
@@ -21,6 +21,14 @@ OPTIONS
 --list::
 	Show current config variables with key and value into each sections.
 
+--global::
+	For writing and reading options: write to global
+	'$HOME/.perfconfig' file or read it.
+
+--system::
+	For writing and reading options: write to system-wide
+	'$(sysconfdir)/perfconfig' or read it.
+
 CONFIGURATION FILE
 ------------------
 
@@ -33,6 +41,10 @@ store a system-wide default configuration.
 The variables are divided into sections. In each section, the variables
 can are composed of a key and value.
 
+When reading or writing, the values are read from the system and  global
+configuration files by default, and options '--system' and '--global'
+can be used to tell the command to read from or write to only that location.
+
 Syntax
 ~~~~~~
 
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index f0541b8..27609bd 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -14,6 +14,8 @@
 #include "util/debug.h"
 
 static int actions;
+static bool use_system_config, use_global_config;
+static const char *config_file_name;
 
 static const char * const config_usage[] = {
 	"perf config [options]",
@@ -23,6 +25,9 @@ static const char * const config_usage[] = {
 #define ACTION_LIST (1<<0)
 
 static const struct option config_options[] = {
+	OPT_GROUP("Config file location"),
+	OPT_BOOLEAN(0, "system", &use_system_config, "use system config file"),
+	OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
 	OPT_GROUP("Action"),
 	OPT_BIT('l', "list", &actions,
 		"show current config variables", ACTION_LIST),
@@ -53,16 +58,26 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	else
 		has_option = false;
 
+	if (use_system_config && use_global_config) {
+		pr_err("Error: only one config file at a time\n");
+		usage_with_options(config_usage, config_options);
+		return -1;
+	} else if (use_global_config || (!use_system_config && !use_global_config))
+		config_file_name = mkpath("%s/.perfconfig", getenv("HOME"));
+	else if (use_system_config)
+		config_file_name = perf_etc_perfconfig();
+
 	switch (actions) {
 	case ACTION_LIST:
 		if (argc == 0)
-			ret = perf_config(show_config, NULL);
+			ret = perf_config_from_file(show_config, config_file_name, NULL);
 		else
 			goto out_err;
 		goto out;
 	default:
-		if (!has_option && argc == 0) {
-			ret = perf_config(show_config, NULL);
+		if ((!has_option || use_global_config || use_system_config)
+		    && argc == 0) {
+			ret = perf_config_from_file(show_config, config_file_name, NULL);
 			goto out;
 		} else
 			goto out_err;
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index c861373..bad3e4e 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -22,11 +22,13 @@
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int perf_default_config(const char *, const char *, void *);
 extern int perf_config(config_fn_t fn, void *);
+extern int perf_config_from_file(config_fn_t fn, const char *filename, void *data);
 extern int perf_config_int(const char *, const char *);
 extern u64 perf_config_u64(const char *, const char *);
 extern int perf_config_bool(const char *, const char *);
 extern int config_error_nonbool(const char *);
 extern const char *perf_config_dirname(const char *, const char *);
+extern const char *perf_etc_perfconfig(void);
 
 /* pager.c */
 extern void setup_pager(void);
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index e18f653..1a9862f 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -412,7 +412,7 @@ int perf_default_config(const char *var, const char *value,
 	return 0;
 }
 
-static int perf_config_from_file(config_fn_t fn, const char *filename, void *data)
+int perf_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
 	int ret;
 	FILE *f = fopen(filename, "r");
@@ -430,7 +430,7 @@ static int perf_config_from_file(config_fn_t fn, const char *filename, void *dat
 	return ret;
 }
 
-static const char *perf_etc_perfconfig(void)
+const char *perf_etc_perfconfig(void)
 {
 	static const char *system_wide;
 	if (!system_wide)
-- 
1.9.1

Re: [PATCH v4 2/5] perf config: Add '--system' and '--global' options to select which config file is used

[Namhyung Kim]

On Mon, Jul 27, 2015 at 12:58:28AM +0900, Taeung Song wrote:
> Which config file is used is decided in only perf_config().
> And a perf-confg command depend on perf_config()
> getting config file path. So add '--system' and '--global' options
> to select which config file to be used without perf_config().
> If file-options isn't used, default config file path is
> $HOME/.perfconfig.

I guess you borrowed the names from git config.  But IMHO perf is
different since we don't have the 'local' (repository) concept.  I
think we should use either system/user or global/local naming scheme.

Thanks,
Namhyung


> 
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/Documentation/perf-config.txt | 14 +++++++++++++-
>  tools/perf/builtin-config.c              | 21 ++++++++++++++++++---
>  tools/perf/util/cache.h                  |  2 ++
>  tools/perf/util/config.c                 |  4 ++--
>  4 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index 1a339ea..f1e50b3 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' -l | --list
> +'perf config' [<file-option>] -l | --list
>  
>  DESCRIPTION
>  -----------
> @@ -21,6 +21,14 @@ OPTIONS
>  --list::
>  	Show current config variables with key and value into each sections.
>  
> +--global::
> +	For writing and reading options: write to global
> +	'$HOME/.perfconfig' file or read it.
> +
> +--system::
> +	For writing and reading options: write to system-wide
> +	'$(sysconfdir)/perfconfig' or read it.
> +
>  CONFIGURATION FILE
>  ------------------
>  
> @@ -33,6 +41,10 @@ store a system-wide default configuration.
>  The variables are divided into sections. In each section, the variables
>  can are composed of a key and value.
>  
> +When reading or writing, the values are read from the system and  global
> +configuration files by default, and options '--system' and '--global'
> +can be used to tell the command to read from or write to only that location.
> +
>  Syntax
>  ~~~~~~
>  
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index f0541b8..27609bd 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -14,6 +14,8 @@
>  #include "util/debug.h"
>  
>  static int actions;
> +static bool use_system_config, use_global_config;
> +static const char *config_file_name;
>  
>  static const char * const config_usage[] = {
>  	"perf config [options]",
> @@ -23,6 +25,9 @@ static const char * const config_usage[] = {
>  #define ACTION_LIST (1<<0)
>  
>  static const struct option config_options[] = {
> +	OPT_GROUP("Config file location"),
> +	OPT_BOOLEAN(0, "system", &use_system_config, "use system config file"),
> +	OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
>  	OPT_GROUP("Action"),
>  	OPT_BIT('l', "list", &actions,
>  		"show current config variables", ACTION_LIST),
> @@ -53,16 +58,26 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  	else
>  		has_option = false;
>  
> +	if (use_system_config && use_global_config) {
> +		pr_err("Error: only one config file at a time\n");
> +		usage_with_options(config_usage, config_options);
> +		return -1;
> +	} else if (use_global_config || (!use_system_config && !use_global_config))
> +		config_file_name = mkpath("%s/.perfconfig", getenv("HOME"));
> +	else if (use_system_config)
> +		config_file_name = perf_etc_perfconfig();
> +
>  	switch (actions) {
>  	case ACTION_LIST:
>  		if (argc == 0)
> -			ret = perf_config(show_config, NULL);
> +			ret = perf_config_from_file(show_config, config_file_name, NULL);
>  		else
>  			goto out_err;
>  		goto out;
>  	default:
> -		if (!has_option && argc == 0) {
> -			ret = perf_config(show_config, NULL);
> +		if ((!has_option || use_global_config || use_system_config)
> +		    && argc == 0) {
> +			ret = perf_config_from_file(show_config, config_file_name, NULL);
>  			goto out;
>  		} else
>  			goto out_err;
> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
> index c861373..bad3e4e 100644
> --- a/tools/perf/util/cache.h
> +++ b/tools/perf/util/cache.h
> @@ -22,11 +22,13 @@
>  typedef int (*config_fn_t)(const char *, const char *, void *);
>  extern int perf_default_config(const char *, const char *, void *);
>  extern int perf_config(config_fn_t fn, void *);
> +extern int perf_config_from_file(config_fn_t fn, const char *filename, void *data);
>  extern int perf_config_int(const char *, const char *);
>  extern u64 perf_config_u64(const char *, const char *);
>  extern int perf_config_bool(const char *, const char *);
>  extern int config_error_nonbool(const char *);
>  extern const char *perf_config_dirname(const char *, const char *);
> +extern const char *perf_etc_perfconfig(void);
>  
>  /* pager.c */
>  extern void setup_pager(void);
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index e18f653..1a9862f 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -412,7 +412,7 @@ int perf_default_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> -static int perf_config_from_file(config_fn_t fn, const char *filename, void *data)
> +int perf_config_from_file(config_fn_t fn, const char *filename, void *data)
>  {
>  	int ret;
>  	FILE *f = fopen(filename, "r");
> @@ -430,7 +430,7 @@ static int perf_config_from_file(config_fn_t fn, const char *filename, void *dat
>  	return ret;
>  }
>  
> -static const char *perf_etc_perfconfig(void)
> +const char *perf_etc_perfconfig(void)
>  {
>  	static const char *system_wide;
>  	if (!system_wide)
> -- 
> 1.9.1
> 

[PATCH v4 3/5] perf config: Add functions which can get or set perf config variables

[Taeung Song]

This patch consists of functions
which can get, set specific config variables.
For the syntax examples,

   perf config [options] [section.name[=value] ...]

   display key-value pairs of specific config variables
   # perf config report.queue-size report.children

   set specific config variables
   # perf config report.queue-size=100M report.children=true

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/Documentation/perf-config.txt |   2 +
 tools/perf/builtin-config.c              | 560 ++++++++++++++++++++++++++++++-
 tools/perf/util/cache.h                  |  17 +
 tools/perf/util/config.c                 |  25 ++
 4 files changed, 602 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index f1e50b3..cd4b1a6 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -8,6 +8,8 @@ perf-config - Get and set variables in a configuration file.
 SYNOPSIS
 --------
 [verse]
+'perf config' [<file-option>] [section.name[=value] ...]
+or
 'perf config' [<file-option>] -l | --list
 
 DESCRIPTION
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 27609bd..6d9f28c 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -18,7 +18,7 @@ static bool use_system_config, use_global_config;
 static const char *config_file_name;
 
 static const char * const config_usage[] = {
-	"perf config [options]",
+	"perf config [options] [section.name[=value] ...]",
 	NULL
 };
 
@@ -34,6 +34,353 @@ static const struct option config_options[] = {
 	OPT_END()
 };
 
+/* section names */
+#define COLORS "colors"
+#define TUI "tui"
+#define BUILDID "buildid"
+#define ANNOTATE "annotate"
+#define GTK "gtk"
+#define PAGER "pager"
+#define HELP "help"
+#define HIST "hist"
+#define UI "ui"
+#define CALL_GRAPH "call-graph"
+#define REPORT "report"
+#define TOP "top"
+#define MAN "man"
+#define KMEM "kmem"
+
+/* config variable types */
+#define TYPE_INT "int"
+#define TYPE_LONG "long"
+#define TYPE_DIRNAME "dirname"
+#define TYPE_BOOL "bool"
+#define TYPE_ON_OFF "on_off"
+
+static struct default_configset {
+	const char *section_name;
+	const char *name, *value, *type;
+
+} default_configsets[] = {
+	{
+		.section_name = COLORS,
+		.name = "top",
+		.value = "red, default",
+		.type = NULL,
+	},
+	{
+		.section_name = COLORS,
+		.name = "medium",
+		.value = "green, default",
+		.type = NULL,
+	},
+	{
+		.section_name = COLORS,
+		.name = "normal",
+		.value = "lightgray, default",
+		.type = NULL,
+	},
+	{
+		.section_name = COLORS,
+		.name = "selected",
+		.value = "white, lightgray",
+		.type = NULL,
+	},
+	{
+		.section_name = COLORS,
+		.name = "code",
+		.value = "blue, default",
+		.type = NULL,
+	},
+	{
+		.section_name = COLORS,
+		.name = "addr",
+		.value = "magenta, default",
+		.type = NULL,
+	},
+	{
+		.section_name = COLORS,
+		.name = "root",
+		.value = "white, blue",
+		.type = NULL,
+	},
+	{
+		.section_name = TUI,
+		.name = "report",
+		.value = "on",
+		.type = TYPE_ON_OFF,
+	},
+	{
+		.section_name = TUI,
+		.name = "annotate",
+		.value = "on",
+		.type = TYPE_ON_OFF,
+	},
+	{
+		.section_name = TUI,
+		.name = "top",
+		.value = "on",
+		.type = TYPE_ON_OFF,
+	},
+	{
+		.section_name = BUILDID,
+		.name = "dir",
+		.value = "~/.debug",
+		.type = TYPE_DIRNAME,
+	},
+	{
+		.section_name = ANNOTATE,
+		.name = "hide_src_code",
+		.value = "false",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = ANNOTATE,
+		.name = "use_offset",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = ANNOTATE,
+		.name = "jump_arrows",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = ANNOTATE,
+		.name = "show_nr_jumps",
+		.value = "false",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = GTK,
+		.name = "annotate",
+		.value = "off",
+		.type = TYPE_ON_OFF,
+	},
+	{
+		.section_name = GTK,
+		.name = "report",
+		.value = "off",
+		.type = TYPE_ON_OFF,
+	},
+	{
+		.section_name = GTK,
+		.name = "top",
+		.value = "off",
+		.type = TYPE_ON_OFF,
+	},
+	{
+		.section_name = PAGER,
+		.name = "cmd",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = PAGER,
+		.name = "report",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = PAGER,
+		.name = "annotate",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = PAGER,
+		.name = "record",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = PAGER,
+		.name = "top",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = PAGER,
+		.name = "diff",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = HELP,
+		.name = "format",
+		.value = "man",
+		.type = NULL,
+	},
+	{
+		.section_name = HELP,
+		.name = "autocorrect",
+		.value = "0",
+		.type = NULL,
+	},
+	{
+		.section_name = HIST,
+		.name = "percentage",
+		.value = "absolute",
+		.type = NULL,
+	},
+	{
+		.section_name = UI,
+		.name = "show-headers",
+		.value = "true",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "record-mode",
+		.value = "fp",
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "dump-size",
+		.value = "8192",
+		.type = TYPE_INT,
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "print-type",
+		.value = "fractal",
+		.type = NULL,
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "order",
+		.value = "caller",
+		.type = NULL,
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "sort-key",
+		.value = "function",
+		.type = NULL,
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "threshold",
+		.value = "0.5",
+		.type = TYPE_LONG,
+	},
+	{
+		.section_name = CALL_GRAPH,
+		.name = "print-limit",
+		.value = "0",
+		.type = TYPE_INT,
+	},
+	{
+		.section_name = REPORT,
+		.name = "children",
+		.value = "false",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = REPORT,
+		.name = "percent-limit",
+		.value = "0",
+		.type = TYPE_INT,
+	},
+	{
+		.section_name = REPORT,
+		.name = "queue-size",
+		.value = "0",
+		.type = TYPE_INT,
+	},
+	{
+		.section_name = TOP,
+		.name = "children",
+		.value = "false",
+		.type = TYPE_BOOL,
+	},
+	{
+		.section_name = MAN,
+		.name = "viewer",
+		.value = "man",
+		.type = NULL,
+	},
+	{
+		.section_name = KMEM,
+		.name = "default",
+		.value = "page",
+		.type = NULL,
+	},
+	{
+		.section_name = NULL,
+		.name = NULL,
+		.value = NULL,
+		.type = NULL,
+	},
+};
+
+static struct config_section *find_section(const char *section_name)
+{
+	struct config_section *section_node;
+
+	list_for_each_entry(section_node, &sections, list)
+		if (!strcmp(section_node->name, section_name))
+			return section_node;
+
+	return NULL;
+}
+
+static struct config_element *find_element(const char *name,
+					   struct config_section *section_node)
+{
+	struct config_element *element_node;
+
+	list_for_each_entry(element_node, &section_node->element_head, list)
+		if (!strcmp(element_node->name, name))
+			return element_node;
+
+	return NULL;
+}
+
+static struct config_section *init_section(const char *section_name)
+{
+	struct config_section *section_node;
+
+	section_node = zalloc(sizeof(*section_node));
+	if (!section_node)
+		return NULL;
+
+	INIT_LIST_HEAD(&section_node->element_head);
+	section_node->name = strdup(section_name);
+	if (!section_node->name) {
+		pr_err("%s: strdup failed\n", __func__);
+		free(section_node);
+		return NULL;
+	}
+
+	return section_node;
+}
+
+static int add_element(struct list_head *head,
+			      const char *name, const char *value)
+{
+	struct config_element *element_node;
+
+	element_node = zalloc(sizeof(*element_node));
+	element_node->name = strdup(name);
+	if (!element_node->name) {
+		pr_err("%s: strdup failed\n", __func__);
+		goto out_free;
+	}
+	if (value)
+		element_node->value = (char *)value;
+	else
+		element_node->value = NULL;
+
+	list_add_tail(&element_node->list, head);
+	return 0;
+
+out_free:
+	free(element_node);
+	return -1;
+}
+
 static int show_config(const char *key, const char *value,
 		       void *cb __maybe_unused)
 {
@@ -45,10 +392,203 @@ static int show_config(const char *key, const char *value,
 	return 0;
 }
 
+static void find_config(struct config_section **section_node,
+			struct config_element **element_node,
+			const char *section_name, const char *name)
+{
+	*section_node = find_section(section_name);
+
+	if (*section_node != NULL)
+		*element_node = find_element(name, *section_node);
+	else
+		*element_node = NULL;
+}
+
+static int show_spec_config(const char *section_name, const char *name,
+			    char *value __maybe_unused)
+{
+	int i;
+	struct config_section *section_node = NULL;
+	struct config_element *element_node = NULL;
+	char key[BUFSIZ];
+
+	find_config(&section_node, &element_node, section_name, name);
+
+	if (section_node && element_node) {
+		scnprintf(key, sizeof(key), "%s.%s",
+			  section_node->name, element_node->name);
+		return show_config(key, element_node->value, NULL);
+	}
+
+	for (i = 0; default_configsets[i].section_name != NULL; i++) {
+		if (!strcmp(default_configsets[i].section_name, section_name)
+		    && !strcmp(default_configsets[i].name, name)) {
+			printf("%s.%s=%s (default)\n", default_configsets[i].section_name,
+			       default_configsets[i].name, default_configsets[i].value);
+			return 0;
+		}
+	}
+
+	pr_err("Error: Failed to find the variable.\n");
+
+	return 0;
+}
+
+static char *normalize_value(const char *section_name, const char *name, const char *value)
+{
+	int i;
+	char key[BUFSIZ];
+	char *normalized;
+
+	scnprintf(key, sizeof(key), "%s.%s", section_name, name);
+	for (i = 0; default_configsets[i].section_name != NULL; i++) {
+		if (!strcmp(default_configsets[i].section_name, section_name)
+		    && !strcmp(default_configsets[i].name, name)) {
+			normalized = zalloc(BUFSIZ);
+			if (!default_configsets[i].type)
+				scnprintf(normalized, BUFSIZ, "%s", value);
+			else if (!strcmp(default_configsets[i].type, TYPE_BOOL))
+				scnprintf(normalized, BUFSIZ, "%s",
+					  perf_config_bool(key, value) ? "true" : "false");
+			else if (!strcmp(default_configsets[i].type, TYPE_ON_OFF))
+				scnprintf(normalized, BUFSIZ, "%s",
+					  perf_config_bool(key, value) ? "on" : "off");
+			else if (!strcmp(default_configsets[i].type, TYPE_INT))
+				scnprintf(normalized, BUFSIZ, "%d",
+					  perf_config_int(key, value));
+			else if (!strcmp(default_configsets[i].type, TYPE_LONG))
+				scnprintf(normalized, BUFSIZ, "%"PRId64,
+					  perf_config_u64(key, value));
+			else if (!strcmp(default_configsets[i].type, TYPE_DIRNAME))
+				scnprintf(normalized, BUFSIZ, "%s",
+					  perf_config_dirname(key, value));
+			return normalized;
+		}
+	}
+
+	normalized = strdup(value);
+	if (!normalized) {
+		pr_err("%s: strdup failed\n", __func__);
+		return NULL;
+	}
+
+	return normalized;
+}
+
+static int set_config(const char *section_name, const char *name, char *value)
+{
+	struct config_section *section_node = NULL;
+	struct config_element *element_node = NULL;
+
+	find_config(&section_node, &element_node, section_name, name);
+	if (value != NULL) {
+		value = normalize_value(section_name, name, value);
+
+		/* if there isn't existent section, add a new section */
+		if (!section_node) {
+			section_node = init_section(section_name);
+			if (!section_node)
+				return -1;
+			list_add_tail(&section_node->list, &sections);
+		}
+		/* if nothing to replace, add a new element which contains key-value pair. */
+		if (!element_node) {
+			add_element(&section_node->element_head, name, value);
+		} else {
+			if (!element_node->value)
+				free(element_node->value);
+			element_node->value = value;
+		}
+	}
+
+	if (access(config_file_name, W_OK) == -1) {
+		pr_err("Error: %s: Can't write to this file or no such file\n",
+		       config_file_name);
+		return -1;
+	}
+
+	return perf_configset_write_in_full(config_file_name);
+}
+
+static int collect_current_config(const char *var, const char *value,
+			  void *cb __maybe_unused)
+{
+	struct config_section *section_node;
+	char *key = strdup(var);
+	char *section_name, *name;
+
+	if (!key) {
+		pr_err("%s: strdup failed\n", __func__);
+		return -1;
+	}
+
+	section_name = strsep(&key, ".");
+	name = strsep(&key, ".");
+	if (name == NULL)
+		return -1;
+
+	section_node = find_section(section_name);
+	if (!section_node) {
+		section_node = init_section(section_name);
+		if (!section_node)
+			return -1;
+		list_add_tail(&section_node->list, &sections);
+	}
+
+	return add_element(&section_node->element_head, name,
+			   normalize_value(section_name, name, value));
+}
+
+static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
+{
+	char *section_name;
+	char *name;
+	const char *last_dot;
+	char *key = strdup(var);
+
+	if (!key) {
+		pr_err("%s: strdup failed\n", __func__);
+		return -1;
+	}
+	last_dot = strchr(key, '.');
+	/*
+	 * Since "key" actually contains the section name and the real
+	 * key name separated by a dot, we have to know where the dot is.
+	 */
+	if (last_dot == NULL || last_dot == key) {
+		pr_err("The config variable does not contain a section: %s\n", key);
+		return -1;
+	}
+	if (!last_dot[1]) {
+		pr_err("The config varible does not contain variable name: %s\n", key);
+		return -1;
+	}
+
+	section_name = strsep(&key, ".");
+	name = strsep(&key, ".");
+
+	if (!value) {
+		/* do nothing */
+	} else if (!strcmp(value, "=")) {
+		pr_err("The config variable does not contain a value: %s.%s\n",
+		       section_name, name);
+		return -1;
+	} else {
+		value++;
+		name = strsep(&name, "=");
+	}
+
+	return fn(section_name, name, value);
+
+	pr_err("invalid key: %s\n", var);
+	return -1;
+}
+
 int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 {
-	int ret = 0;
+	int i, ret = 0;
 	int origin_argc = argc - 1;
+	char *value;
 	bool has_option;
 
 	argc = parse_options(argc, argv, config_options, config_usage,
@@ -67,6 +607,9 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 	else if (use_system_config)
 		config_file_name = perf_etc_perfconfig();
 
+	INIT_LIST_HEAD(&sections);
+	perf_config_from_file(collect_current_config, config_file_name, NULL);
+
 	switch (actions) {
 	case ACTION_LIST:
 		if (argc == 0)
@@ -79,6 +622,19 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 		    && argc == 0) {
 			ret = perf_config_from_file(show_config, config_file_name, NULL);
 			goto out;
+		} else if (argc > 0) {
+			for (i = 0; argv[i]; i++) {
+				value = strchr(argv[i], '=');
+				if (value == NULL)
+					ret = perf_configset_with_option(show_spec_config,
+									 argv[i], value);
+				else
+					ret = perf_configset_with_option(set_config,
+									 argv[i], value);
+				if (ret < 0)
+					break;
+			}
+			goto out;
 		} else
 			goto out_err;
 	}
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index bad3e4e..d831e3c 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -1,6 +1,7 @@
 #ifndef __PERF_CACHE_H
 #define __PERF_CACHE_H
 
+#include <linux/list.h>
 #include <stdbool.h>
 #include "util.h"
 #include "strbuf.h"
@@ -19,6 +20,22 @@
 #define PERF_DEBUGFS_ENVIRONMENT "PERF_DEBUGFS_DIR"
 #define PERF_TRACEFS_ENVIRONMENT "PERF_TRACEFS_DIR"
 
+struct config_element {
+	char *name;
+	char *value;
+	struct list_head list;
+};
+
+struct config_section {
+	char *name;
+	struct list_head element_head;
+	struct list_head list;
+};
+
+struct list_head sections;
+
+typedef int (*configset_fn_t)(const char *, const char *, char*);
+extern int perf_configset_write_in_full(const char *file_name);
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int perf_default_config(const char *, const char *, void *);
 extern int perf_config(config_fn_t fn, void *);
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 1a9862f..b8ee066 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -502,6 +502,31 @@ out:
 	return ret;
 }
 
+int perf_configset_write_in_full(const char *file_name)
+{
+	struct config_section *section_node;
+	struct config_element *element_node;
+	const char *first_line = "# this file is auto-generated.";
+	FILE *fp = fopen(file_name, "w");
+
+	if (!fp)
+		return -1;
+
+	fprintf(fp, "%s\n", first_line);
+	/* overwrite configvariables */
+	list_for_each_entry(section_node, &sections, list) {
+		fprintf(fp, "[%s]\n", section_node->name);
+		list_for_each_entry(element_node, &section_node->element_head, list) {
+			if (element_node->value)
+				fprintf(fp, "\t%s = %s\n",
+					element_node->name, element_node->value);
+		}
+	}
+	fclose(fp);
+
+	return 0;
+}
+
 /*
  * Call this to report error for your variable that should not
  * get a boolean value (i.e. "[my] var" means "true").
-- 
1.9.1

Re: [PATCH v4 3/5] perf config: Add functions which can get or set perf config variables

[Namhyung Kim]

On Mon, Jul 27, 2015 at 12:58:29AM +0900, Taeung Song wrote:
> This patch consists of functions
> which can get, set specific config variables.

I think you'd be better splitting this patch into pieces.  I'd like to
do it with 3 patches - one for adding default config set (possibly
squash/reorder --list-all patch too), next is for adding 'get'
functionality, and finally adding 'set' feature.  It'll make the
reviewer's life little easier. :)

Nitpicks below...


> For the syntax examples,
> 
>    perf config [options] [section.name[=value] ...]
> 
>    display key-value pairs of specific config variables
>    # perf config report.queue-size report.children
> 
>    set specific config variables
>    # perf config report.queue-size=100M report.children=true
> 
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---


[SNIP]
> +	{
> +		.section_name = KMEM,
> +		.name = "default",
> +		.value = "page",

s/page/slab/


> +		.type = NULL,
> +	},
> +	{
> +		.section_name = NULL,
> +		.name = NULL,
> +		.value = NULL,
> +		.type = NULL,
> +	},
> +};
> +

[SNIP]
> +static int add_element(struct list_head *head,
> +			      const char *name, const char *value)
> +{
> +	struct config_element *element_node;
> +
> +	element_node = zalloc(sizeof(*element_node));

Need to check return value here.


> +	element_node->name = strdup(name);
> +	if (!element_node->name) {
> +		pr_err("%s: strdup failed\n", __func__);
> +		goto out_free;
> +	}
> +	if (value)
> +		element_node->value = (char *)value;
> +	else
> +		element_node->value = NULL;
> +
> +	list_add_tail(&element_node->list, head);
> +	return 0;
> +
> +out_free:
> +	free(element_node);
> +	return -1;
> +}
> +
>  static int show_config(const char *key, const char *value,
>  		       void *cb __maybe_unused)
>  {
> @@ -45,10 +392,203 @@ static int show_config(const char *key, const char *value,
>  	return 0;
>  }
>  
> +static void find_config(struct config_section **section_node,
> +			struct config_element **element_node,
> +			const char *section_name, const char *name)
> +{
> +	*section_node = find_section(section_name);
> +
> +	if (*section_node != NULL)
> +		*element_node = find_element(name, *section_node);
> +	else
> +		*element_node = NULL;
> +}
> +
> +static int show_spec_config(const char *section_name, const char *name,
> +			    char *value __maybe_unused)
> +{
> +	int i;
> +	struct config_section *section_node = NULL;
> +	struct config_element *element_node = NULL;
> +	char key[BUFSIZ];
> +
> +	find_config(&section_node, &element_node, section_name, name);
> +
> +	if (section_node && element_node) {
> +		scnprintf(key, sizeof(key), "%s.%s",
> +			  section_node->name, element_node->name);
> +		return show_config(key, element_node->value, NULL);
> +	}
> +
> +	for (i = 0; default_configsets[i].section_name != NULL; i++) {

I'd do this with a local variable:

		struct default_configset *config = &default_configsets[i];


> +		if (!strcmp(default_configsets[i].section_name, section_name)
> +		    && !strcmp(default_configsets[i].name, name)) {

Please put the '&&' operator in the above line.


> +			printf("%s.%s=%s (default)\n", default_configsets[i].section_name,
> +			       default_configsets[i].name, default_configsets[i].value);
> +			return 0;
> +		}
> +	}
> +
> +	pr_err("Error: Failed to find the variable.\n");
> +
> +	return 0;
> +}
> +
> +static char *normalize_value(const char *section_name, const char *name, const char *value)
> +{
> +	int i;
> +	char key[BUFSIZ];
> +	char *normalized;
> +
> +	scnprintf(key, sizeof(key), "%s.%s", section_name, name);
> +	for (i = 0; default_configsets[i].section_name != NULL; i++) {

Ditto for default_configsets[i].


> +		if (!strcmp(default_configsets[i].section_name, section_name)
> +		    && !strcmp(default_configsets[i].name, name)) {
> +			normalized = zalloc(BUFSIZ);

Instead of always allocating BUFSIZ memory, please use asprintf().


> +			if (!default_configsets[i].type)
> +				scnprintf(normalized, BUFSIZ, "%s", value);
> +			else if (!strcmp(default_configsets[i].type, TYPE_BOOL))
> +				scnprintf(normalized, BUFSIZ, "%s",
> +					  perf_config_bool(key, value) ? "true" : "false");
> +			else if (!strcmp(default_configsets[i].type, TYPE_ON_OFF))
> +				scnprintf(normalized, BUFSIZ, "%s",
> +					  perf_config_bool(key, value) ? "on" : "off");
> +			else if (!strcmp(default_configsets[i].type, TYPE_INT))
> +				scnprintf(normalized, BUFSIZ, "%d",
> +					  perf_config_int(key, value));
> +			else if (!strcmp(default_configsets[i].type, TYPE_LONG))
> +				scnprintf(normalized, BUFSIZ, "%"PRId64,
> +					  perf_config_u64(key, value));
> +			else if (!strcmp(default_configsets[i].type, TYPE_DIRNAME))
> +				scnprintf(normalized, BUFSIZ, "%s",
> +					  perf_config_dirname(key, value));
> +			return normalized;
> +		}
> +	}
> +
> +	normalized = strdup(value);
> +	if (!normalized) {
> +		pr_err("%s: strdup failed\n", __func__);
> +		return NULL;
> +	}
> +
> +	return normalized;
> +}
> +
> +static int set_config(const char *section_name, const char *name, char *value)
> +{
> +	struct config_section *section_node = NULL;
> +	struct config_element *element_node = NULL;
> +
> +	find_config(&section_node, &element_node, section_name, name);
> +	if (value != NULL) {
> +		value = normalize_value(section_name, name, value);
> +
> +		/* if there isn't existent section, add a new section */
> +		if (!section_node) {
> +			section_node = init_section(section_name);
> +			if (!section_node)
> +				return -1;
> +			list_add_tail(&section_node->list, &sections);
> +		}
> +		/* if nothing to replace, add a new element which contains key-value pair. */
> +		if (!element_node) {
> +			add_element(&section_node->element_head, name, value);
> +		} else {
> +			if (!element_node->value)
> +				free(element_node->value);

I guess you wanted to check NULL pointer before freeing the old value.
It's inverted, but anyway you can skip the check since free() already
ignored NULL pointers.


> +			element_node->value = value;
> +		}
> +	}
> +
> +	if (access(config_file_name, W_OK) == -1) {
> +		pr_err("Error: %s: Can't write to this file or no such file\n",
> +		       config_file_name);

Not sure we need to error out in the 'no such file' case.  Maybe it's
ok to create a new file..


> +		return -1;
> +	}
> +
> +	return perf_configset_write_in_full(config_file_name);
> +}
> +
> +static int collect_current_config(const char *var, const char *value,
> +			  void *cb __maybe_unused)
> +{
> +	struct config_section *section_node;
> +	char *key = strdup(var);

Who does free the key?


> +	char *section_name, *name;
> +
> +	if (!key) {
> +		pr_err("%s: strdup failed\n", __func__);
> +		return -1;
> +	}
> +
> +	section_name = strsep(&key, ".");
> +	name = strsep(&key, ".");
> +	if (name == NULL)
> +		return -1;
> +
> +	section_node = find_section(section_name);
> +	if (!section_node) {
> +		section_node = init_section(section_name);
> +		if (!section_node)
> +			return -1;
> +		list_add_tail(&section_node->list, &sections);
> +	}
> +
> +	return add_element(&section_node->element_head, name,
> +			   normalize_value(section_name, name, value));
> +}
> +
> +static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
> +{
> +	char *section_name;
> +	char *name;
> +	const char *last_dot;
> +	char *key = strdup(var);

Ditto.

Thanks,
Namhyung


> +
> +	if (!key) {
> +		pr_err("%s: strdup failed\n", __func__);
> +		return -1;
> +	}
> +	last_dot = strchr(key, '.');
> +	/*
> +	 * Since "key" actually contains the section name and the real
> +	 * key name separated by a dot, we have to know where the dot is.
> +	 */
> +	if (last_dot == NULL || last_dot == key) {
> +		pr_err("The config variable does not contain a section: %s\n", key);
> +		return -1;
> +	}
> +	if (!last_dot[1]) {
> +		pr_err("The config varible does not contain variable name: %s\n", key);
> +		return -1;
> +	}
> +
> +	section_name = strsep(&key, ".");
> +	name = strsep(&key, ".");
> +
> +	if (!value) {
> +		/* do nothing */
> +	} else if (!strcmp(value, "=")) {
> +		pr_err("The config variable does not contain a value: %s.%s\n",
> +		       section_name, name);
> +		return -1;
> +	} else {
> +		value++;
> +		name = strsep(&name, "=");
> +	}
> +
> +	return fn(section_name, name, value);
> +
> +	pr_err("invalid key: %s\n", var);
> +	return -1;
> +}

[PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Taeung Song]

A option 'list-all' is to display both current config variables and
all possible config variables with default values.
The syntax examples are like below

    perf config [options]

    display all perf config with default values.
    # perf config -a | --list-all

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/Documentation/perf-config.txt |  6 ++++
 tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index cd4b1a6..d8b3acc 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -11,6 +11,8 @@ SYNOPSIS
 'perf config' [<file-option>] [section.name[=value] ...]
 or
 'perf config' [<file-option>] -l | --list
+or
+'perf config' [<file-option>] -a | --list-all
 
 DESCRIPTION
 -----------
@@ -31,6 +33,10 @@ OPTIONS
 	For writing and reading options: write to system-wide
 	'$(sysconfdir)/perfconfig' or read it.
 
+-a::
+--list-all::
+	Show current and all possible config variables with default values.
+
 CONFIGURATION FILE
 ------------------
 
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 6d9f28c..f4a1569 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -23,6 +23,7 @@ static const char * const config_usage[] = {
 };
 
 #define ACTION_LIST (1<<0)
+#define ACTION_LIST_ALL (1<<1)
 
 static const struct option config_options[] = {
 	OPT_GROUP("Config file location"),
@@ -31,6 +32,8 @@ static const struct option config_options[] = {
 	OPT_GROUP("Action"),
 	OPT_BIT('l', "list", &actions,
 		"show current config variables", ACTION_LIST),
+	OPT_BIT('a', "list-all", &actions,
+		"show current and all possible config variables with default values", ACTION_LIST_ALL),
 	OPT_END()
 };
 
@@ -539,6 +542,45 @@ static int collect_current_config(const char *var, const char *value,
 			   normalize_value(section_name, name, value));
 }
 
+static int show_all_config(void)
+{
+	int i;
+	bool has_config;
+	struct config_section *section_node;
+	struct config_element *element_node;
+
+	for (i = 0; default_configsets[i].section_name != NULL; i++) {
+		find_config(&section_node, &element_node,
+			    default_configsets[i].section_name, default_configsets[i].name);
+
+		if (!element_node)
+			printf("%s.%s=%s\n", default_configsets[i].section_name,
+			       default_configsets[i].name, default_configsets[i].value);
+		else
+			printf("%s.%s=%s\n", section_node->name,
+			       element_node->name, element_node->value);
+	}
+
+	/* Print config variables the default configsets haven't */
+	list_for_each_entry(section_node, &sections, list) {
+		list_for_each_entry(element_node, &section_node->element_head, list) {
+			has_config = false;
+			for (i = 0; default_configsets[i].section_name != NULL; i++) {
+				if (!strcmp(default_configsets[i].section_name, section_node->name)
+				    && !strcmp(default_configsets[i].name, element_node->name)) {
+					has_config = true;
+					break;
+				}
+			}
+			if (!has_config)
+				printf("%s.%s=%s\n", section_node->name,
+				       element_node->name, element_node->value);
+		}
+	}
+
+	return 0;
+}
+
 static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
 {
 	char *section_name;
@@ -617,6 +659,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 		else
 			goto out_err;
 		goto out;
+	case ACTION_LIST_ALL:
+		if (argc == 0)
+			ret = show_all_config();
+		else
+			goto out_err;
+		goto out;
 	default:
 		if ((!has_option || use_global_config || use_system_config)
 		    && argc == 0) {
-- 
1.9.1

Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Namhyung Kim]

On Mon, Jul 27, 2015 at 12:58:30AM +0900, Taeung Song wrote:
> A option 'list-all' is to display both current config variables and
> all possible config variables with default values.
> The syntax examples are like below
> 
>     perf config [options]
> 
>     display all perf config with default values.
>     # perf config -a | --list-all
> 
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/Documentation/perf-config.txt |  6 ++++
>  tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index cd4b1a6..d8b3acc 100644
> --- a/tools/perf/Documentation/perf-config.txt
> +++ b/tools/perf/Documentation/perf-config.txt
> @@ -11,6 +11,8 @@ SYNOPSIS
>  'perf config' [<file-option>] [section.name[=value] ...]
>  or
>  'perf config' [<file-option>] -l | --list
> +or
> +'perf config' [<file-option>] -a | --list-all
>  
>  DESCRIPTION
>  -----------
> @@ -31,6 +33,10 @@ OPTIONS
>  	For writing and reading options: write to system-wide
>  	'$(sysconfdir)/perfconfig' or read it.
>  
> +-a::
> +--list-all::
> +	Show current and all possible config variables with default values.
> +
>  CONFIGURATION FILE
>  ------------------
>  
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> index 6d9f28c..f4a1569 100644
> --- a/tools/perf/builtin-config.c
> +++ b/tools/perf/builtin-config.c
> @@ -23,6 +23,7 @@ static const char * const config_usage[] = {
>  };
>  
>  #define ACTION_LIST (1<<0)
> +#define ACTION_LIST_ALL (1<<1)
>  
>  static const struct option config_options[] = {
>  	OPT_GROUP("Config file location"),
> @@ -31,6 +32,8 @@ static const struct option config_options[] = {
>  	OPT_GROUP("Action"),
>  	OPT_BIT('l', "list", &actions,
>  		"show current config variables", ACTION_LIST),
> +	OPT_BIT('a', "list-all", &actions,
> +		"show current and all possible config variables with default values", ACTION_LIST_ALL),

Why did you use OPT_BIT?  Do you want to support multiple 'actions' at
the same time?  I'd rather support just one action, but I won't insist
it strongly..  Anyway, setting bits will confuse the switch statement
in the cmd_config().

Thanks,
Namhyung


>  	OPT_END()
>  };
>  
> @@ -539,6 +542,45 @@ static int collect_current_config(const char *var, const char *value,
>  			   normalize_value(section_name, name, value));
>  }
>  
> +static int show_all_config(void)
> +{
> +	int i;
> +	bool has_config;
> +	struct config_section *section_node;
> +	struct config_element *element_node;
> +
> +	for (i = 0; default_configsets[i].section_name != NULL; i++) {
> +		find_config(&section_node, &element_node,
> +			    default_configsets[i].section_name, default_configsets[i].name);
> +
> +		if (!element_node)
> +			printf("%s.%s=%s\n", default_configsets[i].section_name,
> +			       default_configsets[i].name, default_configsets[i].value);
> +		else
> +			printf("%s.%s=%s\n", section_node->name,
> +			       element_node->name, element_node->value);
> +	}
> +
> +	/* Print config variables the default configsets haven't */
> +	list_for_each_entry(section_node, &sections, list) {
> +		list_for_each_entry(element_node, &section_node->element_head, list) {
> +			has_config = false;
> +			for (i = 0; default_configsets[i].section_name != NULL; i++) {
> +				if (!strcmp(default_configsets[i].section_name, section_node->name)
> +				    && !strcmp(default_configsets[i].name, element_node->name)) {
> +					has_config = true;
> +					break;
> +				}
> +			}
> +			if (!has_config)
> +				printf("%s.%s=%s\n", section_node->name,
> +				       element_node->name, element_node->value);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
>  {
>  	char *section_name;
> @@ -617,6 +659,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>  		else
>  			goto out_err;
>  		goto out;
> +	case ACTION_LIST_ALL:
> +		if (argc == 0)
> +			ret = show_all_config();
> +		else
> +			goto out_err;
> +		goto out;
>  	default:
>  		if ((!has_option || use_global_config || use_system_config)
>  		    && argc == 0) {
> -- 
> 1.9.1
> 

Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[taeung]

Hi, Namhyung

On 07/27/2015 05:48 PM, Namhyung Kim wrote:
> On Mon, Jul 27, 2015 at 12:58:30AM +0900, Taeung Song wrote:
>> A option 'list-all' is to display both current config variables and
>> all possible config variables with default values.
>> The syntax examples are like below
>>
>>      perf config [options]
>>
>>      display all perf config with default values.
>>      # perf config -a | --list-all
>>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>   tools/perf/Documentation/perf-config.txt |  6 ++++
>>   tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
>> index cd4b1a6..d8b3acc 100644
>> --- a/tools/perf/Documentation/perf-config.txt
>> +++ b/tools/perf/Documentation/perf-config.txt
>> @@ -11,6 +11,8 @@ SYNOPSIS
>>   'perf config' [<file-option>] [section.name[=value] ...]
>>   or
>>   'perf config' [<file-option>] -l | --list
>> +or
>> +'perf config' [<file-option>] -a | --list-all
>>   
>>   DESCRIPTION
>>   -----------
>> @@ -31,6 +33,10 @@ OPTIONS
>>   	For writing and reading options: write to system-wide
>>   	'$(sysconfdir)/perfconfig' or read it.
>>   
>> +-a::
>> +--list-all::
>> +	Show current and all possible config variables with default values.
>> +
>>   CONFIGURATION FILE
>>   ------------------
>>   
>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>> index 6d9f28c..f4a1569 100644
>> --- a/tools/perf/builtin-config.c
>> +++ b/tools/perf/builtin-config.c
>> @@ -23,6 +23,7 @@ static const char * const config_usage[] = {
>>   };
>>   
>>   #define ACTION_LIST (1<<0)
>> +#define ACTION_LIST_ALL (1<<1)
>>   
>>   static const struct option config_options[] = {
>>   	OPT_GROUP("Config file location"),
>> @@ -31,6 +32,8 @@ static const struct option config_options[] = {
>>   	OPT_GROUP("Action"),
>>   	OPT_BIT('l', "list", &actions,
>>   		"show current config variables", ACTION_LIST),
>> +	OPT_BIT('a', "list-all", &actions,
>> +		"show current and all possible config variables with default values", ACTION_LIST_ALL),
> Why did you use OPT_BIT?  Do you want to support multiple 'actions' at
> the same time?  I'd rather support just one action, but I won't insist
> it strongly..  Anyway, setting bits will confuse the switch statement
> in the cmd_config().
>
> Thanks,
> Namhyung
>
I don't understand why setting bits will confuse the switch statement.
Is the reason about readability of source code ?

But I searched for other parse-option which can be replaced.
Is it better to use OPT_SET_INT instead of OPT_BIT ?

Thanks,
Taeung

>>   	OPT_END()
>>   };
>>   
>> @@ -539,6 +542,45 @@ static int collect_current_config(const char *var, const char *value,
>>   			   normalize_value(section_name, name, value));
>>   }
>>   
>> +static int show_all_config(void)
>> +{
>> +	int i;
>> +	bool has_config;
>> +	struct config_section *section_node;
>> +	struct config_element *element_node;
>> +
>> +	for (i = 0; default_configsets[i].section_name != NULL; i++) {
>> +		find_config(&section_node, &element_node,
>> +			    default_configsets[i].section_name, default_configsets[i].name);
>> +
>> +		if (!element_node)
>> +			printf("%s.%s=%s\n", default_configsets[i].section_name,
>> +			       default_configsets[i].name, default_configsets[i].value);
>> +		else
>> +			printf("%s.%s=%s\n", section_node->name,
>> +			       element_node->name, element_node->value);
>> +	}
>> +
>> +	/* Print config variables the default configsets haven't */
>> +	list_for_each_entry(section_node, &sections, list) {
>> +		list_for_each_entry(element_node, &section_node->element_head, list) {
>> +			has_config = false;
>> +			for (i = 0; default_configsets[i].section_name != NULL; i++) {
>> +				if (!strcmp(default_configsets[i].section_name, section_node->name)
>> +				    && !strcmp(default_configsets[i].name, element_node->name)) {
>> +					has_config = true;
>> +					break;
>> +				}
>> +			}
>> +			if (!has_config)
>> +				printf("%s.%s=%s\n", section_node->name,
>> +				       element_node->name, element_node->value);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
>>   {
>>   	char *section_name;
>> @@ -617,6 +659,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>>   		else
>>   			goto out_err;
>>   		goto out;
>> +	case ACTION_LIST_ALL:
>> +		if (argc == 0)
>> +			ret = show_all_config();
>> +		else
>> +			goto out_err;
>> +		goto out;
>>   	default:
>>   		if ((!has_option || use_global_config || use_system_config)
>>   		    && argc == 0) {
>> -- 
>> 1.9.1
>>

-- 
Thanks,
Taeung

Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Taewoong Song]

Hi, Namhyung


> On Aug 7, 2015, at 10:12 AM, taeung <treeze.taeung@gmail.com> wrote:
> 
> Hi, Namhyung
> 
> On 07/27/2015 05:48 PM, Namhyung Kim wrote:
>> On Mon, Jul 27, 2015 at 12:58:30AM +0900, Taeung Song wrote:
>>> A option 'list-all' is to display both current config variables and
>>> all possible config variables with default values.
>>> The syntax examples are like below
>>> 
>>>     perf config [options]
>>> 
>>>     display all perf config with default values.
>>>     # perf config -a | --list-all
>>> 
>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>> ---
>>>  tools/perf/Documentation/perf-config.txt |  6 ++++
>>>  tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
>>>  2 files changed, 54 insertions(+)
>>> 
>>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
>>> index cd4b1a6..d8b3acc 100644
>>> --- a/tools/perf/Documentation/perf-config.txt
>>> +++ b/tools/perf/Documentation/perf-config.txt
>>> @@ -11,6 +11,8 @@ SYNOPSIS
>>>  'perf config' [<file-option>] [section.name[=value] ...]
>>>  or
>>>  'perf config' [<file-option>] -l | --list
>>> +or
>>> +'perf config' [<file-option>] -a | --list-all
>>>    DESCRIPTION
>>>  -----------
>>> @@ -31,6 +33,10 @@ OPTIONS
>>>  	For writing and reading options: write to system-wide
>>>  	'$(sysconfdir)/perfconfig' or read it.
>>>  +-a::
>>> +--list-all::
>>> +	Show current and all possible config variables with default values.
>>> +
>>>  CONFIGURATION FILE
>>>  ------------------
>>>  diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>>> index 6d9f28c..f4a1569 100644
>>> --- a/tools/perf/builtin-config.c
>>> +++ b/tools/perf/builtin-config.c
>>> @@ -23,6 +23,7 @@ static const char * const config_usage[] = {
>>>  };
>>>    #define ACTION_LIST (1<<0)
>>> +#define ACTION_LIST_ALL (1<<1)
>>>    static const struct option config_options[] = {
>>>  	OPT_GROUP("Config file location"),
>>> @@ -31,6 +32,8 @@ static const struct option config_options[] = {
>>>  	OPT_GROUP("Action"),
>>>  	OPT_BIT('l', "list", &actions,
>>>  		"show current config variables", ACTION_LIST),
>>> +	OPT_BIT('a', "list-all", &actions,
>>> +		"show current and all possible config variables with default values", ACTION_LIST_ALL),
>> Why did you use OPT_BIT?  Do you want to support multiple 'actions' at
>> the same time?  I'd rather support just one action, but I won't insist
>> it strongly..  Anyway, setting bits will confuse the switch statement
>> in the cmd_config().
>> 
>> Thanks,
>> Namhyung
>> 
> I don't understand why setting bits will confuse the switch statement.
> Is the reason about readability of source code ?
> 
> But I searched for other parse-option which can be replaced.
> Is it better to use OPT_SET_INT instead of OPT_BIT ?
> 

I modified source code to use OPT_SET_UINT
but the problem is happened. I declared "enum actions” to use OPT_SET_UINT like this.

enum actions {
    ACTION_LIST,
    ACTION_LIST_ALL,
    ACTION_REMOVE
} actions;

But default value of actions” variable is 0 and ACTION_LIST is also 0
so when ‘get’ or ’set’ feature is used, ‘default:' of the switch statement in cmd_config() can’t reached
because of default value of ‘actions’ variable is 0.

Use other parse-option function instead of OPT_SET_UINT ?


Thanks,
Taeung

> Thanks,
> Taeung
> 
>>>  	OPT_END()
>>>  };
>>>  @@ -539,6 +542,45 @@ static int collect_current_config(const char *var, const char *value,
>>>  			   normalize_value(section_name, name, value));
>>>  }
>>>  +static int show_all_config(void)
>>> +{
>>> +	int i;
>>> +	bool has_config;
>>> +	struct config_section *section_node;
>>> +	struct config_element *element_node;
>>> +
>>> +	for (i = 0; default_configsets[i].section_name != NULL; i++) {
>>> +		find_config(&section_node, &element_node,
>>> +			    default_configsets[i].section_name, default_configsets[i].name);
>>> +
>>> +		if (!element_node)
>>> +			printf("%s.%s=%s\n", default_configsets[i].section_name,
>>> +			       default_configsets[i].name, default_configsets[i].value);
>>> +		else
>>> +			printf("%s.%s=%s\n", section_node->name,
>>> +			       element_node->name, element_node->value);
>>> +	}
>>> +
>>> +	/* Print config variables the default configsets haven't */
>>> +	list_for_each_entry(section_node, &sections, list) {
>>> +		list_for_each_entry(element_node, &section_node->element_head, list) {
>>> +			has_config = false;
>>> +			for (i = 0; default_configsets[i].section_name != NULL; i++) {
>>> +				if (!strcmp(default_configsets[i].section_name, section_node->name)
>>> +				    && !strcmp(default_configsets[i].name, element_node->name)) {
>>> +					has_config = true;
>>> +					break;
>>> +				}
>>> +			}
>>> +			if (!has_config)
>>> +				printf("%s.%s=%s\n", section_node->name,
>>> +				       element_node->name, element_node->value);
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
>>>  {
>>>  	char *section_name;
>>> @@ -617,6 +659,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
>>>  		else
>>>  			goto out_err;
>>>  		goto out;
>>> +	case ACTION_LIST_ALL:
>>> +		if (argc == 0)
>>> +			ret = show_all_config();
>>> +		else
>>> +			goto out_err;
>>> +		goto out;
>>>  	default:
>>>  		if ((!has_option || use_global_config || use_system_config)
>>>  		    && argc == 0) {
>>> -- 
>>> 1.9.1
>>> 
> 
> -- 
> Thanks,
> Taeung
> 

Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Namhyung Kim]

On Sat, Aug 08, 2015 at 06:50:59PM +0900, Taewoong Song wrote:
> >>>  	OPT_GROUP("Action"),
> >>>  	OPT_BIT('l', "list", &actions,
> >>>  		"show current config variables", ACTION_LIST),
> >>> +	OPT_BIT('a', "list-all", &actions,
> >>> +		"show current and all possible config variables with default values", ACTION_LIST_ALL),
> >> Why did you use OPT_BIT?  Do you want to support multiple 'actions' at
> >> the same time?  I'd rather support just one action, but I won't insist
> >> it strongly..  Anyway, setting bits will confuse the switch statement
> >> in the cmd_config().
> >> 
> >> Thanks,
> >> Namhyung
> >> 
> > I don't understand why setting bits will confuse the switch statement.
> > Is the reason about readability of source code ?
> > 
> > But I searched for other parse-option which can be replaced.
> > Is it better to use OPT_SET_INT instead of OPT_BIT ?
> > 
> 
> I modified source code to use OPT_SET_UINT
> but the problem is happened. I declared "enum actions” to use OPT_SET_UINT like this.
> 
> enum actions {
>     ACTION_LIST,
>     ACTION_LIST_ALL,
>     ACTION_REMOVE
> } actions;
> 
> But default value of actions” variable is 0 and ACTION_LIST is also 0
> so when ‘get’ or ’set’ feature is used, ‘default:' of the switch statement in cmd_config() can’t reached
> because of default value of ‘actions’ variable is 0.

I don't know what's the problem.  You could either set default value
to -1 or make ACTION_LIST start from 1.

Thanks,
Namhyung

Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Namhyung Kim]

On Fri, Aug 07, 2015 at 10:12:02AM +0900, taeung wrote:
> Hi, Namhyung
> 
> On 07/27/2015 05:48 PM, Namhyung Kim wrote:
> >On Mon, Jul 27, 2015 at 12:58:30AM +0900, Taeung Song wrote:
> >>A option 'list-all' is to display both current config variables and
> >>all possible config variables with default values.
> >>The syntax examples are like below
> >>
> >>     perf config [options]
> >>
> >>     display all perf config with default values.
> >>     # perf config -a | --list-all
> >>
> >>Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> >>---
> >>  tools/perf/Documentation/perf-config.txt |  6 ++++
> >>  tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
> >>  2 files changed, 54 insertions(+)
> >>
> >>diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> >>index cd4b1a6..d8b3acc 100644
> >>--- a/tools/perf/Documentation/perf-config.txt
> >>+++ b/tools/perf/Documentation/perf-config.txt
> >>@@ -11,6 +11,8 @@ SYNOPSIS
> >>  'perf config' [<file-option>] [section.name[=value] ...]
> >>  or
> >>  'perf config' [<file-option>] -l | --list
> >>+or
> >>+'perf config' [<file-option>] -a | --list-all
> >>  DESCRIPTION
> >>  -----------
> >>@@ -31,6 +33,10 @@ OPTIONS
> >>  	For writing and reading options: write to system-wide
> >>  	'$(sysconfdir)/perfconfig' or read it.
> >>+-a::
> >>+--list-all::
> >>+	Show current and all possible config variables with default values.
> >>+
> >>  CONFIGURATION FILE
> >>  ------------------
> >>diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> >>index 6d9f28c..f4a1569 100644
> >>--- a/tools/perf/builtin-config.c
> >>+++ b/tools/perf/builtin-config.c
> >>@@ -23,6 +23,7 @@ static const char * const config_usage[] = {
> >>  };
> >>  #define ACTION_LIST (1<<0)
> >>+#define ACTION_LIST_ALL (1<<1)
> >>  static const struct option config_options[] = {
> >>  	OPT_GROUP("Config file location"),
> >>@@ -31,6 +32,8 @@ static const struct option config_options[] = {
> >>  	OPT_GROUP("Action"),
> >>  	OPT_BIT('l', "list", &actions,
> >>  		"show current config variables", ACTION_LIST),
> >>+	OPT_BIT('a', "list-all", &actions,
> >>+		"show current and all possible config variables with default values", ACTION_LIST_ALL),
> >Why did you use OPT_BIT?  Do you want to support multiple 'actions' at
> >the same time?  I'd rather support just one action, but I won't insist
> >it strongly..  Anyway, setting bits will confuse the switch statement
> >in the cmd_config().
> >
> >Thanks,
> >Namhyung
> >
> I don't understand why setting bits will confuse the switch statement.
> Is the reason about readability of source code ?

Supposed you set ADD as 1 and DEL as 2.  If you want do both action,
it'll have value of 3.  But switch statement only have case 1 or 2
(unless you give all possible combinations - but I don't think we want
it).


> 
> But I searched for other parse-option which can be replaced.
> Is it better to use OPT_SET_INT instead of OPT_BIT ?

Please just use OPT_INTEGER.

Thanks,
Namhyung

Re: [PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Taewoong Song]

> On Aug 9, 2015, at 12:01 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> On Fri, Aug 07, 2015 at 10:12:02AM +0900, taeung wrote:
>> Hi, Namhyung
>> 
>> On 07/27/2015 05:48 PM, Namhyung Kim wrote:
>>> On Mon, Jul 27, 2015 at 12:58:30AM +0900, Taeung Song wrote:
>>>> A option 'list-all' is to display both current config variables and
>>>> all possible config variables with default values.
>>>> The syntax examples are like below
>>>> 
>>>>    perf config [options]
>>>> 
>>>>    display all perf config with default values.
>>>>    # perf config -a | --list-all
>>>> 
>>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>>> ---
>>>> tools/perf/Documentation/perf-config.txt |  6 ++++
>>>> tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
>>>> 2 files changed, 54 insertions(+)
>>>> 
>>>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
>>>> index cd4b1a6..d8b3acc 100644
>>>> --- a/tools/perf/Documentation/perf-config.txt
>>>> +++ b/tools/perf/Documentation/perf-config.txt
>>>> @@ -11,6 +11,8 @@ SYNOPSIS
>>>> 'perf config' [<file-option>] [section.name[=value] ...]
>>>> or
>>>> 'perf config' [<file-option>] -l | --list
>>>> +or
>>>> +'perf config' [<file-option>] -a | --list-all
>>>> DESCRIPTION
>>>> -----------
>>>> @@ -31,6 +33,10 @@ OPTIONS
>>>> 	For writing and reading options: write to system-wide
>>>> 	'$(sysconfdir)/perfconfig' or read it.
>>>> +-a::
>>>> +--list-all::
>>>> +	Show current and all possible config variables with default values.
>>>> +
>>>> CONFIGURATION FILE
>>>> ------------------
>>>> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
>>>> index 6d9f28c..f4a1569 100644
>>>> --- a/tools/perf/builtin-config.c
>>>> +++ b/tools/perf/builtin-config.c
>>>> @@ -23,6 +23,7 @@ static const char * const config_usage[] = {
>>>> };
>>>> #define ACTION_LIST (1<<0)
>>>> +#define ACTION_LIST_ALL (1<<1)
>>>> static const struct option config_options[] = {
>>>> 	OPT_GROUP("Config file location"),
>>>> @@ -31,6 +32,8 @@ static const struct option config_options[] = {
>>>> 	OPT_GROUP("Action"),
>>>> 	OPT_BIT('l', "list", &actions,
>>>> 		"show current config variables", ACTION_LIST),
>>>> +	OPT_BIT('a', "list-all", &actions,
>>>> +		"show current and all possible config variables with default values", ACTION_LIST_ALL),
>>> Why did you use OPT_BIT?  Do you want to support multiple 'actions' at
>>> the same time?  I'd rather support just one action, but I won't insist
>>> it strongly..  Anyway, setting bits will confuse the switch statement
>>> in the cmd_config().
>>> 
>>> Thanks,
>>> Namhyung
>>> 
>> I don't understand why setting bits will confuse the switch statement.
>> Is the reason about readability of source code ?
> 
> Supposed you set ADD as 1 and DEL as 2.  If you want do both action,
> it'll have value of 3.  But switch statement only have case 1 or 2
> (unless you give all possible combinations - but I don't think we want
> it).
> 

I understood what you said. Combinations of actions isn’t needed.
So we don’t need to use OPT_BIT in perf-config.

But I thought that if more than two separate or same  actions is used
error messages should be printed like “error: only one action at a time” like this.

# perf config -l -l
error: only one action at a time

# perf config -l -a -r test.test
error: only one action at a time


Then using more than two actions should be blocked.

I thought about 2 solutions

1) comparing original ‘argc' and ‘argc' after parse_options() work like this.

int origin_argc = arc -1;
argc = parse_options(argc, argv, config_options, config_usage,
                                   PARSE_OPT_STOP_AT_NON_OPTION);

if (origin_argc != argc -1)
        pr_err(“error: only one action at a time\n");


 2) using OPT_BIT and HAS_MULTI_BIT()

HAS_MULTI_BIT() can check whether more than two actions is used or not.


Is needed this exception handing ?
Or it isn’t needed ? 

> 
>> 
>> But I searched for other parse-option which can be replaced.
>> Is it better to use OPT_SET_INT instead of OPT_BIT ?
> 
> Please just use OPT_INTEGER.

I modified source code to use OPT_INTEGER.
But OPT_INTEGER require entering integer value like this.

# perf config —list=1
or
# perf config -l 1

So, I just use OPT_SET_UINT because it can have default value
in contrast with OPT_INTEGER like this.

static const struct option config_options[] = {
        OPT_GROUP(“Action”),
        OPT_SET_UINT(‘l’, “list”, &actions,
                                    “show current config variables”, ACTION_LIST),

And I declared enum variable to distinguish between default value 0
and value of ACTION_LIST like this.

enum actions {
    ACTION_LIST = 1,
    ACTION_LIST_ALL,
    ACTION_REMOVE
} actions;


Aren’t there problems if using OPT_SET_UINT instead of OPT_INTEGER ?


Thanks,
Taeung

> 
> Thanks,
> Namhyung

[PATCH v4 5/5] perf config: Add a option 'remove' to perf-config

[Taeung Song]

A option 'remove' is to remove specific config variables.
For the syntax examples,

    # perf config -r | --remove [section.name ...]

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/Documentation/perf-config.txt |  6 ++++++
 tools/perf/builtin-config.c              | 25 ++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index d8b3acc..416637b 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -13,6 +13,8 @@ or
 'perf config' [<file-option>] -l | --list
 or
 'perf config' [<file-option>] -a | --list-all
+or
+'perf config' [<file-option>] -r | --remove [section.name ...]
 
 DESCRIPTION
 -----------
@@ -37,6 +39,10 @@ OPTIONS
 --list-all::
 	Show current and all possible config variables with default values.
 
+-r::
+--remove::
+	Remove specific config variables.
+
 CONFIGURATION FILE
 ------------------
 
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index f4a1569..74fcedc 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -24,6 +24,7 @@ static const char * const config_usage[] = {
 
 #define ACTION_LIST (1<<0)
 #define ACTION_LIST_ALL (1<<1)
+#define ACTION_REMOVE (1<<2)
 
 static const struct option config_options[] = {
 	OPT_GROUP("Config file location"),
@@ -34,6 +35,8 @@ static const struct option config_options[] = {
 		"show current config variables", ACTION_LIST),
 	OPT_BIT('a', "list-all", &actions,
 		"show current and all possible config variables with default values", ACTION_LIST_ALL),
+	OPT_BIT('r', "remove", &actions,
+		"remove specific variables: [section.name ...]", ACTION_REMOVE),
 	OPT_END()
 };
 
@@ -484,7 +487,15 @@ static int set_config(const char *section_name, const char *name, char *value)
 	struct config_element *element_node = NULL;
 
 	find_config(&section_node, &element_node, section_name, name);
-	if (value != NULL) {
+	if (!value) {
+		/* value == NULL means remove the variable */
+		if (section_node && element_node) {
+			if (!element_node->value)
+				free(element_node->value);
+			element_node->value = NULL;
+		} else
+			pr_err("Error: Failed to find the variable.\n");
+	} else {
 		value = normalize_value(section_name, name, value);
 
 		/* if there isn't existent section, add a new section */
@@ -665,6 +676,18 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 		else
 			goto out_err;
 		goto out;
+	case ACTION_REMOVE:
+		for (i = 0; argv[i]; i++) {
+			if (value == NULL)
+				ret = perf_configset_with_option(set_config, argv[i], NULL);
+			else {
+				pr_err("invalid key: %s\n", argv[i]);
+				return -1;
+			}
+			if (ret < 0)
+				goto out;
+		}
+		goto out;
 	default:
 		if ((!has_option || use_global_config || use_system_config)
 		    && argc == 0) {
-- 
1.9.1

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

[Taeung Song]

Changes in v4:
	- If some config value is default value, notice it is '(default)'as below.

	  # perf config colors.normal
	  colors.normal=lightgray, default (default)

	- A config file path is only decided in perf_config().
	And a perf-config command depend on perf_config() deciding a config file path.
	So if the config file path is NULL because of some reason like no such file,
	a perf-config command can't write to the file. So, I added new options
	which are '--global' and '--system' to be enable to select
	config file path to be used without perf_config().

Changes in v3:
	- Add a config variable 'kmem.default' with a default value
	into 'struct default_configset' which has default config variables and values.

	- Add a option '--global' and '--local' to enable config file location to  be selected

Changes in v2:
	- Renaming variables a more suitable name
	  1. '--list-all' instead of '--all'
	  2. 'name' instead of 'subkey'
	  3. 'section, name, value' instead of 'given_section, subkey, value'

	- Correct small infelicities or typing errors in a perf-config documention.

	- Remove a part description of report.children
	because it was duplicated in Documentation/callchain-overhead-calculation.txt

	- Use a variable 'int actions' instead of struct params which has
	'bool list_action' , 'bool get_action’ and etc. , to simplify a branching statement
	for perf-config options.

	- Declaration a global variable 'static struct default_configsets' has config variables
	with default values instead of using a 'util/PERFCONFIG-DEFAULT' file
	and remove functions merge() and perse_key() to get perf config default values.

	- Add a function to normalize a value and check data type of it.

	- Simplify parsing arguments as arguments is just divided by '=' and then
	in front of '.' is a section, between '.' and '=' is a name, and behind '=' is a value.

	- Print config variables 'struct default_configsets' haven't 

	- Make a command ’perf config' without a option work as with a option '—list'
	instead of '--list-all’.

Taeung Song (5):
  perf tools: Add 'perf-config' command
  perf config: Add '--system' and '--global' options to be able to
    select which config file to be used
  perf config: Add functions which can get or set perf config variables
  perf config: Add a option 'list-all' to perf-config
  perf config: Add a option 'remove' to perf-config

 tools/perf/Build                            |   1 +
 tools/perf/Documentation/perf-config.txt    | 407 ++++++++++++++++
 tools/perf/Documentation/perfconfig.example |  73 ++-
 tools/perf/builtin-config.c                 | 718 ++++++++++++++++++++++++++++
 tools/perf/builtin.h                        |   1 +
 tools/perf/command-list.txt                 |   1 +
 tools/perf/perf.c                           |   1 +
 tools/perf/util/cache.h                     |  19 +
 tools/perf/util/config.c                    |  29 +-
 9 files changed, 1236 insertions(+), 14 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-config.txt
 create mode 100644 tools/perf/builtin-config.c

-- 
1.9.1

[PATCH v4 4/5] perf config: Add a option 'list-all' to perf-config

[Taeung Song]

A option 'list-all' is to display both current config variables and
all possible config variables with default values.
The syntax examples are like below

    perf config [options]

    display all perf config with default values.
    # perf config -a | --list-all

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/Documentation/perf-config.txt |  6 ++++
 tools/perf/builtin-config.c              | 48 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index cd4b1a6..d8b3acc 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -11,6 +11,8 @@ SYNOPSIS
 'perf config' [<file-option>] [section.name[=value] ...]
 or
 'perf config' [<file-option>] -l | --list
+or
+'perf config' [<file-option>] -a | --list-all
 
 DESCRIPTION
 -----------
@@ -31,6 +33,10 @@ OPTIONS
 	For writing and reading options: write to system-wide
 	'$(sysconfdir)/perfconfig' or read it.
 
+-a::
+--list-all::
+	Show current and all possible config variables with default values.
+
 CONFIGURATION FILE
 ------------------
 
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 6d9f28c..f4a1569 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -23,6 +23,7 @@ static const char * const config_usage[] = {
 };
 
 #define ACTION_LIST (1<<0)
+#define ACTION_LIST_ALL (1<<1)
 
 static const struct option config_options[] = {
 	OPT_GROUP("Config file location"),
@@ -31,6 +32,8 @@ static const struct option config_options[] = {
 	OPT_GROUP("Action"),
 	OPT_BIT('l', "list", &actions,
 		"show current config variables", ACTION_LIST),
+	OPT_BIT('a', "list-all", &actions,
+		"show current and all possible config variables with default values", ACTION_LIST_ALL),
 	OPT_END()
 };
 
@@ -539,6 +542,45 @@ static int collect_current_config(const char *var, const char *value,
 			   normalize_value(section_name, name, value));
 }
 
+static int show_all_config(void)
+{
+	int i;
+	bool has_config;
+	struct config_section *section_node;
+	struct config_element *element_node;
+
+	for (i = 0; default_configsets[i].section_name != NULL; i++) {
+		find_config(&section_node, &element_node,
+			    default_configsets[i].section_name, default_configsets[i].name);
+
+		if (!element_node)
+			printf("%s.%s=%s\n", default_configsets[i].section_name,
+			       default_configsets[i].name, default_configsets[i].value);
+		else
+			printf("%s.%s=%s\n", section_node->name,
+			       element_node->name, element_node->value);
+	}
+
+	/* Print config variables the default configsets haven't */
+	list_for_each_entry(section_node, &sections, list) {
+		list_for_each_entry(element_node, &section_node->element_head, list) {
+			has_config = false;
+			for (i = 0; default_configsets[i].section_name != NULL; i++) {
+				if (!strcmp(default_configsets[i].section_name, section_node->name)
+				    && !strcmp(default_configsets[i].name, element_node->name)) {
+					has_config = true;
+					break;
+				}
+			}
+			if (!has_config)
+				printf("%s.%s=%s\n", section_node->name,
+				       element_node->name, element_node->value);
+		}
+	}
+
+	return 0;
+}
+
 static int perf_configset_with_option(configset_fn_t fn, const char *var, char *value)
 {
 	char *section_name;
@@ -617,6 +659,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
 		else
 			goto out_err;
 		goto out;
+	case ACTION_LIST_ALL:
+		if (argc == 0)
+			ret = show_all_config();
+		else
+			goto out_err;
+		goto out;
 	default:
 		if ((!has_option || use_global_config || use_system_config)
 		    && argc == 0) {
-- 
1.9.1