[PATCH 0/2] decode_cb_op_status fix

[PATCH 0/2] decode_cb_op_status fix

[Benny Halevy]

Chuck, it looks like the following patch introduced a couple in
handling of the operation status.

85a5648 NFSD: Update XDR decoders in NFSv4 callback client

The first patch in this series fixes the bugs and the second
reintroduces the handling of nfserr into decode_cb_op_status
as it used to be in decode_cb_op_hdr, since I think it's simpler
with respect to the current usage (vs. theoretical generic use of
decode_cb_op_status to decode the status and return it in an output var).

Benny

[PATCH 1/2] NFSD: use nfserr for status after decode_cb_op_status

[Benny Halevy]

Bugs introduced in 85a56480191ca9f08fc775c129b9eb5c8c1f2c05
"NFSD: Update XDR decoders in NFSv4 callback client"

Cc: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4callback.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 21a63da..5a6dcf8 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -484,7 +484,7 @@ static int decode_cb_sequence4res(struct xdr_stream *xdr,
 out:
 	return status;
 out_default:
-	return nfs_cb_stat_to_errno(status);
+	return nfs_cb_stat_to_errno(nfserr);
 }
 
 /*
@@ -564,11 +564,9 @@ static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
 	if (unlikely(status))
 		goto out;
 	if (unlikely(nfserr != NFS4_OK))
-		goto out_default;
+		status = nfs_cb_stat_to_errno(nfserr);
 out:
 	return status;
-out_default:
-	return nfs_cb_stat_to_errno(status);
 }
 
 /*
-- 
1.7.3.4

Re: [PATCH 1/2] NFSD: use nfserr for status after decode_cb_op_status

[Chuck Lever]

On Jan 13, 2011, at 4:25 AM, Benny Halevy wrote:

> Bugs introduced in 85a56480191ca9f08fc775c129b9eb5c8c1f2c05
> "NFSD: Update XDR decoders in NFSv4 callback client"
> 
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> fs/nfsd/nfs4callback.c |    6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 21a63da..5a6dcf8 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -484,7 +484,7 @@ static int decode_cb_sequence4res(struct xdr_stream *xdr,
> out:
> 	return status;
> out_default:
> -	return nfs_cb_stat_to_errno(status);
> +	return nfs_cb_stat_to_errno(nfserr);

Good fix.

> }
> 
> /*
> @@ -564,11 +564,9 @@ static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
> 	if (unlikely(status))
> 		goto out;
> 	if (unlikely(nfserr != NFS4_OK))
> -		goto out_default;
> +		status = nfs_cb_stat_to_errno(nfserr);
> out:
> 	return status;
> -out_default:
> -	return nfs_cb_stat_to_errno(status);

Good fix, but I would rather keep the same style here that every other NFS decoder uses here.  The "goto out_default" -- we want to keep the non-common cases out of the mainline code path.  It is Bruce-preferred (tm) style to branch on error.

> }
> 
> /*
> -- 
> 1.7.3.4
> 

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

[PATCH 2/2] NFSD: do common error handling in decode_cb_op_status

[Benny Halevy]

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.

Cc: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 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

Re: [PATCH 2/2] NFSD: do common error handling in decode_cb_op_status

[Chuck Lever]

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@oracle.com>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> 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

Re: [PATCH 2/2] NFSD: do common error handling in decode_cb_op_status

[J. Bruce Fields]

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@oracle.com>
> > Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> > ---
> > 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
> 
> 
> 
> 

Re: [PATCH 0/2] decode_cb_op_status fix

[J. Bruce Fields]

On Thu, Jan 13, 2011 at 11:23:56AM +0200, Benny Halevy wrote:
> Chuck, it looks like the following patch introduced a couple in
> handling of the operation status.
> 
> 85a5648 NFSD: Update XDR decoders in NFSv4 callback client
> 
> The first patch in this series fixes the bugs and the second
> reintroduces the handling of nfserr into decode_cb_op_status
> as it used to be in decode_cb_op_hdr, since I think it's simpler
> with respect to the current usage (vs. theoretical generic use of
> decode_cb_op_status to decode the status and return it in an output var).

Looks OK to me, thanks.

--b.

Re: [PATCH 0/2] decode_cb_op_status fix

[Chuck Lever]

I never saw these.  Can you resend?

On Jan 18, 2011, at 4:57 PM, J. Bruce Fields wrote:

> On Thu, Jan 13, 2011 at 11:23:56AM +0200, Benny Halevy wrote:
>> Chuck, it looks like the following patch introduced a couple in
>> handling of the operation status.
>> 
>> 85a5648 NFSD: Update XDR decoders in NFSv4 callback client
>> 
>> The first patch in this series fixes the bugs and the second
>> reintroduces the handling of nfserr into decode_cb_op_status
>> as it used to be in decode_cb_op_hdr, since I think it's simpler
>> with respect to the current usage (vs. theoretical generic use of
>> decode_cb_op_status to decode the status and return it in an output var).
> 
> Looks OK to me, thanks.
> 
> --b.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com