Return-Path: Received: from rcsinet10.oracle.com ([148.87.113.121]:60040 "EHLO rcsinet10.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751698Ab1ARWW0 convert rfc822-to-8bit (ORCPT ); Tue, 18 Jan 2011 17:22:26 -0500 Subject: Re: [PATCH 2/2] NFSD: do common error handling in decode_cb_op_status Content-Type: text/plain; charset=us-ascii From: Chuck Lever In-Reply-To: <1294910741-18407-1-git-send-email-bhalevy@panasas.com> Date: Tue, 18 Jan 2011 17:20:41 -0500 Cc: " J. Bruce Fields" , linux-nfs@vger.kernel.org Message-Id: References: <4D2EC4AC.9090205@panasas.com> <1294910741-18407-1-git-send-email-bhalevy@panasas.com> To: Benny Halevy Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Jan 13, 2011, at 4:25 AM, Benny Halevy wrote: > Rather than handling the cb operation status in each and every > decode_cb_op_status's caller, just put the common code there. > > If need be, and some caller really needs to see the original status > this patch can be reverted but right now there's no real need for the > abstract functionality. a) all the other new status decoders do it the generic way, so this would be a special case. We already know it's not needed here at this point, but it is consistent with the other decoders. b) we lose the distinction between a returned status code and a decoding error. I recall finding a few bugs in the other decoders where the two are conflated. I'd rather not apply this one. I think it's premature and unneeded optimization. I'm sure I'll be voted down, though. > > Cc: Chuck Lever > Signed-off-by: Benny Halevy > --- > fs/nfsd/nfs4callback.c | 22 +++++++--------------- > 1 files changed, 7 insertions(+), 15 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 5a6dcf8..6f69645 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -248,11 +248,11 @@ static int nfs_cb_stat_to_errno(int status) > return -status; > } > > -static int decode_cb_op_status(struct xdr_stream *xdr, enum nfs_opnum4 expected, > - enum nfsstat4 *status) > +static int decode_cb_op_status(struct xdr_stream *xdr, enum nfs_opnum4 expected) > { > __be32 *p; > u32 op; > + enum nfsstat4 status; > > p = xdr_inline_decode(xdr, 4 + 4); > if (unlikely(p == NULL)) > @@ -260,7 +260,9 @@ static int decode_cb_op_status(struct xdr_stream *xdr, enum nfs_opnum4 expected, > op = be32_to_cpup(p++); > if (unlikely(op != expected)) > goto out_unexpected; > - *status = be32_to_cpup(p); > + status = be32_to_cpup(p); > + if (unlikely(status != NFS4_OK)) > + return nfs_cb_stat_to_errno(status); > return 0; > out_overflow: > print_overflow_msg(__func__, xdr); > @@ -469,22 +471,17 @@ out_overflow: > static int decode_cb_sequence4res(struct xdr_stream *xdr, > struct nfsd4_callback *cb) > { > - enum nfsstat4 nfserr; > int status; > > if (cb->cb_minorversion == 0) > return 0; > > - status = decode_cb_op_status(xdr, OP_CB_SEQUENCE, &nfserr); > + status = decode_cb_op_status(xdr, OP_CB_SEQUENCE); > if (unlikely(status)) > goto out; > - if (unlikely(nfserr != NFS4_OK)) > - goto out_default; > status = decode_cb_sequence4resok(xdr, cb); > out: > return status; > -out_default: > - return nfs_cb_stat_to_errno(nfserr); > } > > /* > @@ -547,7 +544,6 @@ static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp, > struct nfsd4_callback *cb) > { > struct nfs4_cb_compound_hdr hdr; > - enum nfsstat4 nfserr; > int status; > > status = decode_cb_compound4res(xdr, &hdr); > @@ -560,11 +556,7 @@ static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp, > goto out; > } > > - status = decode_cb_op_status(xdr, OP_CB_RECALL, &nfserr); > - if (unlikely(status)) > - goto out; > - if (unlikely(nfserr != NFS4_OK)) > - status = nfs_cb_stat_to_errno(nfserr); > + status = decode_cb_op_status(xdr, OP_CB_RECALL); > out: > return status; > } > -- > 1.7.3.4 > -- Chuck Lever chuck[dot]lever[at]oracle[dot]com