linux-kernel - Re: [PATCH 15/16] trivial: use ARRAY

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: <4C290983.5050808@hetnet.nl>
Date:	Mon, 28 Jun 2010 22:43:47 +0200
From:	Henk de Groot <henk.de.groot@...net.nl>
To:	Dan Carpenter <error27@...il.com>,
	Kulikov Vasiliy <segooon@...il.com>, trivial@...nel.org,
	Kernel Janitors <kernel-janitors@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Henk de Groot <pe1dnn@...at.org>,
	Joe Perches <joe@...ches.com>, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 15/16] trivial: use ARRAY_SIZE

Op 28-6-2010 15:24, Dan Carpenter schreef:
> On Mon, Jun 28, 2010 at 05:15:11PM +0400, Kulikov Vasiliy wrote:
>    
>> On Mon, Jun 28, 2010 at 14:52 +0200, Dan Carpenter wrote:
>>      
>>> On Mon, Jun 28, 2010 at 03:55:41PM +0400, Kulikov Vasiliy wrote:
>>>        
>>>> Change sizeof(x) / sizeof(*x) to ARRAY_SIZE(x).
>>>>
>>>> Signed-off-by: Kulikov Vasiliy<segooon@...il.com>
>>>> ---
>>>>   drivers/staging/wlags49_h2/hcf.c |    2 +-
>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wlags49_h2/hcf.c b/drivers/staging/wlags49_h2/hcf.c
>>>> index 390628c..c4fe0ec 100644
>>>> --- a/drivers/staging/wlags49_h2/hcf.c
>>>> +++ b/drivers/staging/wlags49_h2/hcf.c
>>>> @@ -502,7 +502,7 @@ HCF_STATIC hcf_16* BASED xxxx[ ] = {
>>>>   #endif // MSF_COMPONENT_ID
>>>>   	NULL									//endsentinel
>>>>     };
>>>> -#define xxxx_PRI_IDENTITY_OFFSET	(sizeof(xxxx)/sizeof(xxxx[0]) - 3)
>>>> +#define xxxx_PRI_IDENTITY_OFFSET	(ARRAY_SIZE(xxxx) - 3)
>>>>
>>>>          
>>> I would say the more critical problem with this macro is that it doesn't
>>> work unless you name all your arrays "xxxx[]" so it encourages sub par
>>> variable names.
>>>
>>> You could do:
>>> #define PRI_IDENTITY_OFFSET(x) (ARRAY_SIZE(x) - 3)
>>>        
>> Look at the patch:
>>
>>      
>>>> @@ -502,7 +502,7 @@ HCF_STATIC hcf_16* BASED xxxx[ ] = {
>>>>          
>> There is an array called 'xxxx' and macro xxxx_PRI_IDENTITY_OFFSET is defined
>> after array definition. This magic macroconstant is used in the code to get
>> elements of xxxx.
>>      
> Right right.  But xxxx is a stupid name for a variable.  I wanted to
> poke my eyes out with a fork.
>
> Not your fault obviously.  Your patch doesn't make it worse so I'm fine
> with it as far as it goes...
>
> regards,
> dan carpenter
>    

The whole HCF library code is full of this funny stuff. My guess is the 
code is generated and macro's are used to access arrays in a uniform 
way. At least that's the only thing I can think of why it is as it is. 
Only the original developers at Agere can tell the real story. The same 
with structure definitions. You could send this to the IOCCC easily (it 
is only too big to qualify)...

I think the best way to fix this is to take all this funny stuff out and 
make it simplified readable code. Currently its very hard to see through 
all the macro layers what actually happens which make is unmaintainable. 
Changing details is okay, but will never make any of this more readable.

When porting the driver from the original Agere 2.4 kernel code to the 
2.6 driver today I only need to change one or two casts, the rest of the 
HCF code is untouched. The only Linux code is in the wl_* files which 
coding style looks quite different from the rest and is actually readable.

Kind regards,

Henk.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/