Trond, we had a discussion today in Austin about the COMPOUND result header
status. Looking at fs/nfs/nfs4xdr.c we saw that some decoding routines
return an error based on hdr.status after all the compound operations
decoding completed successfully.
For example:
static int nfs4_xdr_dec_setclientid_confirm(struct rpc_rqst *req, __be32 *p, struct nfs_fsinfo *fsinfo)
{
struct xdr_stream xdr;
struct compound_hdr hdr;
int status;
xdr_init_decode(&xdr, &req->rq_rcv_buf, p);
status = decode_compound_hdr(&xdr, &hdr);
if (!status)
status = decode_setclientid_confirm(&xdr);
if (!status)
status = decode_putrootfh(&xdr);
if (!status)
status = decode_fsinfo(&xdr, fsinfo);
if (!status)
status = -nfs4_stat_to_errno(hdr.status);
^^^^^^^^^^
return status;
}
It seems like this is unneeded since hdr.status must be NFS_OK if all operations
in the compound succeeded (and assuming we decode the results from all ops we sent).
The first error will be found by decode_op_hdr called by any of the decoding
routines for the individual ops.
If hdr.status contains an error in this case, the server must be broken, isn't it?
Also, nfs4_stat_to_errno is not handling the success case very efficiently as
it looks for it in its mapping table like any other error code.
One more thing, all use sites for nfs4_stat_to_errno negate its return value
so it might make sense to return a negative error from nfs4_stat_to_errno rather
than negating its return value everywhere.
Benny
On Thu, 2008-02-21 at 00:18 -0500, Halevy, Benny wrote:
> Trond, we had a discussion today in Austin about the COMPOUND result header
> status. Looking at fs/nfs/nfs4xdr.c we saw that some decoding routines
> return an error based on hdr.status after all the compound operations
> decoding completed successfully.
>
> For example:
> static int nfs4_xdr_dec_setclientid_confirm(struct rpc_rqst *req, __be32 *p, struct nfs_fsinfo *fsinfo)
> {
> struct xdr_stream xdr;
> struct compound_hdr hdr;
> int status;
>
> xdr_init_decode(&xdr, &req->rq_rcv_buf, p);
> status = decode_compound_hdr(&xdr, &hdr);
> if (!status)
> status = decode_setclientid_confirm(&xdr);
> if (!status)
> status = decode_putrootfh(&xdr);
> if (!status)
> status = decode_fsinfo(&xdr, fsinfo);
> if (!status)
> status = -nfs4_stat_to_errno(hdr.status);
> ^^^^^^^^^^
> return status;
> }
>
> It seems like this is unneeded since hdr.status must be NFS_OK if all operations
> in the compound succeeded (and assuming we decode the results from all ops we sent).
> The first error will be found by decode_op_hdr called by any of the decoding
> routines for the individual ops.
> If hdr.status contains an error in this case, the server must be broken, isn't it?
Agreed. I can only see 3 cases where we do this, but it would be nice to
fix them.
> One more thing, all use sites for nfs4_stat_to_errno negate its return value
> so it might make sense to return a negative error from nfs4_stat_to_errno rather
> than negating its return value everywhere.
If we do, then we should fix up nfs_stat_to_errno() too. It will just
get confusing if we establish differing standards between NFSv2/v3 and
NFSv4.
Trond