[Cluster-devel] Re: [PATCH 1/2] NLM failover unlock commands

[Neil Brown]

On Monday January 14, bfields at fieldses.org 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