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.
