On Sun, Mar 23, 2014 at 11:11:42AM -0400, J. Bruce Fields wrote: > > > } while (0) > > > -#define ADJUST_ARGS() resp->xdr.p = p > > > +#define ADJUST_ARGS() WARN_ON_ONCE(p != resp->xdr.p) \ > > > > I think the code would become more clear if you'd start expanding these > > macros. IF the RESERVE_SPACE BUG_ON is important enough it should move > > into xdr_reserve_space or a variant thereof, or otherweise just removed. > > Yeah, those will go later. > > Ideas for how to sequence these changes better welcomed. In this case I'd suggest just killing ADJUST_ARGS in this patch. In general if there's something looking ugly that gets killed or cleaned up later I simply mention it in the patch description. > > > + xdr->p = savep; > > > + xdr->iov->iov_len = ((char *)resp->xdr.p) > > > + - (char *)resp->xdr.buf->head[0].iov_base; > > > > Where is this coming from? Looks like deep magic to be and would > > benefit from comments, symbolic names and/or a helper. > > OK, may be worth a comment even if it's only a temporary thing. I didn't know it was going away when I read the patch. Keeping the code as-is is fine with me, but maybe a little comment in the changelog might help reviwers the look at the next iteration. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
