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

Heiko Carstens
2008-06-30T22:53:51Z

> +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 ;)