linux-kernel - Re: [PATCH v8 3/4] perf config: Prepare all default configs

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: <5717A04C.1080403@gmail.com>
Date:	Thu, 21 Apr 2016 00:29:16 +0900
From:	Taeung Song <treeze.taeung@...il.com>
To:	Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
	Namhyung Kim <namhyung@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH v8 3/4] perf config: Prepare all default configs

Hi, Arnaldo :-)

On 04/20/2016 10:22 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 20, 2016 at 09:44:38PM +0900, Namhyung Kim escreveu:
>> On Mon, Apr 18, 2016 at 11:55:18PM +0900, Taeung Song wrote:
>>> On 04/15/2016 01:42 AM, Taeung Song wrote:
>>>> On 04/14/2016 09:19 PM, Arnaldo Carvalho de Melo wrote:
>>>>> Em Thu, Apr 14, 2016 at 04:53:20PM +0900, Taeung Song escreveu:
>>>>>> +++ b/tools/perf/util/config.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>> +#define MAX_CONFIGS 64
>
>>>>> Do we have to add another arbitrary maximum? Where is it used?
>
>>>> IMHO, it is my idea. If you want to avoid using this arbitrary maxinum,
>>>> I'd modify the code.
>>>>
>>>> MAX_CONFIGS is used in order to declare two-dimensional arrays
>>>> 'default_config_items'
>
>>> As above, I used MAX_CONFIGS because of two-dimensinal arrays
>>> 'default_config_items'.
>
>>> What do you think about it ?
>
>> I also agree that we'd better to avoid the arbitrary maximum.
>
>>> We don't need to add this arbitrary maximum ?
>>> or would you mind, if I look for other way about
>>> 'default_config_item' ?
>>
>> What about this?
>
> Yeah, I guess this should work, no? At least this is how it is done
> elsewhere, see:
>
> tools/perf/builtin-bench.c, 'struct collection' has a benchmarks array,
> that in turn is organized as Namhyung suggests.

I got it!

> Then you either use ARRAY_SIZE() somewhere to get the number of entries
> or use a sentinel, i.e. use NULL for the last entry.
>

I think that it would better to use ARRAY_SIZE() than to use NULL.
It makes no odds but it seem to be clean, IMHO. :)

After changing this patchset, I'll send v9 !

Thanks,
Taeung

>
>> struct perf_config_item color_config_items[] = {
>> 	CONF_STR_VAR("top", "red, default"),
>> 	CONF_STR_VAR("medium", "green, default"),
>> 	...
>> };
>>
>> struct perf_config_item tui_config_items[] = {
>> 	CONF_BOOL_VAR("report", true),
>> 	CONF_BOOL_VAR("annotate", true),
>> 	...
>> };
>>
>> struct perf_config_item *default_config_items[] = {
>> 	&color_config_items,
>> 	&tui_config_items,
>> 	...
>> };
>>
>> This way we can access the config array by using constant index
>> without the hard-coded maximum size IMHO.
>>
>> Thanks,
>> Namhyung