[PATCH] use kzalloc instead of malloc+memset

[PATCH] use kzalloc instead of malloc+memset

[Lion Vollnhals]

This patch against 2.6.13-mm3 replaces malloc and memset with kzalloc in drivers/base/class.c .
Furthermore this patch fixes actually two bugs:
  The memset arguments were occasionally swaped and therefore wrong.

Usage of kzalloc makes this code shorter and more bugfree.

Please apply.

Signed-off-by: Lion Vollnhals <webmaster@schiggl.de>

--- 2.6.13-mm3/drivers/base/class.c	2005-09-12 23:42:47.000000000 +0200
+++ 2.6.13-mm3-changed/drivers/base/class.c	2005-09-12 23:54:56.000000000 +0200
@@ -190,12 +190,11 @@ struct class *class_create(struct module
 	struct class *cls;
 	int retval;
 
-	cls = kmalloc(sizeof(struct class), GFP_KERNEL);
+	cls = kzalloc(sizeof(struct class), GFP_KERNEL);
 	if (!cls) {
 		retval = -ENOMEM;
 		goto error;
 	}
-	memset(cls, 0x00, sizeof(struct class));
 
 	cls->name = name;
 	cls->owner = owner;
@@ -519,13 +518,13 @@ int class_device_add(struct class_device
 	/* add the needed attributes to this device */
 	if (MAJOR(class_dev->devt)) {
 		struct class_device_attribute *attr;
-		attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
 		if (!attr) {
 			error = -ENOMEM;
 			kobject_del(&class_dev->kobj);
 			goto register_done;
 		}
-		memset(attr, sizeof(*attr), 0x00);
+		
 		attr->attr.name = "dev";
 		attr->attr.mode = S_IRUGO;
 		attr->attr.owner = parent->owner;
@@ -534,13 +533,13 @@ int class_device_add(struct class_device
 		class_device_create_file(class_dev, attr);
 		class_dev->devt_attr = attr;
 
-		attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
 		if (!attr) {
 			error = -ENOMEM;
 			kobject_del(&class_dev->kobj);
 			goto register_done;
 		}
-		memset(attr, sizeof(*attr), 0x00);
+		
 		attr->attr.name = "sample.sh";
 		attr->attr.mode = S_IRUSR | S_IXUSR | S_IRUGO;
 		attr->attr.owner = parent->owner;
@@ -611,12 +610,11 @@ struct class_device *class_device_create
 	if (cls == NULL || IS_ERR(cls))
 		goto error;
 
-	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
+	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
 	if (!class_dev) {
 		retval = -ENOMEM;
 		goto error;
 	}
-	memset(class_dev, 0x00, sizeof(struct class_device));
 
 	class_dev->devt = devt;
 	class_dev->dev = device;

Re: [PATCH] use kzalloc instead of malloc+memset

[Lion Vollnhals]

On Tuesday 13 September 2005 00:10, Lion Vollnhals wrote:
> This patch against 2.6.13-mm3 replaces malloc and memset with kzalloc in
> drivers/base/class.c . 

The following patch converts further kmalllocs and memsets in drivers/base/* to kzallocs.

Please apply.

Signed-off-by: Lion Vollnhals <webmaster@schiggl.de>

diff -Nurp 2.6.13-mm3/drivers/base/attribute_container.c 2.6.13-mm3-changed/drivers/base/attribute_container.c
--- 2.6.13-mm3/drivers/base/attribute_container.c	2005-09-12 23:42:47.000000000 +0200
+++ 2.6.13-mm3-changed/drivers/base/attribute_container.c	2005-09-13 00:30:59.000000000 +0200
@@ -152,12 +152,13 @@ attribute_container_add_device(struct de
 
 		if (!cont->match(cont, dev))
 			continue;
-		ic = kmalloc(sizeof(struct internal_container), GFP_KERNEL);
+		
+		ic = kzalloc(sizeof(struct internal_container), GFP_KERNEL);
 		if (!ic) {
 			dev_printk(KERN_ERR, dev, "failed to allocate class container\n");
 			continue;
 		}
-		memset(ic, 0, sizeof(struct internal_container));
+		
 		ic->cont = cont;
 		class_device_initialize(&ic->classdev);
 		ic->classdev.dev = get_device(dev);
diff -Nurp 2.6.13-mm3/drivers/base/firmware_class.c 2.6.13-mm3-changed/drivers/base/firmware_class.c
--- 2.6.13-mm3/drivers/base/firmware_class.c	2005-09-12 23:42:47.000000000 +0200
+++ 2.6.13-mm3-changed/drivers/base/firmware_class.c	2005-09-13 00:26:03.000000000 +0200
@@ -301,9 +301,9 @@ fw_register_class_device(struct class_de
 			 const char *fw_name, struct device *device)
 {
 	int retval;
-	struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
+	struct firmware_priv *fw_priv = kzalloc(sizeof (struct firmware_priv),
 						GFP_KERNEL);
-	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
+	struct class_device *class_dev = kzalloc(sizeof (struct class_device),
 						 GFP_KERNEL);
 
 	*class_dev_p = NULL;
@@ -313,8 +313,6 @@ fw_register_class_device(struct class_de
 		retval = -ENOMEM;
 		goto error_kfree;
 	}
-	memset(fw_priv, 0, sizeof (*fw_priv));
-	memset(class_dev, 0, sizeof (*class_dev));
 
 	init_completion(&fw_priv->completion);
 	fw_priv->attr_data = firmware_attr_data_tmpl;
@@ -402,14 +400,13 @@ _request_firmware(const struct firmware 
 	if (!firmware_p)
 		return -EINVAL;
 
-	*firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+	*firmware_p = firmware = kzalloc(sizeof (struct firmware), GFP_KERNEL);
 	if (!firmware) {
 		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
 		       __FUNCTION__);
 		retval = -ENOMEM;
 		goto out;
 	}
-	memset(firmware, 0, sizeof (*firmware));
 
 	retval = fw_setup_class_device(firmware, &class_dev, name, device,
 		hotplug);
diff -Nurp 2.6.13-mm3/drivers/base/map.c 2.6.13-mm3-changed/drivers/base/map.c
--- 2.6.13-mm3/drivers/base/map.c	2005-09-12 23:42:47.000000000 +0200
+++ 2.6.13-mm3-changed/drivers/base/map.c	2005-09-13 00:16:35.000000000 +0200
@@ -135,7 +135,7 @@ retry:
 struct kobj_map *kobj_map_init(kobj_probe_t *base_probe, struct semaphore *sem)
 {
 	struct kobj_map *p = kmalloc(sizeof(struct kobj_map), GFP_KERNEL);
-	struct probe *base = kmalloc(sizeof(struct probe), GFP_KERNEL);
+	struct probe *base = kzalloc(sizeof(struct probe), GFP_KERNEL);
 	int i;
 
 	if ((p == NULL) || (base == NULL)) {
@@ -144,7 +144,6 @@ struct kobj_map *kobj_map_init(kobj_prob
 		return NULL;
 	}
 
-	memset(base, 0, sizeof(struct probe));
 	base->dev = 1;
 	base->range = ~0;
 	base->get = base_probe;
diff -Nurp 2.6.13-mm3/drivers/base/memory.c 2.6.13-mm3-changed/drivers/base/memory.c
--- 2.6.13-mm3/drivers/base/memory.c	2005-09-12 23:42:47.000000000 +0200
+++ 2.6.13-mm3-changed/drivers/base/memory.c	2005-09-13 00:31:55.000000000 +0200
@@ -341,14 +341,12 @@ int add_memory_block(unsigned long node_
 		     unsigned long state, int phys_device)
 {
 	size_t size = sizeof(struct memory_block);
-	struct memory_block *mem = kmalloc(size, GFP_KERNEL);
+	struct memory_block *mem = kzalloc(size, GFP_KERNEL);
 	int ret = 0;
 
 	if (!mem)
 		return -ENOMEM;
 
-	memset(mem, 0, size);
-
 	mem->phys_index = __section_nr(section);
 	mem->state = state;
 	init_MUTEX(&mem->state_sem);
diff -Nurp 2.6.13-mm3/drivers/base/platform.c 2.6.13-mm3-changed/drivers/base/platform.c
--- 2.6.13-mm3/drivers/base/platform.c	2005-09-12 23:42:47.000000000 +0200
+++ 2.6.13-mm3-changed/drivers/base/platform.c	2005-09-13 00:23:43.000000000 +0200
@@ -225,13 +225,12 @@ struct platform_device *platform_device_
 	struct platform_object *pobj;
 	int retval;
 
-	pobj = kmalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
+	pobj = kzalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
 	if (!pobj) {
 		retval = -ENOMEM;
 		goto error;
 	}
 
-	memset(pobj, 0, sizeof(*pobj));
 	pobj->pdev.name = name;
 	pobj->pdev.id = id;
 	pobj->pdev.dev.release = platform_device_release_simple;

Re: [PATCH] use kzalloc instead of malloc+memset

[Jiri Slaby]

Lion Vollnhals napsal(a):

>This patch against 2.6.13-mm3 replaces malloc and memset with kzalloc in drivers/base/class.c .
>Furthermore this patch fixes actually two bugs:
>  The memset arguments were occasionally swaped and therefore wrong.
>
>Usage of kzalloc makes this code shorter and more bugfree.
>
>Please apply.
>
>Signed-off-by: Lion Vollnhals <webmaster@schiggl.de>
>
>--- 2.6.13-mm3/drivers/base/class.c	2005-09-12 23:42:47.000000000 +0200
>+++ 2.6.13-mm3-changed/drivers/base/class.c	2005-09-12 23:54:56.000000000 +0200
>@@ -190,12 +190,11 @@ struct class *class_create(struct module
> 	struct class *cls;
> 	int retval;
> 
>-	cls = kmalloc(sizeof(struct class), GFP_KERNEL);
>+	cls = kzalloc(sizeof(struct class), GFP_KERNEL);
>  
>
maybe, the better way is to write `*cls' instead of `struct class', 
better for further changes

> 	if (!cls) {
> 		retval = -ENOMEM;
> 		goto error;
> 	}
>-	memset(cls, 0x00, sizeof(struct class));
> 
> 	cls->name = name;
> 	cls->owner = owner;
>  
>
[snip]

>@@ -611,12 +610,11 @@ struct class_device *class_device_create
> 	if (cls == NULL || IS_ERR(cls))
> 		goto error;
> 
>-	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
>+	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
>  
>
`*class_dev' instead of `struct class_device'

> 	if (!class_dev) {
> 		retval = -ENOMEM;
> 		goto error;
> 	}
>-	memset(class_dev, 0x00, sizeof(struct class_device));
> 
> 	class_dev->devt = devt;
> 	class_dev->dev = device;
>  
>
thanks,

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] use kzalloc instead of malloc+memset

[Pekka Enberg]

On 9/13/05, Jiri Slaby <jirislaby@gmail.com> wrote:
> >-      cls = kmalloc(sizeof(struct class), GFP_KERNEL);
> >+      cls = kzalloc(sizeof(struct class), GFP_KERNEL);
> >
> >
> maybe, the better way is to write `*cls' instead of `struct class',
> better for further changes

Please note that some maintainers don't like it. I at least could not
sneak in patches like these to drivers/usb/ because I had changed
sizeof.

                                 Pekka

Re: [PATCH] use kzalloc instead of malloc+memset

[Dmitry Torokhov]

On Monday 12 September 2005 23:43, Pekka Enberg wrote:
> On 9/13/05, Jiri Slaby <jirislaby@gmail.com> wrote:
> > >-      cls = kmalloc(sizeof(struct class), GFP_KERNEL);
> > >+      cls = kzalloc(sizeof(struct class), GFP_KERNEL);
> > >
> > >
> > maybe, the better way is to write `*cls' instead of `struct class',
> > better for further changes
> 
> Please note that some maintainers don't like it. I at least could not
> sneak in patches like these to drivers/usb/ because I had changed
> sizeof.
> 

And given the fact that Greg maintains driver core it probably won't be
accepted here either :)

FWIW I also prefer spelling out the structure I am allocating.

-- 
Dmitry

Re: [PATCH] use kzalloc instead of malloc+memset

[Denis Vlasenko]

On Tuesday 13 September 2005 01:47, Lion Vollnhals wrote:
> On Tuesday 13 September 2005 00:10, Lion Vollnhals wrote:
> > This patch against 2.6.13-mm3 replaces malloc and memset with kzalloc in
> > drivers/base/class.c . 
> 
> The following patch converts further kmalllocs and memsets in drivers/base/* to kzallocs.
> 
> Please apply.
> 
> Signed-off-by: Lion Vollnhals <webmaster@schiggl.de>

> diff -Nurp 2.6.13-mm3/drivers/base/platform.c 2.6.13-mm3-changed/drivers/base/platform.c
> --- 2.6.13-mm3/drivers/base/platform.c	2005-09-12 23:42:47.000000000 +0200
> +++ 2.6.13-mm3-changed/drivers/base/platform.c	2005-09-13 00:23:43.000000000 +0200
> @@ -225,13 +225,12 @@ struct platform_device *platform_device_
>  	struct platform_object *pobj;
>  	int retval;
>  
> -	pobj = kmalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
> +	pobj = kzalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
>  	if (!pobj) {
>  		retval = -ENOMEM;
>  		goto error;
>  	}
>  
> -	memset(pobj, 0, sizeof(*pobj));

Was this a bug or did you just introduced one?
--
vda

Re: [PATCH] use kzalloc instead of malloc+memset

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> On Monday 12 September 2005 23:43, Pekka Enberg wrote:
> > On 9/13/05, Jiri Slaby <jirislaby@gmail.com> wrote:
> > > >-      cls = kmalloc(sizeof(struct class), GFP_KERNEL);
> > > >+      cls = kzalloc(sizeof(struct class), GFP_KERNEL);
> > > >
> > > >
> > > maybe, the better way is to write `*cls' instead of `struct class',
> > > better for further changes
> > 
> > Please note that some maintainers don't like it. I at least could not
> > sneak in patches like these to drivers/usb/ because I had changed
> > sizeof.
> > 
> 
> And given the fact that Greg maintains driver core it probably won't be
> accepted here either :)
> 
> FWIW I also prefer spelling out the structure I am allocating.
> 

It hurts readability.  Quick question: is this code correct?

	dev = kmalloc(sizeof(struct net_device), GFP_KERNEL);

you don't know.  You have to go hunting down the declaration of `dev' to
find out.

Re: [PATCH] use kzalloc instead of malloc+memset

[Pekka J Enberg]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > FWIW I also prefer spelling out the structure I am allocating.

On Mon, 12 Sep 2005, Andrew Morton wrote:
> It hurts readability.  Quick question: is this code correct?
> 
> 	dev = kmalloc(sizeof(struct net_device), GFP_KERNEL);
> 
> you don't know.  You have to go hunting down the declaration of `dev' to
> find out.

Andrew, how about something like this?

			Pekka

[PATCH] CodingStyle: memory allocation

This patch adds a new chapter on memory allocation to Documentation/CodingStyle.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 CodingStyle |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletion(-)

Index: 2.6-mm/Documentation/CodingStyle
===================================================================
--- 2.6-mm.orig/Documentation/CodingStyle
+++ 2.6-mm/Documentation/CodingStyle
@@ -410,7 +410,26 @@ Kernel messages do not have to be termin
 Printing numbers in parentheses (%d) adds no value and should be avoided.
 
 
-		Chapter 13: References
+		Chapter 13: Allocating memory
+
+The kernel provides the following general purpose memory allocators:
+kmalloc(), kzalloc(), kcalloc(), and vmalloc().  Please refer to the API
+documentation for further information about them.
+
+The preferred form for passing a size of a struct is the following:
+
+	p = kmalloc(sizeof(*p), ...);
+
+The alternative form where struct name is spelled out hurts readability and
+introduces an opportunity for a bug when the pointer variable type is changed
+but the corresponding sizeof that is passed to a memory allocator is not.
+
+Casting the return value which is a void pointer is redundant. The conversion
+from void pointer to any other pointer type is guaranteed by the C programming
+language.
+
+
+		Chapter 14: References
 
 The C Programming Language, Second Edition
 by Brian W. Kernighan and Dennis M. Ritchie.

Re: [PATCH] use kzalloc instead of malloc+memset

[Nikita Danilov]

Pekka J Enberg writes:

[...]

 > +
 > +The kernel provides the following general purpose memory allocators:
 > +kmalloc(), kzalloc(), kcalloc(), and vmalloc().  Please refer to the API
 > +documentation for further information about them.
 > +
 > +The preferred form for passing a size of a struct is the following:
 > +
 > +	p = kmalloc(sizeof(*p), ...);

Parentheses around *p are superfluous. See

 >   The C Programming Language, Second Edition
 >   by Brian W. Kernighan and Dennis M. Ritchie.

Nikita.

Re: [PATCH] use kzalloc instead of malloc+memset

[Jiri Slaby]

Denis Vlasenko napsal(a):

>On Tuesday 13 September 2005 01:47, Lion Vollnhals wrote:
>  
>
>>On Tuesday 13 September 2005 00:10, Lion Vollnhals wrote:
>>    
>>
>>>This patch against 2.6.13-mm3 replaces malloc and memset with kzalloc in
>>>drivers/base/class.c . 
>>>      
>>>
>>The following patch converts further kmalllocs and memsets in drivers/base/* to kzallocs.
>>
>>Please apply.
>>
>>Signed-off-by: Lion Vollnhals <webmaster@schiggl.de>
>>    
>>
>
>  
>
>>diff -Nurp 2.6.13-mm3/drivers/base/platform.c 2.6.13-mm3-changed/drivers/base/platform.c
>>--- 2.6.13-mm3/drivers/base/platform.c	2005-09-12 23:42:47.000000000 +0200
>>+++ 2.6.13-mm3-changed/drivers/base/platform.c	2005-09-13 00:23:43.000000000 +0200
>>@@ -225,13 +225,12 @@ struct platform_device *platform_device_
>> 	struct platform_object *pobj;
>> 	int retval;
>> 
>>-	pobj = kmalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
>>+	pobj = kzalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
>> 	if (!pobj) {
>> 		retval = -ENOMEM;
>> 		goto error;
>> 	}
>> 
>>-	memset(pobj, 0, sizeof(*pobj));
>>    
>>
>
>Was this a bug or did you just introduced one?
>  
>
Yes, this was a bug.

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] use kzalloc instead of malloc+memset

[Pekka Enberg]

Pekka J Enberg writes:
>  > +The kernel provides the following general purpose memory allocators:
>  > +kmalloc(), kzalloc(), kcalloc(), and vmalloc().  Please refer to the API
>  > +documentation for further information about them.
>  > +
>  > +The preferred form for passing a size of a struct is the following:
>  > +
>  > +	p = kmalloc(sizeof(*p), ...);

On Tue, 2005-09-13 at 11:42 +0400, Nikita Danilov wrote:
> Parentheses around *p are superfluous. See

Sure but is it the preferred form for the kernel?

			Pekka

Re: [PATCH] use kzalloc instead of malloc+memset

[Paul Jackson]

Andrew wrote:
> It hurts readability.  Quick question: is this code correct?
> 
> 	dev = kmalloc(sizeof(struct net_device), GFP_KERNEL);

And it hurts maintainability.  If someone changes 'dev' so
that it is no longer of type 'struct net_device', then they
risk missing this allocation, and introducing what could be
a nasty memory corruption kernel bug.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [PATCH] use kzalloc instead of malloc+memset

[Denis Vlasenko]

On Tuesday 13 September 2005 10:42, Nikita Danilov wrote:
> Pekka J Enberg writes:
> 
> [...]
> 
>  > +
>  > +The kernel provides the following general purpose memory allocators:
>  > +kmalloc(), kzalloc(), kcalloc(), and vmalloc().  Please refer to the API
>  > +documentation for further information about them.
>  > +
>  > +The preferred form for passing a size of a struct is the following:
>  > +
>  > +	p = kmalloc(sizeof(*p), ...);
> 
> Parentheses around *p are superfluous. See
> 
>  >   The C Programming Language, Second Edition
>  >   by Brian W. Kernighan and Dennis M. Ritchie.

I remember that sizeof has two forms: sizeof(type) and
sizeof(expr), and in one of them ()'s are optional.
But I fail to remember in which one. I use ()'s always.

Thanks for refreshing my memory but I'm sure
I'll forget again ;)
--
vda

Re: [PATCH] use kzalloc instead of malloc+memset

[Bernd Petrovitsch]

On Tue, 2005-09-13 at 16:28 -0700, Paul Jackson wrote:
> Andrew wrote:
> > It hurts readability.  Quick question: is this code correct?
> > 
> > 	dev = kmalloc(sizeof(struct net_device), GFP_KERNEL);
> 
> And it hurts maintainability.  If someone changes 'dev' so
> that it is no longer of type 'struct net_device', then they
                                'struct net_device *'
> risk missing this allocation, and introducing what could be
> a nasty memory corruption kernel bug.

SCNR,
	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services

Re: [PATCH] use kzalloc instead of malloc+memset

[Nikita Danilov]

Denis Vlasenko writes:

[...]

 > 
 > I remember that sizeof has two forms: sizeof(type) and
 > sizeof(expr), and in one of them ()'s are optional.
 > But I fail to remember in which one. I use ()'s always.

Formally speaking, sizeof have forms

        sizeof(type), and

        sizeof expr

it is just that expression can usually be wrapped into parentheses, like
(((((0))))).

 > 
 > Thanks for refreshing my memory but I'm sure
 > I'll forget again ;)

That's why we need more instances of sizeof expr in the kernel code, to
keep your knowledge of C afresh all the time. :-)

Note that Linux doesn't follow a custom of... some other kernels to
parenthesize everything to the heretical extent of writing 

   if ((a == 0) && (b == 1))

or

   return (foo);

 > --
 > vda

Nikita.