| Message ID | 1395537141-10389-19-git-send-email-bfields@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> #define RESERVE_SPACE(nbytes) do { \ > - p = resp->xdr.p; \ > - BUG_ON(p + XDR_QUADLEN(nbytes) > resp->xdr.end); \ > + p = xdr_reserve_space(&resp->xdr, nbytes); \ > + BUG_ON(!p); \ > } 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. > + if (nfserr) { > + xdr->p -= 2; > + xdr->iov->iov_len -= 8; > + if (nfserr) { > + xdr->p--; > + xdr->iov->iov_len -= 4; > return nfserr; > + } > + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 22, 2014 at 11:43:36PM -0700, Christoph Hellwig wrote: > > #define RESERVE_SPACE(nbytes) do { \ > > - p = resp->xdr.p; \ > > - BUG_ON(p + XDR_QUADLEN(nbytes) > resp->xdr.end); \ > > + p = xdr_reserve_space(&resp->xdr, nbytes); \ > > + BUG_ON(!p); \ > > } 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. > > + if (nfserr) { > > + xdr->p -= 2; > > + xdr->iov->iov_len -= 8; > > > > + if (nfserr) { > > + xdr->p--; > > + xdr->iov->iov_len -= 4; > > return nfserr; > > + } > > > > + 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. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 9e7cb05..e5cc711 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -1242,7 +1242,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp, svcxdr_init_encode(rqstp, resp); resp->tagp = resp->xdr.p; /* reserve space for: taglen, tag, and opcnt */ - resp->xdr.p += 2 + XDR_QUADLEN(args->taglen); + xdr_reserve_space(&resp->xdr, 8 + args->taglen); resp->taglen = args->taglen; resp->tag = args->tag; resp->opcnt = 0; diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 6025713..d665c60 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -1748,10 +1748,10 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c) } #define RESERVE_SPACE(nbytes) do { \ - p = resp->xdr.p; \ - BUG_ON(p + XDR_QUADLEN(nbytes) > resp->xdr.end); \ + p = xdr_reserve_space(&resp->xdr, nbytes); \ + BUG_ON(!p); \ } while (0) -#define ADJUST_ARGS() resp->xdr.p = p +#define ADJUST_ARGS() WARN_ON_ONCE(p != resp->xdr.p) \ /* Encode as an array of strings the string given with components * separated @sep, escaped with esc_enter and esc_exit. @@ -3040,8 +3040,11 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr, read->rd_offset, resp->rqstp->rq_vec, read->rd_vlen, &maxcount); - if (nfserr) + if (nfserr) { + xdr->p -= 2; + xdr->iov->iov_len -= 8; return nfserr; + } eof = (read->rd_offset + maxcount >= read->rd_fhp->fh_dentry->d_inode->i_size); @@ -3094,9 +3097,12 @@ nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd */ nfserr = nfsd_readlink(readlink->rl_rqstp, readlink->rl_fhp, page, &maxcount); if (nfserr == nfserr_isdir) - return nfserr_inval; - if (nfserr) + nfserr = nfserr_inval; + if (nfserr) { + xdr->p--; + xdr->iov->iov_len -= 4; return nfserr; + } WRITE32(maxcount); ADJUST_ARGS(); @@ -3196,8 +3202,9 @@ nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4 return 0; err_no_verf: - p = savep; - ADJUST_ARGS(); + xdr->p = savep; + xdr->iov->iov_len = ((char *)resp->xdr.p) + - (char *)resp->xdr.buf->head[0].iov_base; return nfserr; }