* [PATCH v1] platform/x86: wmi: Replace kmalloc + sprintf() with kasprintf()
@ 2018-02-16 15:40 Andy Shevchenko
2018-02-16 15:55 ` David Laight
0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2018-02-16 15:40 UTC (permalink / raw)
To: Darren Hart, platform-driver-x86, linux-kernel; +Cc: Andy Shevchenko
kasprintf() does the job of two: kmalloc() and sprintf().
Replace two calls with one.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/platform/x86/wmi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index c0c8945603cb..4e76ffcb5394 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -933,12 +933,11 @@ static int wmi_dev_probe(struct device *dev)
goto probe_failure;
}
- buf = kmalloc(strlen(wdriver->driver.name) + 5, GFP_KERNEL);
+ buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
if (!buf) {
ret = -ENOMEM;
goto probe_string_failure;
}
- sprintf(buf, "wmi/%s", wdriver->driver.name);
wblock->char_dev.minor = MISC_DYNAMIC_MINOR;
wblock->char_dev.name = buf;
wblock->char_dev.fops = &wmi_fops;
--
2.16.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH v1] platform/x86: wmi: Replace kmalloc + sprintf() with kasprintf()
2018-02-16 15:40 [PATCH v1] platform/x86: wmi: Replace kmalloc + sprintf() with kasprintf() Andy Shevchenko
@ 2018-02-16 15:55 ` David Laight
2018-02-16 19:30 ` Andy Shevchenko
2018-02-16 19:54 ` Darren Hart
0 siblings, 2 replies; 4+ messages in thread
From: David Laight @ 2018-02-16 15:55 UTC (permalink / raw)
To: 'Andy Shevchenko',
Darren Hart, platform-driver-x86, linux-kernel
From: Andy Shevchenko
> Sent: 16 February 2018 15:40
>
> kasprintf() does the job of two: kmalloc() and sprintf().
> Replace two calls with one.
...
> - buf = kmalloc(strlen(wdriver->driver.name) + 5, GFP_KERNEL);
> + buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
...
Except that kasprintf() has no idea how long a buffer is needed.
It might even do the printf twice just to get the length.
David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] platform/x86: wmi: Replace kmalloc + sprintf() with kasprintf()
2018-02-16 15:55 ` David Laight
@ 2018-02-16 19:30 ` Andy Shevchenko
2018-02-16 19:54 ` Darren Hart
1 sibling, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2018-02-16 19:30 UTC (permalink / raw)
To: David Laight
Cc: Andy Shevchenko, Darren Hart, platform-driver-x86, linux-kernel
On Fri, Feb 16, 2018 at 5:55 PM, David Laight <David.Laight@aculab.com> wrote:
>> kasprintf() does the job of two: kmalloc() and sprintf().
>> Replace two calls with one.
> ...
>> - buf = kmalloc(strlen(wdriver->driver.name) + 5, GFP_KERNEL);
>> + buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
> ...
>
> Except that kasprintf() has no idea how long a buffer is needed.
Hmm...
> It might even do the printf twice just to get the length.
It does exactly that. So what?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] platform/x86: wmi: Replace kmalloc + sprintf() with kasprintf()
2018-02-16 15:55 ` David Laight
2018-02-16 19:30 ` Andy Shevchenko
@ 2018-02-16 19:54 ` Darren Hart
1 sibling, 0 replies; 4+ messages in thread
From: Darren Hart @ 2018-02-16 19:54 UTC (permalink / raw)
To: David Laight; +Cc: 'Andy Shevchenko', platform-driver-x86, linux-kernel
On Fri, Feb 16, 2018 at 03:55:24PM +0000, David Laight wrote:
> From: Andy Shevchenko
> > Sent: 16 February 2018 15:40
> >
> > kasprintf() does the job of two: kmalloc() and sprintf().
> > Replace two calls with one.
> ...
> > - buf = kmalloc(strlen(wdriver->driver.name) + 5, GFP_KERNEL);
> > + buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
> ...
>
> Except that kasprintf() has no idea how long a buffer is needed.
> It might even do the printf twice just to get the length.
Sure, but this is one-time and non-critical path. Unfortunately there
isn't any guidance in Documentation here. Of the 520 or so instances of
kasprintf usages in the kernel, device name or similar is a very common
pattern. Eliminating manual counting of characters for buffer allocation
seems like a good plan to me.
So unless we want to argue that all those use cases are wrong, this
would appear to be at least common practice, if not best practice for
this particular pattern.
Reviewed-by: Darren Hart (VMware) <dvhart@infradead.org>
--
Darren Hart
VMware Open Source Technology Center
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-16 19:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-16 15:40 [PATCH v1] platform/x86: wmi: Replace kmalloc + sprintf() with kasprintf() Andy Shevchenko
2018-02-16 15:55 ` David Laight
2018-02-16 19:30 ` Andy Shevchenko
2018-02-16 19:54 ` Darren Hart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.