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@xxxxxxx> >>> >>> --- >>> 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. > -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
