On Jan 14, 2008, at 6:31 PM, Neil Brown wrote:
> On Monday January 14, [email protected] wrote:
>>>
>>> +static ssize_t failover_unlock_ip(struct file *file, char *buf,
>>> size_t size)
>>> +{
>>> + __be32 server_ip;
>>> + char *fo_path;
>>> + char *mesg;
>>> +
>>> + /* sanity check */
>>> + if (size <= 0)
>>> + return -EINVAL;
>>
>> Not only is size never negative, it's actually an unsigned type
>> here, so
>> this is a no-op.
>
> No, It it equivalent to
> if (size == 0)
>
> which alternative is clearer and more maintainable is debatable.
I think Wendy should fix this. Otherwise, it can easily confuse
someone trying to read this code. If the equals-zero check is
needed, leave that part. But it's not clear what is intended by the
less-than-zero check on an unsigned.
And, if we ever enable thorough compiler warnings, the less-than-zero
check will cause the compiler to throw needless warnings, adding to
noise.
>>> +
>>> + if (buf[size-1] == '\n')
>>> + buf[size-1] = 0;
>>
>> The other write methods in this file actually just do
>>
>> if (buf[size-1] != '\n')
>> return -EINVAL;
>
> and those which don't check for size == 0 are underflowing an array.
> That should probably be fixed.
>
>>
>> I don't know why. But absent some reason, I'd rather these two new
>> files behaved the same as existing ones.
>>
>>> +
>>> + fo_path = mesg = buf;
>>> + if (qword_get(&mesg, fo_path, size) < 0)
>>> + return -EINVAL;
>>
>> "mesg" is unneeded here, right? You can just do:
>>
>> fo_path = buf;
>> if (qword_get(&buf, buf, size) < 0)
>>
>>> +
>>> + server_ip = in_aton(fo_path);
>>
>> It'd be nice if we could sanity-check this. (Is there code
>> already in
>> the kernel someplace to do this?)
>
> In ip_map_parse we do:
>
> if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> return -EINVAL;
> ...
> addr.s_addr =
> htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
>
> I suspect that would fit in an inline function somewhere quite
> nicely. but where?
By sanity-check, I assumed Bruce meant that the value of fo_path
ought to be verified as a valid IP address, and not some arbitrary
string. I think in_aton() actually returns INADDR_ANY if it can't
parse the passed-in string, so a check for that after the in_aton()
call might be sufficient.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com