Re: [PATCH 1/2] scsi: sd: Separate zeroout and discard command choices

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Wed, 2017-04-05 at 07:41 -0400, Martin K. Petersen wrote:
> +static const char *zeroing_mode[] = {
> +	[SD_ZERO_WRITE]		= "write",
> +	[SD_ZERO_WS]		= "writesame",
> +	[SD_ZERO_WS16_UNMAP]	= "writesame_16_unmap",
> +	[SD_ZERO_WS10_UNMAP]	= "writesame_10_unmap",
> +};
> +
> +static ssize_t
> +zeroing_mode_show(struct device *dev, struct device_attribute *attr,
> +		  char *buf)
> +{
> +	struct scsi_disk *sdkp = to_scsi_disk(dev);
> +
> +	return snprintf(buf, 20, "%s\n", zeroing_mode[sdkp->zeroing_mode]);
> +}

Hello Martin,

If anyone would ever add a string to zeroing_mode[] that is longer than 20
characters then zeroing_mode_show() will truncate it. Since all strings in
the zeroing_mode[] array are short, have you considered to use sprintf()
instead? And if you do not want to use sprintf(), how about using
snprintf(buf, PAGE_SIZE, ...)? I'm asking this because I'm no fan of magic
constants.
 
> +static ssize_t
> +zeroing_mode_store(struct device *dev, struct device_attribute *attr,
> +		   const char *buf, size_t count)
> +{
> +	struct scsi_disk *sdkp = to_scsi_disk(dev);
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EACCES;
> +
> +	if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE], 20))
> +		sdkp->zeroing_mode = SD_ZERO_WRITE;
> +	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS], 20))
> +		sdkp->zeroing_mode = SD_ZERO_WS;
> +	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP], 20))
> +		sdkp->zeroing_mode = SD_ZERO_WS16_UNMAP;
> +	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP], 20))
> +		sdkp->zeroing_mode = SD_ZERO_WS10_UNMAP;
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}

Since sysfs guarantees that buf is '\0'-terminated, why does the above
function call strncmp() instead of strcmp()?

Can the above chain of if-statements be replaced by a for-loop such that
zeroing_mode_store() won't have to be updated if the zeroing_mode[] array
is modified?

Thanks,

Bart.