/lib/kobject.c

PATCH In review Christophe Jaillet 4 participants 5 messages Mar 8, 2010 – Mar 9, 2010

Subsystems (3)

Kernel janitors Library code Driver core

lore

Share & tools

lore

Christophe Jaillet

author

2010-03-08 21:21 UTC #0 lore raw

PATCH 1/1 /lib/kobject.c

From: Christophe Jaillet <[email protected]>

Hi, here is a patch against /lib/kobject.c.

No need to allocate more memory than usefull.
Here, we only need 12 (strlen("DEVPATH_OLD=")) + 1 (\0) extra bytes. This saves 2 bytes !!! and improve readability IMO.

Signed-off-by Christophe Jaillet <[email protected]>

---

lib/kobject.c +0 -0

index 0487d1f..958cc76 100644
@@ -412,7 +412,7 @@ int kobject_rename(struct kobject *kobj, const char

*new_name)
error = -ENOMEM;
goto out;
}
- devpath_string = kmalloc(strlen(devpath) + 15, GFP_KERNEL);
+ devpath_string = kmalloc(12 + strlen(devpath) + 1, GFP_KERNEL); if (!devpath_string) {
error = -ENOMEM;
goto out;

Greg KH

2010-03-08 21:35 UTC #1 lore raw

Quoted 7 lines · Christophe Jaillet

From: Christophe Jaillet <[email protected]>
Hi, here is a patch against /lib/kobject.c.
No need to allocate more memory than usefull.
Here, we only need 12 (strlen("DEVPATH_OLD=")) + 1 (\0) extra bytes. This saves 2 bytes !!! and improve readability IMO.

Are you sure this really is even noticable anywhere? The memory is then freed, right? So no real memory savings happen here.

Quoted 11 lines

Signed-off-by Christophe Jaillet <[email protected]>
---
lib/kobject.c +0 -0
index 0487d1f..958cc76 100644

lib/kobject.c int kobject_rename(struct kobject *kobj, const char · line 414

Quoted 3 lines

lib/kobject.c +0 -0

@@ -412,7 +412,7 @@ int kobject_rename(struct kobject *kobj, const char
 *new_name)
   error = -ENOMEM;
   goto out;

Your patch is line-wrapped and the tabs converted to spaces, making it impossible to apply :(

Quoted 13 lines

lib/kobject.c +1 -1
  }
- devpath_string = kmalloc(strlen(devpath) + 15, GFP_KERNEL);
+ devpath_string = kmalloc(12 + strlen(devpath) + 1, GFP_KERNEL);
  if (!devpath_string) {
   error = -ENOMEM;
   goto out;
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

walter harms

2010-03-09 17:35 UTC #2 lore raw

In reply to Christophe Jaillet

Quoted 17 lines · Greg KH

Quoted 7 lines · Christophe Jaillet
From: Christophe Jaillet <[email protected]>
Hi, here is a patch against /lib/kobject.c.
No need to allocate more memory than usefull.
Here, we only need 12 (strlen("DEVPATH_OLD=")) + 1 (\0) extra bytes. This saves 2 bytes !!! and improve readability IMO.
Are you sure this really is even noticable anywhere? The memory is then freed, right? So no real memory savings happen here.
Quoted 5 lines
Signed-off-by Christophe Jaillet <[email protected]>
---

Quoted 20 lines

lib/kobject.c int kobject_rename(struct kobject *kobj, const char · line 414
Quoted 8 lines
lib/kobject.c +0 -0
index 0487d1f..958cc76 100644
@@ -412,7 +412,7 @@ int kobject_rename(struct kobject *kobj, const char
 *new_name)
   error = -ENOMEM;
   goto out;
Your patch is line-wrapped and the tabs converted to spaces, making it impossible to apply :(
Quoted 8 lines
lib/kobject.c +1 -1
  }
- devpath_string = kmalloc(strlen(devpath) + 15, GFP_KERNEL);
+ devpath_string = kmalloc(12 + strlen(devpath) + 1, GFP_KERNEL);
  if (!devpath_string) {
   error = -ENOMEM;
   goto out;

this is actualy done:
sprintf(devpath_string, "DEVPATH_OLD=%s", devpath);

i have seen something that works like asprintf() devpath_string = kasprintf(GFP_KERNEL,"DEVPATH_OLD=%s",devpath); just put it in place of the kmalloc
The interessting question is:
can devpath be manipulated to be VERY large at some point ?

@julia:
i did not check but i can imagine that the pattern A=kmalloc() ... sprintf(a," ",b);

come up at some points inside the kernel since a lot of programmers do not know about asprint().

re,
wh

Greg KH

2010-03-09 18:12 UTC #3 lore raw

In reply to Christophe Jaillet

Quoted 41 lines · walter harms

Quoted 17 lines · Greg KH
Quoted 7 lines · Christophe Jaillet
From: Christophe Jaillet <[email protected]>
Hi, here is a patch against /lib/kobject.c.
No need to allocate more memory than usefull.
Here, we only need 12 (strlen("DEVPATH_OLD=")) + 1 (\0) extra bytes. This saves 2 bytes !!! and improve readability IMO.
Are you sure this really is even noticable anywhere? The memory is then freed, right? So no real memory savings happen here.
Quoted 5 lines
Signed-off-by Christophe Jaillet <[email protected]>
---
Quoted 20 lines
lib/kobject.c int kobject_rename(struct kobject *kobj, const char · line 414
Quoted 8 lines
lib/kobject.c +0 -0
index 0487d1f..958cc76 100644
@@ -412,7 +412,7 @@ int kobject_rename(struct kobject *kobj, const char
 *new_name)
   error = -ENOMEM;
   goto out;
Your patch is line-wrapped and the tabs converted to spaces, making it impossible to apply :(
Quoted 8 lines
lib/kobject.c +1 -1
  }
- devpath_string = kmalloc(strlen(devpath) + 15, GFP_KERNEL);
+ devpath_string = kmalloc(12 + strlen(devpath) + 1, GFP_KERNEL);
  if (!devpath_string) {
   error = -ENOMEM;
   goto out;

Quoted 8 lines

this is actualy done:
sprintf(devpath_string, "DEVPATH_OLD=%s", devpath);
i have seen something that works like asprintf() devpath_string = kasprintf(GFP_KERNEL,"DEVPATH_OLD=%s",devpath); just put it in place of the kmalloc
The interessting question is:
can devpath be manipulated to be VERY large at some point ?

Possibly, yes. But on systems where is is very large, you usually have very large mounts of memory :)

Either way, you can't manipulate it except if you are root, so all is fine.

thanks,

greg k-h

Julia Lawall

2010-03-09 18:49 UTC #4 lore raw

In reply to Christophe Jaillet

Quoted 50 lines · walter harms

Quoted 17 lines · Greg KH
Quoted 7 lines · Christophe Jaillet
From: Christophe Jaillet <[email protected]>
Hi, here is a patch against /lib/kobject.c.
No need to allocate more memory than usefull.
Here, we only need 12 (strlen("DEVPATH_OLD=")) + 1 (\0) extra bytes. This saves 2 bytes !!! and improve readability IMO.
Are you sure this really is even noticable anywhere? The memory is then freed, right? So no real memory savings happen here.
Quoted 5 lines
Signed-off-by Christophe Jaillet <[email protected]>
---
Quoted 20 lines
lib/kobject.c int kobject_rename(struct kobject *kobj, const char · line 414
Quoted 8 lines
lib/kobject.c +0 -0
index 0487d1f..958cc76 100644
@@ -412,7 +412,7 @@ int kobject_rename(struct kobject *kobj, const char
 *new_name)
   error = -ENOMEM;
   goto out;
Your patch is line-wrapped and the tabs converted to spaces, making it impossible to apply :(
Quoted 8 lines
lib/kobject.c +1 -1
  }
- devpath_string = kmalloc(strlen(devpath) + 15, GFP_KERNEL);
+ devpath_string = kmalloc(12 + strlen(devpath) + 1, GFP_KERNEL);
  if (!devpath_string) {
   error = -ENOMEM;
   goto out;
this is actualy done:
sprintf(devpath_string, "DEVPATH_OLD=%s", devpath);
i have seen something that works like asprintf() devpath_string = kasprintf(GFP_KERNEL,"DEVPATH_OLD=%s",devpath); just put it in place of the kmalloc
The interessting question is:
can devpath be manipulated to be VERY large at some point ?

Quoted 6 lines

@julia:
i did not check but i can imagine that the pattern A=kmalloc() ... sprintf(a," ",b);
come up at some points inside the kernel since a lot of programmers do not know about asprint().

Indeed, I find around 40 occurrences, such as:

diff -u -p /var/linuxes/linux-next/arch/ia64/pci/pci.c /tmp/nothing
--- /var/linuxes/linux-next/arch/ia64/pci/pci.c 2010-03-01 07:52:48.000000000 +0
100
+++ /tmp/nothing
@@ -366,11 +366,9 @@ pci_acpi_scan_root(struct acpi_device *d
if (!controller->window)
goto out2;

- name = kmalloc(16, GFP_KERNEL);
if (!name)
goto out3;

- sprintf(name, "PCI Bus %04x:%02x", domain, bus);
info.bridge = device;
info.controller = controller;
info.name = name;

(- means "item of interest", not necessarily something to remove)

julia