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