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.
>
> > +
> > + 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?
NeilBrown