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 <[email protected]>
--- 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;
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 <[email protected]>
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;
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 <[email protected]>
>
>--- 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 http://www.fi.muni.cz/~xslaby
~\-/~ [email protected] ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10
On 9/13/05, Jiri Slaby <[email protected]> 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
On Monday 12 September 2005 23:43, Pekka Enberg wrote:
> On 9/13/05, Jiri Slaby <[email protected]> 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
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 <[email protected]>
> 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
Dmitry Torokhov <[email protected]> wrote:
>
> On Monday 12 September 2005 23:43, Pekka Enberg wrote:
> > On 9/13/05, Jiri Slaby <[email protected]> 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.
Dmitry Torokhov <[email protected]> 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 <[email protected]>
---
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.
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 <[email protected]>
>>
>>
>
>
>
>>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 http://www.fi.muni.cz/~xslaby
~\-/~ [email protected] ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10
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.
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
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 <[email protected]> 1.925.600.0401
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
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
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.