[PATCH 0/3] Use kasprintf

[PATCH 0/3] Use kasprintf

[Julia Lawall]

These patches convert a sequence of kmalloc and memcpy to use kasprintf
instead.

[PATCH 1/3] drivers/staging/cx25821: Use kasprintf

[Julia Lawall]

Rewrite the initialization of a dev field.  In the original code, in each
case there was a kmalloc followed by a memcpy, as illustrated by the
semantic patch below.  In the case that the provided string was the empty
string, the allocated memory was then overwritten with a constant string,
causing a memory leak.  Finally, there was no provision for returning
-ENOMEM in case of failure of the memory allocation.  Indeed, the return
value in an error case was err, a variable that was never initialized to
anything other than 0.

The following patch rewrites the above code to first select a string based
on various conditions, and then to copy it into a newly allocated memory
region, using kasprintf.  This decreases subtantially the code size
and removes the memory leak.  The instruction for getting the length of the
string and the associated variable declaration are also deleted.

The patch also drops err, changes the return value to retval, which in each
file was already initialized elsewhere to an error code, and initializes
retval to -ENOMEM when kasprintf fails.

The semantic patch that motivated this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  len = strlen(arg)
  ... when != len = e1
      when != arg = e2
  a =
-  \(kmalloc\|kzalloc\)(len+1,flag)
+  kasprintf(flag,"%s",arg)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
This patch makes quite a lot of changes and is only compile tested.

 drivers/staging/cx25821/cx25821-audio-upstream.c     |   36 +++---------
 drivers/staging/cx25821/cx25821-video-upstream-ch2.c |   57 +++++++------------
 drivers/staging/cx25821/cx25821-video-upstream.c     |   56 +++++++-----------
 3 files changed, 56 insertions(+), 93 deletions(-)

diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
index 27087db..a8f6343 100644
--- a/drivers/staging/cx25821/cx25821-audio-upstream.c
+++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
@@ -723,8 +723,7 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 {
 	struct sram_channel *sram_ch;
 	int retval = 0;
-	int err = 0;
-	int str_length = 0;
+	char *filename;
 
 	if (dev->_audio_is_running) {
 		printk(KERN_WARNING "Audio Channel is still running so return!\n");
@@ -752,28 +751,15 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
 	_line_size = AUDIO_LINE_SIZE;
 
-	if (dev->input_audiofilename) {
-		str_length = strlen(dev->input_audiofilename);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, dev->input_audiofilename,
-		       str_length + 1);
-
-		/* Default if filename is empty string */
-		if (strcmp(dev->input_audiofilename, "") == 0)
-			dev->_audiofilename = "/root/audioGOOD.wav";
-
-	} else {
-		str_length = strlen(_defaultAudioName);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
+	if (dev->input_audiofilename &&
+	    strcmp(dev->input_audiofilename, "") != 0)
+		filename = dev->input_audiofilename;
+	else
+		filename = _defaultAudioName;
+	dev->_audiofilename = kasprintf(GFP_KERNEL, "%s", filename);
+	if (!dev->_audiofilename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -802,5 +788,5 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
index d12dbb5..531e0c4 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
@@ -747,10 +747,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
+	char *filename;
 
 	if (dev->_is_running_ch2) {
 		printk("Video Channel is still running so return!\n");
@@ -789,38 +788,26 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	    dev->_isNTSC_ch2 ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
 	if (dev->input_filename_ch2) {
-		str_length = strlen(dev->input_filename_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->input_filename_ch2,
-		       str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->_defaultname_ch2,
-		       str_length + 1);
-	}
-
-       /* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") == 0) {
-		if (dev->_isNTSC_ch2) {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+		/* Default if filename is empty string */
+		if (strcmp(dev->input_filename_ch2, "") == 0) {
+			if (dev->_isNTSC_ch2)
+				filename =
+					(dev->_pixel_format_ch2 ==
+					PIXEL_FRMT_411) ? "/root/vid411.yuv" :
+					"/root/vidtest.yuv";
+			else
+				filename =
+					(dev->_pixel_format_ch2 ==
+					PIXEL_FRMT_411) ? "/root/pal411.yuv" :
+					"/root/pal422.yuv";
+		} else
+			filename = dev->input_filename_ch2;
+	} else
+		filename = dev->_defaultname_ch2;
+	dev->_filename_ch2 = kasprintf(GFP_KERNEL, "%s", filename);
+	if (!dev->_filename_ch2) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -851,5 +838,5 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
       error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream.c b/drivers/staging/cx25821/cx25821-video-upstream.c
index 756a820..040f795 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream.c
@@ -800,10 +800,9 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
+	char *filename;
 
 	if (dev->_is_running) {
 		printk(KERN_INFO "Video Channel is still running so return!\n");
@@ -841,36 +840,27 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	    dev->_isNTSC ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
 	if (dev->input_filename) {
-		str_length = strlen(dev->input_filename);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->input_filename, str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->_defaultname, str_length + 1);
-	}
-
-	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") == 0) {
-		if (dev->_isNTSC) {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+		/* Default if filename is empty string */
+		if (strcmp(dev->input_filename, "") == 0) {
+			if (dev->_isNTSC)
+				filename =
+					(dev->_pixel_format ==
+					PIXEL_FRMT_411) ? "/root/vid411.yuv" :
+					"/root/vidtest.yuv";
+			else
+				filename =
+					(dev->_pixel_format ==
+					PIXEL_FRMT_411) ? "/root/pal411.yuv" :
+					"/root/pal422.yuv";
+		} else
+			filename = dev->input_filename;
+	} else
+		filename = dev->_defaultname;
+
+	dev->_filename = kasprintf(GFP_KERNEL, "%s", filename);
+	if (!dev->_filename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	dev->_is_running = 0;
@@ -908,5 +898,5 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }

[PATCH 2/3] fs/ceph/xattr.c: Use kasprintf

[Julia Lawall]

Convert a sequence of kmalloc and memcpy to use kasprintf.  The argument is
checked for being a string by the presence of a previous call to strlen.

The semantic patch that performs this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  len = strlen(arg)
  ... when != len = e1
      when != arg = e2
  a =
-  \(kmalloc\|kzalloc\)(len+1,flag)
+  kasprintf(flag,"%s",arg)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
 fs/ceph/xattr.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 70e9199..b8dd1f0 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -716,10 +716,9 @@ int ceph_setxattr(struct dentry *dentry, const char *name,
 
 	/* preallocate memory for xattr name, value, index node */
 	err = -ENOMEM;
-	newname = kmalloc(name_len + 1, GFP_NOFS);
+	newname = kasprintf(GFP_NOFS, "%s", name);
 	if (!newname)
 		goto out;
-	memcpy(newname, name, name_len + 1);
 
 	if (val_len) {
 		newval = kmalloc(val_len + 1, GFP_NOFS);

[PATCH 3/3] fs/jffs2/dir.c: Use kasprintf

[Julia Lawall]

Convert a sequence of kmalloc and memcpy to use kasprintf.  The argument is
checked for being a string by the presence of a previous call to strlen.

The semantic patch that performs this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  len = strlen(arg)
  ... when != len = e1
      when != arg = e2
  a =
-  \(kmalloc\|kzalloc\)(len+1,flag)
+  kasprintf(flag,"%s",arg)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
 fs/jffs2/dir.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index ed78a3c..8ee5675 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -367,7 +367,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 	}
 
 	/* We use f->target field to store the target path. */
-	f->target = kmalloc(targetlen + 1, GFP_KERNEL);
+	f->target = kasprintf(GFP_KERNEL, "%s", target);
 	if (!f->target) {
 		printk(KERN_WARNING "Can't allocate %d bytes of memory\n", targetlen + 1);
 		mutex_unlock(&f->sem);
@@ -376,7 +376,6 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 		goto fail;
 	}
 
-	memcpy(f->target, target, targetlen + 1);
 	D1(printk(KERN_DEBUG "jffs2_symlink: symlink's target '%s' cached\n", (char *)f->target));
 
 	/* No data here. Only a metadata node, which will be

Re: [PATCH 2/3] fs/ceph/xattr.c: Use kasprintf

[Joe Perches]

On Sun, 2010-10-17 at 20:48 +0200, Julia Lawall wrote:
> Convert a sequence of kmalloc and memcpy to use kasprintf.  The argument is
> checked for being a string by the presence of a previous call to strlen.
[]
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
[]
> @@ -716,10 +716,9 @@ int ceph_setxattr(struct dentry *dentry, const char *name,
>  
>  	/* preallocate memory for xattr name, value, index node */
>  	err = -ENOMEM;
> -	newname = kmalloc(name_len + 1, GFP_NOFS);
> +	newname = kasprintf(GFP_NOFS, "%s", name);

This one is probably better converted to kmemdup
as name_len is already known and is used later
in the routine.

kstrdup is also a possibility.

Re: [PATCH 2/3] fs/ceph/xattr.c: Use kasprintf

[Julia Lawall]

On Sun, 17 Oct 2010, Joe Perches wrote:

> On Sun, 2010-10-17 at 20:48 +0200, Julia Lawall wrote:
> > Convert a sequence of kmalloc and memcpy to use kasprintf.  The argument is
> > checked for being a string by the presence of a previous call to strlen.
> []
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> []
> > @@ -716,10 +716,9 @@ int ceph_setxattr(struct dentry *dentry, const char *name,
> >  
> >  	/* preallocate memory for xattr name, value, index node */
> >  	err = -ENOMEM;
> > -	newname = kmalloc(name_len + 1, GFP_NOFS);
> > +	newname = kasprintf(GFP_NOFS, "%s", name);
> 
> This one is probably better converted to kmemdup
> as name_len is already known and is used later
> in the routine.
> 
> kstrdup is also a possibility.

That calculates the length again, so I will change it to kmemdup.

julia

[PATCH 2/3] fs/ceph/xattr.c: Use kmemdup

[Julia Lawall]

Convert a sequence of kmalloc and memcpy to use kmemdup.

The semantic patch that performs this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  a =
-  \(kmalloc\|kzalloc\)(len,flag)
+  kmemdup(arg,len,flag)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
 fs/ceph/xattr.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -u -p a/fs/ceph/xattr.c b/fs/ceph/xattr.c
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -716,10 +716,9 @@ int ceph_setxattr(struct dentry *dentry,
 
 	/* preallocate memory for xattr name, value, index node */
 	err = -ENOMEM;
-	newname = kmalloc(name_len + 1, GFP_NOFS);
+	newname = kmemdup(name, name_len + 1, GFP_NOFS);
 	if (!newname)
 		goto out;
-	memcpy(newname, name, name_len + 1);
 
 	if (val_len) {
 		newval = kmalloc(val_len + 1, GFP_NOFS);

[PATCH 3/3] fs/jffs2/dir.c: Use kmemdup

[Julia Lawall]

Convert a sequence of kmalloc and memcpy to use kmemdup.

The semantic patch that performs this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  a =
-  \(kmalloc\|kzalloc\)(len,flag)
+  kmemdup(arg,len,flag)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
 fs/jffs2/dir.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -u -p a/fs/jffs2/dir.c b/fs/jffs2/dir.c
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -367,7 +367,7 @@ static int jffs2_symlink (struct inode *
 	}
 
 	/* We use f->target field to store the target path. */
-	f->target = kmalloc(targetlen + 1, GFP_KERNEL);
+	f->target = kmemdup(target, targetlen + 1, GFP_KERNEL);
 	if (!f->target) {
 		printk(KERN_WARNING "Can't allocate %d bytes of memory\n", targetlen + 1);
 		mutex_unlock(&f->sem);
@@ -376,7 +376,6 @@ static int jffs2_symlink (struct inode *
 		goto fail;
 	}
 
-	memcpy(f->target, target, targetlen + 1);
 	D1(printk(KERN_DEBUG "jffs2_symlink: symlink's target '%s' cached\n", (char *)f->target));
 
 	/* No data here. Only a metadata node, which will be

Re: [PATCH 2/3] fs/ceph/xattr.c: Use kmemdup

[Sage Weil]

Thanks, applied!

sage


On Sun, 17 Oct 2010, Julia Lawall wrote:

> Convert a sequence of kmalloc and memcpy to use kmemdup.
> 
> The semantic patch that performs this transformation is:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression a,flag,len;
> expression arg,e1,e2;
> statement S;
> @@
> 
>   a =
> -  \(kmalloc\|kzalloc\)(len,flag)
> +  kmemdup(arg,len,flag)
>   <... when != a
>   if (a == NULL || ...) S
>   ...>
> - memcpy(a,arg,len+1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  fs/ceph/xattr.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff -u -p a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -716,10 +716,9 @@ int ceph_setxattr(struct dentry *dentry,
>  
>  	/* preallocate memory for xattr name, value, index node */
>  	err = -ENOMEM;
> -	newname = kmalloc(name_len + 1, GFP_NOFS);
> +	newname = kmemdup(name, name_len + 1, GFP_NOFS);
>  	if (!newname)
>  		goto out;
> -	memcpy(newname, name, name_len + 1);
>  
>  	if (val_len) {
>  		newval = kmalloc(val_len + 1, GFP_NOFS);
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 3/3] fs/jffs2/dir.c: Use kmemdup

[Artem Bityutskiy]

On Sun, 2010-10-17 at 21:56 +0200, Julia Lawall wrote:
> Convert a sequence of kmalloc and memcpy to use kmemdup.
> 
> The semantic patch that performs this transformation is:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression a,flag,len;
> expression arg,e1,e2;
> statement S;
> @@
> 
>   a =
> -  \(kmalloc\|kzalloc\)(len,flag)
> +  kmemdup(arg,len,flag)
>   <... when != a
>   if (a == NULL || ...) S
>   ...>
> - memcpy(a,arg,len+1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  fs/jffs2/dir.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Picked this one and put to l2-mtd-2.6.git, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/3] drivers/staging/cx25821: Use kasprintf

[walter harms]

Julia Lawall schrieb:
> Rewrite the initialization of a dev field.  In the original code, in each
> case there was a kmalloc followed by a memcpy, as illustrated by the
> semantic patch below.  In the case that the provided string was the empty
> string, the allocated memory was then overwritten with a constant string,
> causing a memory leak.  Finally, there was no provision for returning
> -ENOMEM in case of failure of the memory allocation.  Indeed, the return
> value in an error case was err, a variable that was never initialized to
> anything other than 0.
> 
> The following patch rewrites the above code to first select a string based
> on various conditions, and then to copy it into a newly allocated memory
> region, using kasprintf.  This decreases subtantially the code size
> and removes the memory leak.  The instruction for getting the length of the
> string and the associated variable declaration are also deleted.
> 
> The patch also drops err, changes the return value to retval, which in each
> file was already initialized elsewhere to an error code, and initializes
> retval to -ENOMEM when kasprintf fails.
> 
> The semantic patch that motivated this transformation is:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression a,flag,len;
> expression arg,e1,e2;
> statement S;
> @@
> 
>   len = strlen(arg)
>   ... when != len = e1
>       when != arg = e2
>   a =
> -  \(kmalloc\|kzalloc\)(len+1,flag)
> +  kasprintf(flag,"%s",arg)
>   <... when != a
>   if (a == NULL || ...) S
>   ...>
> - memcpy(a,arg,len+1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
> This patch makes quite a lot of changes and is only compile tested.
> 
>  drivers/staging/cx25821/cx25821-audio-upstream.c     |   36 +++---------
>  drivers/staging/cx25821/cx25821-video-upstream-ch2.c |   57 +++++++------------
>  drivers/staging/cx25821/cx25821-video-upstream.c     |   56 +++++++-----------
>  3 files changed, 56 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
> index 27087db..a8f6343 100644
> --- a/drivers/staging/cx25821/cx25821-audio-upstream.c
> +++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
> @@ -723,8 +723,7 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>  {
>  	struct sram_channel *sram_ch;
>  	int retval = 0;
> -	int err = 0;
> -	int str_length = 0;
> +	char *filename;
>  
>  	if (dev->_audio_is_running) {
>  		printk(KERN_WARNING "Audio Channel is still running so return!\n");
> @@ -752,28 +751,15 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>  	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
>  	_line_size = AUDIO_LINE_SIZE;
>  
> -	if (dev->input_audiofilename) {
> -		str_length = strlen(dev->input_audiofilename);
> -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_audiofilename)
> -			goto error;
> -
> -		memcpy(dev->_audiofilename, dev->input_audiofilename,
> -		       str_length + 1);
> -
> -		/* Default if filename is empty string */
> -		if (strcmp(dev->input_audiofilename, "") == 0)
> -			dev->_audiofilename = "/root/audioGOOD.wav";
> -
> -	} else {
> -		str_length = strlen(_defaultAudioName);
> -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_audiofilename)
> -			goto error;
> -
> -		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
> +	if (dev->input_audiofilename &&
> +	    strcmp(dev->input_audiofilename, "") != 0)
> +		filename = dev->input_audiofilename;
> +	else
> +		filename = _defaultAudioName;
> +	dev->_audiofilename = kasprintf(GFP_KERNEL, "%s", filename);
> +	if (!dev->_audiofilename) {
> +		retval = -ENOMEM;
> +		goto error;
>  	}
>

Is filename needed here at all ?
The if statement looks strange this looks more familar to me:
if (!dev->input_audiofilename || *dev->input_audiofilename==0)
	filename = _defaultAudioName;
else
	filename = dev->input_audiofilename;

re
 wh


>  	retval =
> @@ -802,5 +788,5 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>  error:
>  	cx25821_dev_unregister(dev);
>  
> -	return err;
> +	return retval;
>  }
> diff --git a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
> index d12dbb5..531e0c4 100644
> --- a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
> +++ b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
> @@ -747,10 +747,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
>  	struct sram_channel *sram_ch;
>  	u32 tmp;
>  	int retval = 0;
> -	int err = 0;
>  	int data_frame_size = 0;
>  	int risc_buffer_size = 0;
> -	int str_length = 0;
> +	char *filename;
>  
>  	if (dev->_is_running_ch2) {
>  		printk("Video Channel is still running so return!\n");
> @@ -789,38 +788,26 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
>  	    dev->_isNTSC_ch2 ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
>  
>  	if (dev->input_filename_ch2) {
> -		str_length = strlen(dev->input_filename_ch2);
> -		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename_ch2)
> -			goto error;
> -
> -		memcpy(dev->_filename_ch2, dev->input_filename_ch2,
> -		       str_length + 1);
> -	} else {
> -		str_length = strlen(dev->_defaultname_ch2);
> -		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename_ch2)
> -			goto error;
> -
> -		memcpy(dev->_filename_ch2, dev->_defaultname_ch2,
> -		       str_length + 1);
> -	}
> -
> -       /* Default if filename is empty string */
> -	if (strcmp(dev->input_filename_ch2, "") == 0) {
> -		if (dev->_isNTSC_ch2) {
> -			dev->_filename_ch2 =
> -			    (dev->_pixel_format_ch2 ==
> -			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
> -			    "/root/vidtest.yuv";
> -		} else {
> -			dev->_filename_ch2 =
> -			    (dev->_pixel_format_ch2 ==
> -			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
> -			    "/root/pal422.yuv";
> -		}
> +		/* Default if filename is empty string */
> +		if (strcmp(dev->input_filename_ch2, "") == 0) {
> +			if (dev->_isNTSC_ch2)
> +				filename =
> +					(dev->_pixel_format_ch2 ==
> +					PIXEL_FRMT_411) ? "/root/vid411.yuv" :
> +					"/root/vidtest.yuv";
> +			else
> +				filename =
> +					(dev->_pixel_format_ch2 ==
> +					PIXEL_FRMT_411) ? "/root/pal411.yuv" :
> +					"/root/pal422.yuv";
> +		} else
> +			filename = dev->input_filename_ch2;
> +	} else
> +		filename = dev->_defaultname_ch2;
> +	dev->_filename_ch2 = kasprintf(GFP_KERNEL, "%s", filename);
> +	if (!dev->_filename_ch2) {
> +		retval = -ENOMEM;
> +		goto error;
>  	}
>  
>  	retval =
> @@ -851,5 +838,5 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
>        error:
>  	cx25821_dev_unregister(dev);
>  
> -	return err;
> +	return retval;
>  }
> diff --git a/drivers/staging/cx25821/cx25821-video-upstream.c b/drivers/staging/cx25821/cx25821-video-upstream.c
> index 756a820..040f795 100644
> --- a/drivers/staging/cx25821/cx25821-video-upstream.c
> +++ b/drivers/staging/cx25821/cx25821-video-upstream.c
> @@ -800,10 +800,9 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
>  	struct sram_channel *sram_ch;
>  	u32 tmp;
>  	int retval = 0;
> -	int err = 0;
>  	int data_frame_size = 0;
>  	int risc_buffer_size = 0;
> -	int str_length = 0;
> +	char *filename;
>  
>  	if (dev->_is_running) {
>  		printk(KERN_INFO "Video Channel is still running so return!\n");
> @@ -841,36 +840,27 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
>  	    dev->_isNTSC ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
>  
>  	if (dev->input_filename) {
> -		str_length = strlen(dev->input_filename);
> -		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename)
> -			goto error;
> -
> -		memcpy(dev->_filename, dev->input_filename, str_length + 1);
> -	} else {
> -		str_length = strlen(dev->_defaultname);
> -		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename)
> -			goto error;
> -
> -		memcpy(dev->_filename, dev->_defaultname, str_length + 1);
> -	}
> -
> -	/* Default if filename is empty string */
> -	if (strcmp(dev->input_filename, "") == 0) {
> -		if (dev->_isNTSC) {
> -			dev->_filename =
> -			    (dev->_pixel_format ==
> -			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
> -			    "/root/vidtest.yuv";
> -		} else {
> -			dev->_filename =
> -			    (dev->_pixel_format ==
> -			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
> -			    "/root/pal422.yuv";
> -		}
> +		/* Default if filename is empty string */
> +		if (strcmp(dev->input_filename, "") == 0) {
> +			if (dev->_isNTSC)
> +				filename =
> +					(dev->_pixel_format ==
> +					PIXEL_FRMT_411) ? "/root/vid411.yuv" :
> +					"/root/vidtest.yuv";
> +			else
> +				filename =
> +					(dev->_pixel_format ==
> +					PIXEL_FRMT_411) ? "/root/pal411.yuv" :
> +					"/root/pal422.yuv";
> +		} else
> +			filename = dev->input_filename;
> +	} else
> +		filename = dev->_defaultname;
> +
> +	dev->_filename = kasprintf(GFP_KERNEL, "%s", filename);
> +	if (!dev->_filename) {
> +		retval = -ENOMEM;
> +		goto error;
>  	}
>  
>  	dev->_is_running = 0;
> @@ -908,5 +898,5 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
>  error:
>  	cx25821_dev_unregister(dev);
>  
> -	return err;
> +	return retval;
>  }
> 
> --
> 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
> 
> 

Re: [PATCH 1/3] drivers/staging/cx25821: Use kasprintf

[Julia Lawall]

On Mon, 18 Oct 2010, walter harms wrote:

> 
> 
> Julia Lawall schrieb:
> > Rewrite the initialization of a dev field.  In the original code, in each
> > case there was a kmalloc followed by a memcpy, as illustrated by the
> > semantic patch below.  In the case that the provided string was the empty
> > string, the allocated memory was then overwritten with a constant string,
> > causing a memory leak.  Finally, there was no provision for returning
> > -ENOMEM in case of failure of the memory allocation.  Indeed, the return
> > value in an error case was err, a variable that was never initialized to
> > anything other than 0.
> > 
> > The following patch rewrites the above code to first select a string based
> > on various conditions, and then to copy it into a newly allocated memory
> > region, using kasprintf.  This decreases subtantially the code size
> > and removes the memory leak.  The instruction for getting the length of the
> > string and the associated variable declaration are also deleted.
> > 
> > The patch also drops err, changes the return value to retval, which in each
> > file was already initialized elsewhere to an error code, and initializes
> > retval to -ENOMEM when kasprintf fails.
> > 
> > The semantic patch that motivated this transformation is:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression a,flag,len;
> > expression arg,e1,e2;
> > statement S;
> > @@
> > 
> >   len = strlen(arg)
> >   ... when != len = e1
> >       when != arg = e2
> >   a =
> > -  \(kmalloc\|kzalloc\)(len+1,flag)
> > +  kasprintf(flag,"%s",arg)
> >   <... when != a
> >   if (a == NULL || ...) S
> >   ...>
> > - memcpy(a,arg,len+1);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> > This patch makes quite a lot of changes and is only compile tested.
> > 
> >  drivers/staging/cx25821/cx25821-audio-upstream.c     |   36 +++---------
> >  drivers/staging/cx25821/cx25821-video-upstream-ch2.c |   57 +++++++------------
> >  drivers/staging/cx25821/cx25821-video-upstream.c     |   56 +++++++-----------
> >  3 files changed, 56 insertions(+), 93 deletions(-)
> > 
> > diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
> > index 27087db..a8f6343 100644
> > --- a/drivers/staging/cx25821/cx25821-audio-upstream.c
> > +++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
> > @@ -723,8 +723,7 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
> >  {
> >  	struct sram_channel *sram_ch;
> >  	int retval = 0;
> > -	int err = 0;
> > -	int str_length = 0;
> > +	char *filename;
> >  
> >  	if (dev->_audio_is_running) {
> >  		printk(KERN_WARNING "Audio Channel is still running so return!\n");
> > @@ -752,28 +751,15 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
> >  	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
> >  	_line_size = AUDIO_LINE_SIZE;
> >  
> > -	if (dev->input_audiofilename) {
> > -		str_length = strlen(dev->input_audiofilename);
> > -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
> > -
> > -		if (!dev->_audiofilename)
> > -			goto error;
> > -
> > -		memcpy(dev->_audiofilename, dev->input_audiofilename,
> > -		       str_length + 1);
> > -
> > -		/* Default if filename is empty string */
> > -		if (strcmp(dev->input_audiofilename, "") == 0)
> > -			dev->_audiofilename = "/root/audioGOOD.wav";
> > -
> > -	} else {
> > -		str_length = strlen(_defaultAudioName);
> > -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
> > -
> > -		if (!dev->_audiofilename)
> > -			goto error;
> > -
> > -		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
> > +	if (dev->input_audiofilename &&
> > +	    strcmp(dev->input_audiofilename, "") != 0)
> > +		filename = dev->input_audiofilename;
> > +	else
> > +		filename = _defaultAudioName;
> > +	dev->_audiofilename = kasprintf(GFP_KERNEL, "%s", filename);
> > +	if (!dev->_audiofilename) {
> > +		retval = -ENOMEM;
> > +		goto error;
> >  	}
> >
> 
> Is filename needed here at all ?

I'm not sure to understand the question.  There indeed appear to be 
different options.  The kasprintf (which seems like it could be changed to 
kstrdup) could indeed be moved into the two branches, but it seemed nicer 
to have only one function call and one block of error handling code, since 
the error handling code is identical.

> The if statement looks strange this looks more familar to me:
> if (!dev->input_audiofilename || *dev->input_audiofilename==0)
> 	filename = _defaultAudioName;
> else
> 	filename = dev->input_audiofilename;

OK.

julia

Re: [PATCH 1/3] drivers/staging/cx25821: Use kasprintf

[walter harms]

Julia Lawall schrieb:
> On Mon, 18 Oct 2010, walter harms wrote:
> 
>>
>> Julia Lawall schrieb:
>>> Rewrite the initialization of a dev field.  In the original code, in each
>>> case there was a kmalloc followed by a memcpy, as illustrated by the
>>> semantic patch below.  In the case that the provided string was the empty
>>> string, the allocated memory was then overwritten with a constant string,
>>> causing a memory leak.  Finally, there was no provision for returning
>>> -ENOMEM in case of failure of the memory allocation.  Indeed, the return
>>> value in an error case was err, a variable that was never initialized to
>>> anything other than 0.
>>>
>>> The following patch rewrites the above code to first select a string based
>>> on various conditions, and then to copy it into a newly allocated memory
>>> region, using kasprintf.  This decreases subtantially the code size
>>> and removes the memory leak.  The instruction for getting the length of the
>>> string and the associated variable declaration are also deleted.
>>>
>>> The patch also drops err, changes the return value to retval, which in each
>>> file was already initialized elsewhere to an error code, and initializes
>>> retval to -ENOMEM when kasprintf fails.
>>>
>>> The semantic patch that motivated this transformation is:
>>> (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @@
>>> expression a,flag,len;
>>> expression arg,e1,e2;
>>> statement S;
>>> @@
>>>
>>>   len = strlen(arg)
>>>   ... when != len = e1
>>>       when != arg = e2
>>>   a =
>>> -  \(kmalloc\|kzalloc\)(len+1,flag)
>>> +  kasprintf(flag,"%s",arg)
>>>   <... when != a
>>>   if (a == NULL || ...) S
>>>   ...>
>>> - memcpy(a,arg,len+1);
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <julia@diku.dk>
>>>
>>> ---
>>> This patch makes quite a lot of changes and is only compile tested.
>>>
>>>  drivers/staging/cx25821/cx25821-audio-upstream.c     |   36 +++---------
>>>  drivers/staging/cx25821/cx25821-video-upstream-ch2.c |   57 +++++++------------
>>>  drivers/staging/cx25821/cx25821-video-upstream.c     |   56 +++++++-----------
>>>  3 files changed, 56 insertions(+), 93 deletions(-)
>>>
>>> diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
>>> index 27087db..a8f6343 100644
>>> --- a/drivers/staging/cx25821/cx25821-audio-upstream.c
>>> +++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
>>> @@ -723,8 +723,7 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>>>  {
>>>  	struct sram_channel *sram_ch;
>>>  	int retval = 0;
>>> -	int err = 0;
>>> -	int str_length = 0;
>>> +	char *filename;
>>>  
>>>  	if (dev->_audio_is_running) {
>>>  		printk(KERN_WARNING "Audio Channel is still running so return!\n");
>>> @@ -752,28 +751,15 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>>>  	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
>>>  	_line_size = AUDIO_LINE_SIZE;
>>>  
>>> -	if (dev->input_audiofilename) {
>>> -		str_length = strlen(dev->input_audiofilename);
>>> -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
>>> -
>>> -		if (!dev->_audiofilename)
>>> -			goto error;
>>> -
>>> -		memcpy(dev->_audiofilename, dev->input_audiofilename,
>>> -		       str_length + 1);
>>> -
>>> -		/* Default if filename is empty string */
>>> -		if (strcmp(dev->input_audiofilename, "") == 0)
>>> -			dev->_audiofilename = "/root/audioGOOD.wav";
>>> -
>>> -	} else {
>>> -		str_length = strlen(_defaultAudioName);
>>> -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
>>> -
>>> -		if (!dev->_audiofilename)
>>> -			goto error;
>>> -
>>> -		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
>>> +	if (dev->input_audiofilename &&
>>> +	    strcmp(dev->input_audiofilename, "") != 0)
>>> +		filename = dev->input_audiofilename;
>>> +	else
>>> +		filename = _defaultAudioName;
>>> +	dev->_audiofilename = kasprintf(GFP_KERNEL, "%s", filename);
>>> +	if (!dev->_audiofilename) {
>>> +		retval = -ENOMEM;
>>> +		goto error;
>>>  	}
>>>
>> Is filename needed here at all ?
> 
> I'm not sure to understand the question.  There indeed appear to be 
> different options.  The kasprintf (which seems like it could be changed to 
> kstrdup) could indeed be moved into the two branches, but it seemed nicer 
> to have only one function call and one block of error handling code, since 
> the error handling code is identical.


the long version is:

the variable char * filename seems to be used as tmp buffer for the result of
the if statement. IMHO you could use dev->_audiofilename directly without loss
of clarity.

Turning the if condition on its head (and removing the strcmp) is the way i would
check. Please see this a comment the result is of cause identical.

hope that helps
re
 wh



> 
>> The if statement looks strange this looks more familar to me:
>> if (!dev->input_audiofilename || *dev->input_audiofilename==0)
>> 	filename = _defaultAudioName;
>> else
>> 	filename = dev->input_audiofilename;
> 
> OK.
> 

Re: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

[Julia Lawall]

Rewrite the initialization of a dev field.  In the original code, in each
case there was a kmalloc followed by a memcpy, as illustrated by the
semantic patch below.  In the case that the provided string was the empty
string, the allocated memory was then overwritten with a constant string,
causing a memory leak.  Finally, there was no provision for returning
-ENOMEM in case of failure of the memory allocation.  Indeed, the return
value in an error case was err, a variable that was never initialized to
anything other than 0.

The following patch rewrites the above code to first select a string based
on various conditions, and then to copy it into a newly allocated memory
region, using kstrdup.  This decreases subtantially the code size
and removes the memory leak.  The instruction for getting the length of the
string and the associated variable declaration are also deleted.

The patch also drops err, changes the return value to retval, which in each
file was already initialized elsewhere to an error code, and initializes
retval to -ENOMEM when kstrdup fails.

The semantic patch that motivated this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  len = strlen(arg)
  ... when != len = e1
      when != arg = e2
  a =
-  \(kmalloc\|kzalloc\)(len+1,flag)
+  kstrdup(arg,flag)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
This patch makes quite a lot of changes and is only compile tested.
As compared to the previous version, this uses kstrdup and it stores the 
selected string in the same field in which it will ultimately allocate new 
memory and copy the selected string.

diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
index 27087db..180840f 100644
--- a/drivers/staging/cx25821/cx25821-audio-upstream.c
+++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
@@ -723,8 +723,6 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 {
 	struct sram_channel *sram_ch;
 	int retval = 0;
-	int err = 0;
-	int str_length = 0;
 
 	if (dev->_audio_is_running) {
 		printk(KERN_WARNING "Audio Channel is still running so return!\n");
@@ -752,28 +750,14 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
 	_line_size = AUDIO_LINE_SIZE;
 
-	if (dev->input_audiofilename) {
-		str_length = strlen(dev->input_audiofilename);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, dev->input_audiofilename,
-		       str_length + 1);
-
-		/* Default if filename is empty string */
-		if (strcmp(dev->input_audiofilename, "") == 0)
-			dev->_audiofilename = "/root/audioGOOD.wav";
-
-	} else {
-		str_length = strlen(_defaultAudioName);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
+	if (!dev->input_audiofilename || *dev->input_audiofilename == 0)
+		dev->_audiofilename = _defaultAudioName;
+	else
+		dev->_audiofilename = dev->input_audiofilename;
+	dev->_audiofilename = kstrdup(dev->_audiofilename, GFP_KERNEL);
+	if (!dev->_audiofilename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -802,5 +786,5 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
index d12dbb5..2a9c430 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
@@ -747,10 +747,8 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
 
 	if (dev->_is_running_ch2) {
 		printk("Video Channel is still running so return!\n");
@@ -789,38 +787,26 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	    dev->_isNTSC_ch2 ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
 	if (dev->input_filename_ch2) {
-		str_length = strlen(dev->input_filename_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->input_filename_ch2,
-		       str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->_defaultname_ch2,
-		       str_length + 1);
-	}
-
-       /* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") == 0) {
-		if (dev->_isNTSC_ch2) {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+		/* Default if filename is empty string */
+		if (*dev->input_filename_ch2 == 0) {
+			if (dev->_isNTSC_ch2)
+				dev->_filename_ch2 =
+					(dev->_pixel_format_ch2 ==
+					PIXEL_FRMT_411) ? "/root/vid411.yuv" :
+					"/root/vidtest.yuv";
+			else
+				dev->_filename_ch2 =
+					(dev->_pixel_format_ch2 ==
+					PIXEL_FRMT_411) ? "/root/pal411.yuv" :
+					"/root/pal422.yuv";
+		} else
+			dev->_filename_ch2 = dev->input_filename_ch2;
+	} else
+		dev->_filename_ch2 = dev->_defaultname_ch2;
+	dev->_filename_ch2 = kstrdup(dev->_filename_ch2, GFP_KERNEL);
+	if (!dev->_filename_ch2) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -851,5 +837,5 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
       error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream.c b/drivers/staging/cx25821/cx25821-video-upstream.c
index 756a820..58e67f2 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream.c
@@ -800,10 +800,8 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
 
 	if (dev->_is_running) {
 		printk(KERN_INFO "Video Channel is still running so return!\n");
@@ -841,36 +839,27 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	    dev->_isNTSC ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
 	if (dev->input_filename) {
-		str_length = strlen(dev->input_filename);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->input_filename, str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->_defaultname, str_length + 1);
-	}
-
-	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") == 0) {
-		if (dev->_isNTSC) {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+		/* Default if filename is empty string */
+		if (*dev->input_filename == 0) {
+			if (dev->_isNTSC)
+				dev->_filename =
+					(dev->_pixel_format ==
+					PIXEL_FRMT_411) ? "/root/vid411.yuv" :
+					"/root/vidtest.yuv";
+			else
+				dev->_filename =
+					(dev->_pixel_format ==
+					PIXEL_FRMT_411) ? "/root/pal411.yuv" :
+					"/root/pal422.yuv";
+		} else
+			dev->_filename = dev->input_filename;
+	} else
+		dev->_filename = dev->_defaultname;
+
+	dev->_filename = kstrdup(dev->_filename, GFP_KERNEL);
+	if (!dev->_filename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	dev->_is_running = 0;
@@ -908,5 +897,5 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }

Re: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

[Joe Perches]

Perhaps it's more readable code to recheck the
field name flag and introduce a temporary
char *fname so the slightly unusual reuse of
	field = kstrdup(field, GFP)
becomes
	field = kstrdup(fname, GFP)

---
 drivers/staging/cx25821/cx25821-audio-upstream.c   |   34 +++---------
 .../staging/cx25821/cx25821-video-upstream-ch2.c   |   55 +++++++-------------
 drivers/staging/cx25821/cx25821-video-upstream.c   |   53 +++++++------------
 3 files changed, 47 insertions(+), 95 deletions(-)

diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
index 27087db..180840f 100644
--- a/drivers/staging/cx25821/cx25821-audio-upstream.c
+++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
@@ -723,8 +723,6 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 {
 	struct sram_channel *sram_ch;
 	int retval = 0;
-	int err = 0;
-	int str_length = 0;
 
 	if (dev->_audio_is_running) {
 		printk(KERN_WARNING "Audio Channel is still running so return!\n");
@@ -752,28 +750,14 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
 	_line_size = AUDIO_LINE_SIZE;
 
-	if (dev->input_audiofilename) {
-		str_length = strlen(dev->input_audiofilename);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, dev->input_audiofilename,
-		       str_length + 1);
-
-		/* Default if filename is empty string */
-		if (strcmp(dev->input_audiofilename, "") == 0)
-			dev->_audiofilename = "/root/audioGOOD.wav";
-
-	} else {
-		str_length = strlen(_defaultAudioName);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
+	if (!dev->input_audiofilename || *dev->input_audiofilename == 0)
+		dev->_audiofilename = _defaultAudioName;
+	else
+		dev->_audiofilename = dev->input_audiofilename;
+	dev->_audiofilename = kstrdup(dev->_audiofilename, GFP_KERNEL);
+	if (!dev->_audiofilename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -802,5 +786,5 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
index d12dbb5..fa7cd70 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
@@ -747,10 +747,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
+	char *fname;
 
 	if (dev->_is_running_ch2) {
 		printk("Video Channel is still running so return!\n");
@@ -788,39 +787,23 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	risc_buffer_size =
 	    dev->_isNTSC_ch2 ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
-	if (dev->input_filename_ch2) {
-		str_length = strlen(dev->input_filename_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->input_filename_ch2,
-		       str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->_defaultname_ch2,
-		       str_length + 1);
-	}
-
-       /* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") == 0) {
-		if (dev->_isNTSC_ch2) {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+	if (dev->input_filename_ch2 && *dev->input_filename_ch2)
+		fname = dev->input_filename_ch2;
+	else if (dev->input_filename_ch2) {
+		/* Default if filename is empty string */
+		if (dev->_isNTSC_ch2)
+			fname = (dev->_pixel_format_ch2 == PIXEL_FRMT_411) ?
+				"/root/vid411.yuv" : "/root/vidtest.yuv";
+		else
+			fname = (dev->_pixel_format_ch2 == PIXEL_FRMT_411) ?
+				"/root/pal411.yuv" : "/root/pal422.yuv";
+	} else
+		fname = dev->_defaultname_ch2;
+
+	dev->_filename_ch2 = kstrdup(fname, GFP_KERNEL);
+	if (!dev->_filename_ch2) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -851,5 +834,5 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
       error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream.c b/drivers/staging/cx25821/cx25821-video-upstream.c
index 756a820..d51c6c1 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream.c
@@ -800,10 +800,9 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
+	char *fname;
 
 	if (dev->_is_running) {
 		printk(KERN_INFO "Video Channel is still running so return!\n");
@@ -840,37 +839,23 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	risc_buffer_size =
 	    dev->_isNTSC ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
-	if (dev->input_filename) {
-		str_length = strlen(dev->input_filename);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->input_filename, str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->_defaultname, str_length + 1);
-	}
-
-	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") == 0) {
-		if (dev->_isNTSC) {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+	if (dev->input_filename && *dev->input_filename)
+		fname = dev->input_filename;
+	else if (dev->input_filename) {
+		/* Default if filename is empty string */
+		if (dev->_isNTSC)
+			fname = (dev->_pixel_format == PIXEL_FRMT_411) ?
+				"/root/vid411.yuv" : "/root/vidtest.yuv";
+		else
+			fname = (dev->_pixel_format == PIXEL_FRMT_411) ?
+				"/root/pal411.yuv" : "/root/pal422.yuv";
+	} else
+		fname = dev->_defaultname;
+
+	dev->_filename = kstrdup(fname, GFP_KERNEL);
+	if (!dev->_filename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	dev->_is_running = 0;
@@ -908,5 +893,5 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }

Re: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

[Julia Lawall]

On Mon, 18 Oct 2010, Joe Perches wrote:

> Perhaps it's more readable code to recheck the
> field name flag and introduce a temporary
> char *fname so the slightly unusual reuse of
> 	field = kstrdup(field, GFP)
> becomes
> 	field = kstrdup(fname, GFP)

Before I had a local variable filename.  I preferred that because I felt 
uneasy about putting both statically and dynamically allocated memory in 
the same field.  But it does mean adding a new local variable.

I'm not sure to understand "recheck the field name flag", though.

julia


> ---
>  drivers/staging/cx25821/cx25821-audio-upstream.c   |   34 +++---------
>  .../staging/cx25821/cx25821-video-upstream-ch2.c   |   55 +++++++-------------
>  drivers/staging/cx25821/cx25821-video-upstream.c   |   53 +++++++------------
>  3 files changed, 47 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
> index 27087db..180840f 100644
> --- a/drivers/staging/cx25821/cx25821-audio-upstream.c
> +++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
> @@ -723,8 +723,6 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>  {
>  	struct sram_channel *sram_ch;
>  	int retval = 0;
> -	int err = 0;
> -	int str_length = 0;
>  
>  	if (dev->_audio_is_running) {
>  		printk(KERN_WARNING "Audio Channel is still running so return!\n");
> @@ -752,28 +750,14 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>  	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
>  	_line_size = AUDIO_LINE_SIZE;
>  
> -	if (dev->input_audiofilename) {
> -		str_length = strlen(dev->input_audiofilename);
> -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_audiofilename)
> -			goto error;
> -
> -		memcpy(dev->_audiofilename, dev->input_audiofilename,
> -		       str_length + 1);
> -
> -		/* Default if filename is empty string */
> -		if (strcmp(dev->input_audiofilename, "") == 0)
> -			dev->_audiofilename = "/root/audioGOOD.wav";
> -
> -	} else {
> -		str_length = strlen(_defaultAudioName);
> -		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_audiofilename)
> -			goto error;
> -
> -		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
> +	if (!dev->input_audiofilename || *dev->input_audiofilename == 0)
> +		dev->_audiofilename = _defaultAudioName;
> +	else
> +		dev->_audiofilename = dev->input_audiofilename;
> +	dev->_audiofilename = kstrdup(dev->_audiofilename, GFP_KERNEL);
> +	if (!dev->_audiofilename) {
> +		retval = -ENOMEM;
> +		goto error;
>  	}
>  
>  	retval =
> @@ -802,5 +786,5 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
>  error:
>  	cx25821_dev_unregister(dev);
>  
> -	return err;
> +	return retval;
>  }
> diff --git a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
> index d12dbb5..fa7cd70 100644
> --- a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
> +++ b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
> @@ -747,10 +747,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
>  	struct sram_channel *sram_ch;
>  	u32 tmp;
>  	int retval = 0;
> -	int err = 0;
>  	int data_frame_size = 0;
>  	int risc_buffer_size = 0;
> -	int str_length = 0;
> +	char *fname;
>  
>  	if (dev->_is_running_ch2) {
>  		printk("Video Channel is still running so return!\n");
> @@ -788,39 +787,23 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
>  	risc_buffer_size =
>  	    dev->_isNTSC_ch2 ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
>  
> -	if (dev->input_filename_ch2) {
> -		str_length = strlen(dev->input_filename_ch2);
> -		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename_ch2)
> -			goto error;
> -
> -		memcpy(dev->_filename_ch2, dev->input_filename_ch2,
> -		       str_length + 1);
> -	} else {
> -		str_length = strlen(dev->_defaultname_ch2);
> -		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename_ch2)
> -			goto error;
> -
> -		memcpy(dev->_filename_ch2, dev->_defaultname_ch2,
> -		       str_length + 1);
> -	}
> -
> -       /* Default if filename is empty string */
> -	if (strcmp(dev->input_filename_ch2, "") == 0) {
> -		if (dev->_isNTSC_ch2) {
> -			dev->_filename_ch2 =
> -			    (dev->_pixel_format_ch2 ==
> -			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
> -			    "/root/vidtest.yuv";
> -		} else {
> -			dev->_filename_ch2 =
> -			    (dev->_pixel_format_ch2 ==
> -			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
> -			    "/root/pal422.yuv";
> -		}
> +	if (dev->input_filename_ch2 && *dev->input_filename_ch2)
> +		fname = dev->input_filename_ch2;
> +	else if (dev->input_filename_ch2) {
> +		/* Default if filename is empty string */
> +		if (dev->_isNTSC_ch2)
> +			fname = (dev->_pixel_format_ch2 == PIXEL_FRMT_411) ?
> +				"/root/vid411.yuv" : "/root/vidtest.yuv";
> +		else
> +			fname = (dev->_pixel_format_ch2 == PIXEL_FRMT_411) ?
> +				"/root/pal411.yuv" : "/root/pal422.yuv";
> +	} else
> +		fname = dev->_defaultname_ch2;
> +
> +	dev->_filename_ch2 = kstrdup(fname, GFP_KERNEL);
> +	if (!dev->_filename_ch2) {
> +		retval = -ENOMEM;
> +		goto error;
>  	}
>  
>  	retval =
> @@ -851,5 +834,5 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
>        error:
>  	cx25821_dev_unregister(dev);
>  
> -	return err;
> +	return retval;
>  }
> diff --git a/drivers/staging/cx25821/cx25821-video-upstream.c b/drivers/staging/cx25821/cx25821-video-upstream.c
> index 756a820..d51c6c1 100644
> --- a/drivers/staging/cx25821/cx25821-video-upstream.c
> +++ b/drivers/staging/cx25821/cx25821-video-upstream.c
> @@ -800,10 +800,9 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
>  	struct sram_channel *sram_ch;
>  	u32 tmp;
>  	int retval = 0;
> -	int err = 0;
>  	int data_frame_size = 0;
>  	int risc_buffer_size = 0;
> -	int str_length = 0;
> +	char *fname;
>  
>  	if (dev->_is_running) {
>  		printk(KERN_INFO "Video Channel is still running so return!\n");
> @@ -840,37 +839,23 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
>  	risc_buffer_size =
>  	    dev->_isNTSC ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
>  
> -	if (dev->input_filename) {
> -		str_length = strlen(dev->input_filename);
> -		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename)
> -			goto error;
> -
> -		memcpy(dev->_filename, dev->input_filename, str_length + 1);
> -	} else {
> -		str_length = strlen(dev->_defaultname);
> -		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
> -
> -		if (!dev->_filename)
> -			goto error;
> -
> -		memcpy(dev->_filename, dev->_defaultname, str_length + 1);
> -	}
> -
> -	/* Default if filename is empty string */
> -	if (strcmp(dev->input_filename, "") == 0) {
> -		if (dev->_isNTSC) {
> -			dev->_filename =
> -			    (dev->_pixel_format ==
> -			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
> -			    "/root/vidtest.yuv";
> -		} else {
> -			dev->_filename =
> -			    (dev->_pixel_format ==
> -			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
> -			    "/root/pal422.yuv";
> -		}
> +	if (dev->input_filename && *dev->input_filename)
> +		fname = dev->input_filename;
> +	else if (dev->input_filename) {
> +		/* Default if filename is empty string */
> +		if (dev->_isNTSC)
> +			fname = (dev->_pixel_format == PIXEL_FRMT_411) ?
> +				"/root/vid411.yuv" : "/root/vidtest.yuv";
> +		else
> +			fname = (dev->_pixel_format == PIXEL_FRMT_411) ?
> +				"/root/pal411.yuv" : "/root/pal422.yuv";
> +	} else
> +		fname = dev->_defaultname;
> +
> +	dev->_filename = kstrdup(fname, GFP_KERNEL);
> +	if (!dev->_filename) {
> +		retval = -ENOMEM;
> +		goto error;
>  	}
>  
>  	dev->_is_running = 0;
> @@ -908,5 +893,5 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
>  error:
>  	cx25821_dev_unregister(dev);
>  
> -	return err;
> +	return retval;
>  }
> 
> 
> 

Re: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

[Joe Perches]

On Mon, 2010-10-18 at 19:44 +0200, Julia Lawall wrote:
> On Mon, 18 Oct 2010, Joe Perches wrote:
> 
> > Perhaps it's more readable code to recheck the
> > field name flag and introduce a temporary
> > char *fname so the slightly unusual reuse of
> > 	field = kstrdup(field, GFP)
> > becomes
> > 	field = kstrdup(fname, GFP)
> 
> Before I had a local variable filename.  I preferred that because I felt 
> uneasy about putting both statically and dynamically allocated memory in 
> the same field.  But it does mean adding a new local variable.

I think readability is better using a temporary.

> I'm not sure to understand "recheck the field name flag", though.

Sorry, poor wording.  I meant using this style:

	if (field && *field)
		foo
	else if (field)
		bar
	else
		baz

instead of:

	if (field) {
		if (*field)
			foo
		else
			bar
	} else
		baz

Re: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

[Julia Lawall]

On Mon, 18 Oct 2010, Joe Perches wrote:

> On Mon, 2010-10-18 at 19:44 +0200, Julia Lawall wrote:
> > On Mon, 18 Oct 2010, Joe Perches wrote:
> > 
> > > Perhaps it's more readable code to recheck the
> > > field name flag and introduce a temporary
> > > char *fname so the slightly unusual reuse of
> > > 	field = kstrdup(field, GFP)
> > > becomes
> > > 	field = kstrdup(fname, GFP)
> > 
> > Before I had a local variable filename.  I preferred that because I felt 
> > uneasy about putting both statically and dynamically allocated memory in 
> > the same field.  But it does mean adding a new local variable.
> 
> I think readability is better using a temporary.
> 
> > I'm not sure to understand "recheck the field name flag", though.
> 
> Sorry, poor wording.  I meant using this style:
> 
> 	if (field && *field)
> 		foo
> 	else if (field)
> 		bar
> 	else
> 		baz
> 
> instead of:
> 
> 	if (field) {
> 		if (*field)
> 			foo
> 		else
> 			bar
> 	} else
> 		baz

OK :)  I'll do that.

julia

Re: [PATCH 0/3] Use kasprintf

[Paulo Marques]

Julia Lawall wrote:
> These patches convert a sequence of kmalloc and memcpy to use kasprintf
> instead.

Aren't these patches just a more convoluted way of doing a kstrdup?

I would imagine that a kasprintf would make more sense when the format
string is more complex than "%s", or am I missing something?

-- 
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."

Re: [PATCH 0/3] Use kasprintf

[Julia Lawall]

On Mon, 18 Oct 2010, Paulo Marques wrote:

> Julia Lawall wrote:
> > These patches convert a sequence of kmalloc and memcpy to use kasprintf
> > instead.
> 
> Aren't these patches just a more convoluted way of doing a kstrdup?
> 
> I would imagine that a kasprintf would make more sense when the format
> string is more complex than "%s", or am I missing something?

They have all already been changed to either kmemdup or kstrdup.

thanks,
julia

Re: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

[Julia Lawall]

>From nobody Sun Oct 17 20:14:32 CEST 2010
From: Julia Lawall <julia@diku.dk>
To: Greg Kroah-Hartman <gregkh@suse.de>
Cc: devel@driverdev.osuosl.org,linux-kernel@vger.kernel.org
Subject: [PATCH 1/3] drivers/staging/cx25821: Use kstrdup

Rewrite the initialization of a dev field.  In the original code, in each
case there was a kmalloc followed by a memcpy, as illustrated by the
semantic patch below.  In the case that the provided string was the empty
string, the allocated memory was then overwritten with a constant string,
causing a memory leak.  Finally, there was no provision for returning
-ENOMEM in case of failure of the memory allocation.  Indeed, the return
value in an error case was err, a variable that was never initialized to
anything other than 0.

The following patch rewrites the above code to first select a string based
on various conditions, and then to copy it into a newly allocated memory
region, using kstrdup.  This decreases subtantially the code size
and removes the memory leak.  The instruction for getting the length of the
string and the associated variable declaration are also deleted.

The patch also drops err, changes the return value to retval, which in each
file was already initialized elsewhere to an error code, and initializes
retval to -ENOMEM when kstrdup fails.

The semantic patch that motivated this transformation is:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,flag,len;
expression arg,e1,e2;
statement S;
@@

  len = strlen(arg)
  ... when != len = e1
      when != arg = e2
  a =
-  \(kmalloc\|kzalloc\)(len+1,flag)
+  kstrdup(arg,flag)
  <... when != a
  if (a == NULL || ...) S
  ...>
- memcpy(a,arg,len+1);
// </smpl>

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

---
This patch makes quite a lot of changes and is only compile tested.
As compared to the previous version, the local variable filename is readded
and the nested ifs in the second and third file are reorganized.

 drivers/staging/cx25821/cx25821-audio-upstream.c     |   35 +++--------
 drivers/staging/cx25821/cx25821-video-upstream-ch2.c |   58 +++++++------------
 drivers/staging/cx25821/cx25821-video-upstream.c     |   55 +++++++-----------
 3 files changed, 54 insertions(+), 94 deletions(-)

diff --git a/drivers/staging/cx25821/cx25821-audio-upstream.c b/drivers/staging/cx25821/cx25821-audio-upstream.c
index 27087db..252aa49 100644
--- a/drivers/staging/cx25821/cx25821-audio-upstream.c
+++ b/drivers/staging/cx25821/cx25821-audio-upstream.c
@@ -723,8 +723,7 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 {
 	struct sram_channel *sram_ch;
 	int retval = 0;
-	int err = 0;
-	int str_length = 0;
+	char *filename;
 
 	if (dev->_audio_is_running) {
 		printk(KERN_WARNING "Audio Channel is still running so return!\n");
@@ -752,28 +751,14 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 	dev->_audio_lines_count = LINES_PER_AUDIO_BUFFER;
 	_line_size = AUDIO_LINE_SIZE;
 
-	if (dev->input_audiofilename) {
-		str_length = strlen(dev->input_audiofilename);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, dev->input_audiofilename,
-		       str_length + 1);
-
-		/* Default if filename is empty string */
-		if (strcmp(dev->input_audiofilename, "") == 0)
-			dev->_audiofilename = "/root/audioGOOD.wav";
-
-	} else {
-		str_length = strlen(_defaultAudioName);
-		dev->_audiofilename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_audiofilename)
-			goto error;
-
-		memcpy(dev->_audiofilename, _defaultAudioName, str_length + 1);
+	if (!dev->input_audiofilename || *dev->input_audiofilename == 0)
+		filename = _defaultAudioName;
+	else
+		filename = dev->input_audiofilename;
+	dev->_audiofilename = kstrdup(filename, GFP_KERNEL);
+	if (!dev->_audiofilename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -802,5 +787,5 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
index d12dbb5..3d276c1 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream-ch2.c
@@ -747,10 +747,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
+	char *filename;
 
 	if (dev->_is_running_ch2) {
 		printk("Video Channel is still running so return!\n");
@@ -788,39 +787,26 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	risc_buffer_size =
 	    dev->_isNTSC_ch2 ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
-	if (dev->input_filename_ch2) {
-		str_length = strlen(dev->input_filename_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->input_filename_ch2,
-		       str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname_ch2);
-		dev->_filename_ch2 = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename_ch2)
-			goto error;
-
-		memcpy(dev->_filename_ch2, dev->_defaultname_ch2,
-		       str_length + 1);
-	}
-
-       /* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") == 0) {
-		if (dev->_isNTSC_ch2) {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename_ch2 =
-			    (dev->_pixel_format_ch2 ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+	if (dev->input_filename_ch2 && *dev->input_filename_ch2 == 0) {
+		/* Default if filename is empty string */
+		if (dev->_isNTSC_ch2)
+			filename =
+				(dev->_pixel_format_ch2 == PIXEL_FRMT_411) ?
+				"/root/vid411.yuv" :
+				"/root/vidtest.yuv";
+		else
+			filename =
+				(dev->_pixel_format_ch2 == PIXEL_FRMT_411) ?
+				"/root/pal411.yuv" :
+				"/root/pal422.yuv";
+	} else if (dev->input_filename_ch2)
+		filename = dev->input_filename_ch2;
+	else
+		filename = dev->_defaultname_ch2;
+	dev->_filename_ch2 = kstrdup(filename, GFP_KERNEL);
+	if (!dev->_filename_ch2) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	retval =
@@ -851,5 +837,5 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
       error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }
diff --git a/drivers/staging/cx25821/cx25821-video-upstream.c b/drivers/staging/cx25821/cx25821-video-upstream.c
index 756a820..465c2b2 100644
--- a/drivers/staging/cx25821/cx25821-video-upstream.c
+++ b/drivers/staging/cx25821/cx25821-video-upstream.c
@@ -800,10 +800,9 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	struct sram_channel *sram_ch;
 	u32 tmp;
 	int retval = 0;
-	int err = 0;
 	int data_frame_size = 0;
 	int risc_buffer_size = 0;
-	int str_length = 0;
+	char *filename;
 
 	if (dev->_is_running) {
 		printk(KERN_INFO "Video Channel is still running so return!\n");
@@ -840,37 +839,27 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	risc_buffer_size =
 	    dev->_isNTSC ? NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
 
-	if (dev->input_filename) {
-		str_length = strlen(dev->input_filename);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->input_filename, str_length + 1);
-	} else {
-		str_length = strlen(dev->_defaultname);
-		dev->_filename = kmalloc(str_length + 1, GFP_KERNEL);
-
-		if (!dev->_filename)
-			goto error;
-
-		memcpy(dev->_filename, dev->_defaultname, str_length + 1);
-	}
+	if (dev->input_filename && *dev->input_filename == 0) {
+		/* Default if filename is empty string */
+		if (dev->_isNTSC)
+			filename =
+				(dev->_pixel_format == PIXEL_FRMT_411) ?
+				"/root/vid411.yuv" :
+				"/root/vidtest.yuv";
+		else
+			filename =
+				(dev->_pixel_format == PIXEL_FRMT_411) ?
+				"/root/pal411.yuv" :
+				"/root/pal422.yuv";
+	} else if (dev->input_filename)
+		filename = dev->input_filename;
+	else
+		filename = dev->_defaultname;
 
-	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") == 0) {
-		if (dev->_isNTSC) {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/vid411.yuv" :
-			    "/root/vidtest.yuv";
-		} else {
-			dev->_filename =
-			    (dev->_pixel_format ==
-			     PIXEL_FRMT_411) ? "/root/pal411.yuv" :
-			    "/root/pal422.yuv";
-		}
+	dev->_filename = kstrdup(filename, GFP_KERNEL);
+	if (!dev->_filename) {
+		retval = -ENOMEM;
+		goto error;
 	}
 
 	dev->_is_running = 0;
@@ -908,5 +897,5 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 error:
 	cx25821_dev_unregister(dev);
 
-	return err;
+	return retval;
 }