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

[Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html