Re: [patch 07/11] zfcp: Cleanup of code in zfcp_aux.c

[Swen Schillig <swen@vnet.ibm.com>]

On Tuesday 01 July 2008 00:52, Heiko Carstens wrote:
> > +static int __init zfcp_device_setup(char *devstr)
> >  {
> > -	char *tmp, *str;
> > -	size_t len;
> > +	char *token;
> > 
> >  	if (!devstr)
> >  		return 0;
> > 
> > -	len = strlen(devstr) + 1;
> > -	str = kmalloc(len, GFP_KERNEL);
> > -	if (!str) {
> > -		pr_err("zfcp: Could not allocate memory for "
> > -		       "device parameter string, device not attached.\n");
> > -		return 0;
> > -	}
> > -	memcpy(str, devstr, len);
> > -
> > -	tmp = strchr(str, ',');
> > -	if (!tmp)
> > +	token = strsep(&devstr, ",");
> > +	if (!token || strlen(token) >= BUS_ID_SIZE)
> >  		goto err_out;
> > -	*tmp++ = '\0';
> > -	strncpy(zfcp_data.init_busid, str, BUS_ID_SIZE);
> > -	zfcp_data.init_busid[BUS_ID_SIZE-1] = '\0';
> > +	strncpy(zfcp_data.init_busid, token, BUS_ID_SIZE);
> > 
> > -	zfcp_data.init_wwpn = simple_strtoull(tmp, &tmp, 0);
> > -	if (*tmp++ != ',')
> > -		goto err_out;
> > -	if (*tmp == '\0')
> > +	token = strsep(&devstr, ",");
> > +	if (!token || strict_strtoull(token, 0, &zfcp_data.init_wwpn))
> >  		goto err_out;
> > 
> > -	zfcp_data.init_fcp_lun = simple_strtoull(tmp, &tmp, 0);
> > -	if (*tmp != '\0')
> > +	token = strsep(&devstr, ",");
> > +	if (!token || strict_strtoull(token, 0, &zfcp_data.init_fcp_lun))
> >  		goto err_out;
> > -	kfree(str);
> >  	return 1;
> > 
> >   err_out:
> >  	pr_err("zfcp: Parse error for device parameter string %s, "
> > -	       "device not attached.\n", str);
> > -	kfree(str);
> > +	       "device not attached.\n", devstr);
> >  	return 0;
> >  }
> 
> Sorry, but this still seems to be incorrect. strsep modifies the string that
> gets passed to it. In this case you let it operate on the original module
> parameter string which is also exported via sysfs.
> So after parsing finished the exported string is much shorter than the
> original string. That was actually the whole point why the old code allocated
> memory and copied the string so the copy could be modified.
> A comment would have been good ;)
> --
Correct,

I fixed the patch and Christof will send it soon to the list.

Cheers Swen