[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-scsi
Subject:    Re: [patch 07/11] zfcp: Cleanup of code in zfcp_aux.c
From:       Heiko Carstens <heiko.carstens () de ! ibm ! com>
Date:       2008-06-30 22:52:59
Message-ID: 20080630225259.GB4727 () osiris ! boeblingen ! de ! ibm ! com
[Download RAW message or body]

> +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 ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]