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

Swen Schillig
2008-07-01T13:44:53Z

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