Re: RFC: turn kmalloc+memset(,0,) into kcalloc

[Paulo Marques <pmarques@grupopie.com>]

Jörn Engel wrote:
> On Tue, 5 April 2005 22:01:49 +0200, Jesper Juhl wrote:
>
> > On Tue, 5 Apr 2005, Roland Dreier wrote:
> >
> >
> > >    > or simply
> > >    > 	if (!(ptr = kcalloc(n, size, ...)))
> > >    > 		goto out;
> > >    > and save an additional line of screen realestate while you are at it...
> > >
> > > No, please don't do that.  The general kernel style is to avoid
> > > assignments within conditionals.

FWIW, I also agree that assignments within conditionals are evil and 
hurt readability a lot, for no actual benefit.

Since one of the advantages of this cleanup is improve readability, it 
would be counterproductive to do this.

To explain why this cleanup improves readability take the following 
sample from sound/usb/usx2y/usbusx2yaudio.c (a random sample):

> 		us = kmalloc(sizeof(*us) + sizeof(struct urb*) * NOOF_SETRATE_URBS, GFP_KERNEL);
> 		if (NULL == us) {
> 			err = -ENOMEM;
> 			goto cleanup;
> 		}
> 		memset(us, 0, sizeof(*us) + sizeof(struct urb*) * NOOF_SETRATE_URBS); 

In this case you have to read the size portion of the kmalloc and the 
memset carefully to make sure that they are exactly the same, whereas in 
code like this:

> 		us = kcalloc(1, sizeof(*us) + sizeof(struct urb*) * NOOF_SETRATE_URBS, GFP_KERNEL);
> 		if (NULL == us) {
> 			err = -ENOMEM;
> 			goto cleanup;
> 		}

it is self-evident that the whole area is being cleared. It also leaves 
a smaller room for mistakes.

I still don't like the kcalloc API with a "1" argument. Not only most of 
these allocations fall into that case, but also kmalloc_zero (or 
kmalloc_zeroed) is much more clear than kcalloc that changes just one 
letter from kmalloc. Someone reading fast through the code may kcalloc 
as if it were kmalloc.

More over, passing an extra parameter waste a few more bytes of code. I 
know is not much, but if the cleanup will address hundreds of these then 
it starts to be something to consider.

However "calloc" is the standard C interface for doing this, so it makes 
some sense to use it here as well... :(

--
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/