* Re: [patch] /lib/kobject.c
2010-03-08 21:21 [patch] /lib/kobject.c Christophe Jaillet
@ 2010-03-08 21:35 ` Greg KH
2010-03-09 17:35 ` walter harms
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2010-03-08 21:35 UTC (permalink / raw)
To: kernel-janitors
On Mon, Mar 08, 2010 at 10:21:20PM +0100, Christophe Jaillet wrote:
> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>
> 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.
>
>
> Signed-off-by: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>
> ---
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 0487d1f..958cc76 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -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 :(
> }
> - 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] /lib/kobject.c
2010-03-08 21:21 [patch] /lib/kobject.c Christophe Jaillet
2010-03-08 21:35 ` Greg KH
@ 2010-03-09 17:35 ` walter harms
2010-03-09 18:12 ` Greg KH
2010-03-09 18:49 ` Julia Lawall
3 siblings, 0 replies; 5+ messages in thread
From: walter harms @ 2010-03-09 17:35 UTC (permalink / raw)
To: kernel-janitors
Greg KH schrieb:
> On Mon, Mar 08, 2010 at 10:21:20PM +0100, Christophe Jaillet wrote:
>> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>>
>> 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.
>
>>
>> Signed-off-by: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>>
>> ---
>>
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index 0487d1f..958cc76 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -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 :(
>
>> }
>> - 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] /lib/kobject.c
2010-03-08 21:21 [patch] /lib/kobject.c Christophe Jaillet
2010-03-08 21:35 ` Greg KH
2010-03-09 17:35 ` walter harms
@ 2010-03-09 18:12 ` Greg KH
2010-03-09 18:49 ` Julia Lawall
3 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2010-03-09 18:12 UTC (permalink / raw)
To: kernel-janitors
On Tue, Mar 09, 2010 at 06:35:27PM +0100, walter harms wrote:
>
>
> Greg KH schrieb:
> > On Mon, Mar 08, 2010 at 10:21:20PM +0100, Christophe Jaillet wrote:
> >> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> >>
> >> 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.
> >
> >>
> >> Signed-off-by: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> >>
> >> ---
> >>
> >> diff --git a/lib/kobject.c b/lib/kobject.c
> >> index 0487d1f..958cc76 100644
> >> --- a/lib/kobject.c
> >> +++ b/lib/kobject.c
> >> @@ -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 :(
> >
> >> }
> >> - 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 ?
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] /lib/kobject.c
2010-03-08 21:21 [patch] /lib/kobject.c Christophe Jaillet
` (2 preceding siblings ...)
2010-03-09 18:12 ` Greg KH
@ 2010-03-09 18:49 ` Julia Lawall
3 siblings, 0 replies; 5+ messages in thread
From: Julia Lawall @ 2010-03-09 18:49 UTC (permalink / raw)
To: kernel-janitors
On Tue, 9 Mar 2010, walter harms wrote:
>
>
> Greg KH schrieb:
> > On Mon, Mar 08, 2010 at 10:21:20PM +0100, Christophe Jaillet wrote:
> >> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> >>
> >> 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.
> >
> >>
> >> Signed-off-by: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> >>
> >> ---
> >>
> >> diff --git a/lib/kobject.c b/lib/kobject.c
> >> index 0487d1f..958cc76 100644
> >> --- a/lib/kobject.c
> >> +++ b/lib/kobject.c
> >> @@ -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 :(
> >
> >> }
> >> - 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().
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
^ permalink raw reply [flat|nested] 5+ messages in thread