[PATCH 35/37] drivers/block/aoe: Use kmemdup

[PATCH 35/37] drivers/block/aoe: Use kmemdup

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Use kmemdup when some other buffer is immediately copied into the
allocated region.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression from,to,size,flag;
statement S;
@@

-  to = \(kmalloc\|kzalloc\)(size,flag);
+  to = kmemdup(from,size,flag);
   if (to==NULL || ...) S
-  memcpy(to, from, size);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/block/aoe/aoechr.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -u -p a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
--- a/drivers/block/aoe/aoechr.c
+++ b/drivers/block/aoe/aoechr.c
@@ -132,13 +132,12 @@ bail:		spin_unlock_irqrestore(&emsgs_loc
 		return;
 	}
 
-	mp = kmalloc(n, GFP_ATOMIC);
+	mp = kmemdup(msg, n, GFP_ATOMIC);
 	if (mp == NULL) {
 		printk(KERN_ERR "aoe: allocation failure, len=%ld\n", n);
 		goto bail;
 	}
 
-	memcpy(mp, msg, n);
 	em->msg = mp;
 	em->flags |= EMFL_VALID;
 	em->len = n;

Re: [PATCH 35/37] drivers/block/aoe: Use kmemdup

[Ed Cashin]

On May 15, 2010, at 5:23 PM, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> Use kmemdup when some other buffer is immediately copied into the
> allocated region.

I have seen this patch but have no comment about it specifically.

(Because the argument about whether this kind of change
adds value in general has already taken place.  I'd say no, but that's 
based on a maybe faulty idea that if folks weren't doing this they'd have
more time for reviewing patches, etc.)

-- 
  Ed Cashin
  ecashin@coraid.com

Re: [PATCH 35/37] drivers/block/aoe: Use kmemdup

[Dan Carpenter]

On Sun, May 16, 2010 at 08:42:59AM -0400, Ed Cashin wrote:
> On May 15, 2010, at 5:23 PM, Julia Lawall wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Use kmemdup when some other buffer is immediately copied into the
> > allocated region.
> 
> I have seen this patch but have no comment about it specifically.
> 
> (Because the argument about whether this kind of change
> adds value in general has already taken place.  I'd say no, but that's 
> based on a maybe faulty idea that if folks weren't doing this they'd have
> more time for reviewing patches, etc.)

Nah nah.  Nit-picky patches like this is what you get when people start
reviewing code.  It's really hard to review code properly without being
annoyed and writing clean up patches is very calming.

git log --pretty=oneline --grep=Lawall

Tons and tons of bugs fixed.

regards,
dan carpenter

Re: [PATCH 35/37] drivers/block/aoe: Use kmemdup

[Julia Lawall]

On Sun, 16 May 2010, Dan Carpenter wrote:

> On Sun, May 16, 2010 at 08:42:59AM -0400, Ed Cashin wrote:
> > On May 15, 2010, at 5:23 PM, Julia Lawall wrote:
> > 
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > Use kmemdup when some other buffer is immediately copied into the
> > > allocated region.
> > 
> > I have seen this patch but have no comment about it specifically.
> > 
> > (Because the argument about whether this kind of change
> > adds value in general has already taken place.  I'd say no, but that's 
> > based on a maybe faulty idea that if folks weren't doing this they'd have
> > more time for reviewing patches, etc.)
> 
> Nah nah.  Nit-picky patches like this is what you get when people start
> reviewing code.  It's really hard to review code properly without being
> annoyed and writing clean up patches is very calming.

Actually, someone asked me to convery a kmalloc to kstrdup, and I saw 
kmemdup nearby, and thought why not...  There is also the similar 
memdup_user.  Using that would more significantly simplify the code, in 
particular reducing the amount of error handling code, since it 
encompasses two calls that can fail (kmalloc and copy_from_user).

julia

Re: [PATCH 35/37] drivers/block/aoe: Use kmemdup

[Pekka Enberg]

On Sun, May 16, 2010 at 3:42 PM, Ed Cashin <ecashin@coraid.com> wrote:
> (Because the argument about whether this kind of change
> adds value in general has already taken place.  I'd say no, but that's
> based on a maybe faulty idea that if folks weren't doing this they'd have
> more time for reviewing patches, etc.)

It makes kernel text size smaller so yes, it adds value.