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
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
