Re: [PATCH v2 08/15] scsi_transport_srp: Add transport layer error handling

Bart Van Assche
2013-07-01T07:01:21Z

On 06/30/13 23:05, David Dillow wrote:
> On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:
>> +int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
>> +{
>> + return (fast_io_fail_tmo < 0 || dev_loss_tmo < 0 ||
>> + fast_io_fail_tmo < dev_loss_tmo) &&
>> + fast_io_fail_tmo < LONG_MAX / HZ &&
>> + dev_loss_tmo < LONG_MAX / HZ ? 0 : -EINVAL;
>> +}
>
> They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
> LONG_MAX / HZ, I think.

The fast_io_fail_tmo should indeed be capped by that value. However, I'm
not sure about dev_loss_tmo. I think there are several use cases (e.g.
initiator-side mirroring) where it's useful to set dev_loss_tmo to a
larger value than ten minutes.

>> +static ssize_t store_srp_rport_fast_io_fail_tmo(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct srp_rport *rport = transport_class_to_srp_rport(dev);
>> + char ch[16], *p;
>> + int res;
>> + int fast_io_fail_tmo;
>> +
>> + sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
>> + p = strchr(ch, '\n');
>> + if (p)
>> + *p = '\0';
>
> Again, no need for the sprintf if you don't modify the buffer? Instead
> of using strchr() to make the strcmp() work with newlines, just do
>
> if (!strcmp(buf, "off") || !strcmp(buf, "off\n")) {
> fast_io_fail_tmo = 1;
> } else {
> res = kstrtoint(buf, 0, &fast_io_fail_tmo);
> ...
>
> instead?
>
> Same comment applies for store_srp_rport_dev_loss_tmo().

OK, will remove the temporary char arrays.

Bart.