linux-kernel - Re: [PATCH v9 1/7] perf config: Introduce default_config_section and default_config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9adee2dc-542c-bcfd-19f6-eee05dcb280d@gmail.com>
Date:   Wed, 30 Nov 2016 02:21:56 +0900
From:   Taeung Song <treeze.taeung@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Wang Nan <wangnan0@...wei.com>, Jiri Olsa <jolsa@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH v9 1/7] perf config: Introduce default_config_section and
 default_config_item for default config key-value pairs

Hi, Arnaldo :)

On 11/30/2016 01:05 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 28, 2016 at 05:51:12PM +0900, Taeung Song escreveu:
>> When initializing default perf config values,
>> we currently use values of actual type(int, bool, char *, etc.).
>>
>> For example,
>> If there isn't a user config value for 'annotate.use_offset'
>> config variable at ~/.perfconfig,
>> default value for it is 'true' bool type value in perf like below.
>>
>> At ui/browsers/annoate.c
>>
>> static struct annotate_browser_opt {
>>         bool hide_src_code,
>>              use_offset,
>>              jump_arrows,
>>              show_linenr,
>>              show_nr_jumps,
>>              show_total_period;
>> } annotate_browser__opts = {
>>         .use_offset      = true,
>>         .jump_arrows     = true,
>> };
>
> And isn't this a good thing? I.e. its compact, we have just to set
> values to things that are !0 (or !false), and it is close to the code
> where it is used.

I understood it. But I think we also need to manage all default config
values in only util/config.c.

The reason is that if we individually have default config arrays,
some difference can be made between actual config values and
values of default config arrays. (e.g. if we change default config
values or add new default config items and values)

So IMHO, I suggest that we use default config arrays instead of
actual type values even though its compact.

And if we have default config arrays, we can also show default values as 
below.

     [acme@...et linux]$ perf config report.queue-size report.children
     report.children=false
     # report.queue-size=18446744073709551615 # Default, not set in 
~/.perfconfig
     [acme@...et linux]$


Just individually make default config arrays in util/config.c ?

Thanks,
Taeung

>> But I suggest using 'struct default_config_section' and
>> 'struct default_config_item' that can contain default config key-value pairs
>> in order to initialize default config values with them, in near future.
>
>
>> If we do, we could manage default perf config values at one spot (i.e. util/config.c)
>> with default config arrays and it could be easy and simple to modify
>> existing default config values or add default values for new config item.
>>
>> Cc: Namhyung Kim <namhyung@...nel.org>
>> Cc: Jiri Olsa <jolsa@...hat.com>
>> Cc: Wang Nan <wangnan0@...wei.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Masami Hiramatsu <mhiramat@...nel.org>
>> Cc: David Ahern <dsahern@...il.com>
>> Signed-off-by: Taeung Song <treeze.taeung@...il.com>
>> ---
>>  tools/perf/util/config.h | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h
>> index 1a59a6b..434d71c 100644
>> --- a/tools/perf/util/config.h
>> +++ b/tools/perf/util/config.h
>> @@ -67,4 +67,33 @@ void perf_config__refresh(void);
>>  	perf_config_sections__for_each_entry(&set->sections, section)		\
>>  	perf_config_items__for_each_entry(&section->items, item)
>>
>> +enum perf_config_type {
>> +	CONFIG_TYPE_BOOL,
>> +	CONFIG_TYPE_INT,
>> +	CONFIG_TYPE_LONG,
>> +	CONFIG_TYPE_U64,
>> +	CONFIG_TYPE_FLOAT,
>> +	CONFIG_TYPE_DOUBLE,
>> +	CONFIG_TYPE_STRING
>> +};
>> +
>> +struct default_config_item {
>> +	const char *name;
>> +	union {
>> +		bool b;
>> +		int i;
>> +		u32 l;
>> +		u64 ll;
>> +		float f;
>> +		double d;
>> +		const char *s;
>> +	} value;
>> +	enum perf_config_type type;
>> +};
>> +
>> +struct default_config_section {
>> +	const char *name;
>> +	const struct default_config_item *items;
>> +};
>> +
>>  #endif /* __PERF_CONFIG_H */
>> --
>> 2.7.4