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 <chuck.lever@xxxxxxxxxx>
> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> ---
> 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
--
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
