linux-kernel - Re: [PATCH] drivers: Conversions from kmalloc+memset to k(z|c)alloc.

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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 21 Jul 2006 11:09:38 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Jeff Garzik <jgarzik@...ox.com>
CC:	Pekka Enberg <penberg@...helsinki.fi>,
	Rolf Eike Beer <eike-kernel@...tec.de>,
	Panagiotis Issaris <takis@...umba.uhasselt.be>,
	linux-kernel@...r.kernel.org, len.brown@...el.com,
	chas@....nrl.navy.mil, miquel@...uba.ar, kkeil@...e.de,
	benh@...nel.crashing.org, video4linux-list@...hat.com,
	rmk+mmc@....linux.org.uk, Neela.Kolli@...enio.com,
	vandrove@...cvut.cz, adaplas@....net, thomas@...ischhofer.net,
	weissg@...nna.at, philb@....org, linux-pcmcia@...ts.infradead.org,
	jkmaline@...hut.fi, paulus@...ba.org
Subject: Re: [PATCH] drivers: Conversions from kmalloc+memset to k(z|c)alloc.

Jeff Garzik wrote:
> Pekka Enberg wrote:
>> On 7/21/06, Rolf Eike Beer <eike-kernel@...tec.de> wrote:
>>> > -     if (!(handle = kmalloc(sizeof(struct input_handle), GFP_KERNEL)))
>>> > +     handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
>>> > +     if (!handle)
>>> >               return NULL;
>>>
>>> sizeof(*handle)?
>> 
>> In general, yes. However, some maintainers don't like that, so I would
>> recommend to keep them as-is unless you get a clear ack from the
>> maintainer to change it.

I suggest:
 - check if "sizeof(type)"->"sizeof(*ptr)" is correct
 - if yes, change it
 - do this for all kmalloc + kzalloc in a file you touched, or
   better yet for all kmalloc + kzalloc in a driver or subsystem you
   touched
Maintainers who don't agree can always post a NAK. There are hardly any
special cases which speak against that change --- provided the change is
done for the whole subsystem.

> Strongly agreed.  Follow the style of the existing code as closely as 
> possible, and resist the temptation of making little "improvements" 
> while you are doing a task...

The 2nd half of this statement is a good and important rule. But look at
this particular case. The patch does (or could) contain
 - consolidation of kmalloc+memset/0 -> kzalloc where correct,
 - better style of the size argument where correct,
 - whitespace style adjustments of the touched regions.
All of these changes, if done correct, are true improvements WRT
programming idioms. IMO it's a single task, therefore adheres to that
rule. I'd only split this kind of patches if separately maintained
subsystems are touched.

As for the 1st half of this statement, "follow the style of existing
code as closely as possible": This is problematic in light of the more
important rule "follow CodingStyle".

I suggest to follow existing style if small changes are introduced and
the existing code mostly complies to CodingStyle. If existing code with
small deviations from CodingStyle is touched, the potential loss of
readability by partial conversion to CodingStyle may be negligible. If
existing code with bigger deviations from CodingStyle is touched, the
author of a patch should consider to provide an additional patch before
or after that with a complete conversion to CodingStyle. (I took the
time to do this on a few occasions too.)
-- 
Stefan Richter
-=====-=-==- -=== =--==
http://arcgraph.de/sr/
-
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/