On Tue, Jan 18, 2011 at 05:20:41PM -0500, Chuck Lever wrote:
>
> 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.
I think what you mean to say is: "if you vote me down, I shall become
more powerful than you can possibly imagine."
Anyway, I suppose if we fix the callback error handling soon (as we
should) then we'd be reverting this soon anyway, so I'll drop this one
for now.
--b.
>
> >
> > 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
