decode_cb_op_status fix

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

Cc Chuck Lever <[email protected]>

Signed-off-by Benny Halevy <[email protected]>

---
fs/nfsd/nfs4callback.c | 6 ++----

1 files changed, 2 insertions(+), 4 deletions(-)

fs/nfsd/nfs4callback.c +0 -0

index 21a63da..5a6dcf8 100644
@@ -484,7 +484,7 @@ static int decode_cb_sequence4res(struct xdr_stream *xdr,

out:

	return status;

out_default:

}

/*

Signature · 1 line

1.7.3.4

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 <[email protected]>

Signed-off-by Benny Halevy <[email protected]>

---
fs/nfsd/nfs4callback.c | 22 +++++++---------------

1 files changed, 7 insertions(+), 15 deletions(-)

fs/nfsd/nfs4callback.c +5 -3

index 5a6dcf8..6f69645 100644
@@ -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);

fs/nfsd/nfs4callback.c +1 -4

@@ -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;

fs/nfsd/nfs4callback.c +0 -2

-out_default:
-	return nfs_cb_stat_to_errno(nfserr);

}

/*

fs/nfsd/nfs4callback.c +1 -6

@@ -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;
}

Signature · 1 line

1.7.3.4

Quoted 6 lines · 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.

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.

Quoted 85 lines

Cc Chuck Lever <[email protected]>

Signed-off-by Benny Halevy <[email protected]>

---
fs/nfsd/nfs4callback.c | 22 +++++++---------------

1 files changed, 7 insertions(+), 15 deletions(-)

fs/nfsd/nfs4callback.c +5 -3

index 5a6dcf8..6f69645 100644
@@ -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);

fs/nfsd/nfs4callback.c +1 -4

@@ -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;

fs/nfsd/nfs4callback.c +0 -2

-out_default:
-	return nfs_cb_stat_to_errno(nfserr);

}

/*

fs/nfsd/nfs4callback.c +1 -6

@@ -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;
}

Signature · 1 line

1.7.3.4

Signature · 2 lines

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

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).

Quoted 10 lines · 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).

Looks OK to me, thanks.

--b.