On Tue, 26 Apr 2011 15:27:32 +0100 David Howells <dhowells@xxxxxxxxxx> wrote: > Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > - __u16 byte_count, total_data_size, total_in_buf, total_in_buf2; > > + unsigned int byte_count, total_in_buf; > > + __u16 total_data_size, total_in_buf2; > > There's no particular need for any of these to be __u16; I'd recommend making > them all unsigned int or size_t. > Fair enough. For those two, they can be __u16 with no problem, AFAICT. Is there some reason that an unsigned int would be better here? > > + /* did this field "wrap" ? */ > > + if (total_in_buf & ~((1<<16)-1)) > > + return -EINVAL; > > I'd recommend something more like the following: > > /* check the server isn't offering too much data */ > if (total_in_buf > USHRT_MAX) > return -EINVAL; > > rather than calculating a mask. > Ahh, didn't know about USHRT_MAX, I'll change it to use that instead. > Also, would EPROTO be a better choice for packet parsing errors than EINVAL? > > > + /* did this field "wrap" ? */ > > + if (byte_count & ~((1<<16)-1)) > > + return -EINVAL; > > Ditto. > > David Probably -- or EIO maybe. This error eventually gets discarded anyway once we mark the mid as malformed, so it really doesn't matter much for this. Still, more specific errors are better so maybe I'll change this when I respin it. Thanks for the review. -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
