On May. 11, 2008, 16:26 -0700, Trond Myklebust <trond.myklebust@xxxxxxxxxx> wrote:
> On Sun, 2008-05-11 at 16:21 -0700, Benny Halevy wrote:
>> The compound header status must be equivalent to the
>> status of the last operation in the compound results.
>> In certain cases like lack of resources or xdr decoding error,
>> the nfs server may return a non-zero status in the compound header
>> which is not returned by any operation. In this case we would
>> notice that today when looking for the respective operations
>> code in the results and we return -EIO when we cannot find it.
>> This patch fixes that by returning the status available in the
>> compound header instead.
>>
>> This patch also fixes 3 call sites where we looked at the compound
>> hdr.status in the success case which is useless (yet benign).
>> These are nfs4_xdr_dec_{fsinfo,setclientid,setclientid_confirm}
>>
>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
>> ---
>> fs/nfs/nfs4xdr.c | 88 +++++++++++++++++++++++++++---------------------------
>> 1 files changed, 44 insertions(+), 44 deletions(-)
>>
>> Changes in PATCH v3:
>> * nfs4_fixup_status made static inline (with very minor increase
>> in code size over macro)
>>
>> Changes in PATCH v2:
>> * rebased onto 2.6.26 (off of linux-2.6 28a4acb4)
>> * fixed checkpatch.pl nits
>> * do not fixup status in nfs4_xdr_enc_setacl
>>
>>
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 5a2d649..e6f7f0b 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -3791,6 +3791,13 @@ static int decode_delegreturn(struct xdr_stream *xdr)
>> return decode_op_hdr(xdr, OP_DELEGRETURN);
>> }
>>
>> +static inline int nfs4_fixup_status(int status, int hdr_status)
>> +{
>> + if (likely(!status))
>> + return 0;
>> + return nfs4_stat_to_errno(hdr_status);
>> +}
>> +
>> /*
>> * Decode OPEN_DOWNGRADE response
>> */
>> @@ -3812,7 +3819,7 @@ static int nfs4_xdr_dec_open_downgrade(struct rpc_rqst *rqstp, __be32 *p, struct
>> goto out;
>> decode_getfattr(&xdr, res->fattr, res->server);
>> out:
>> - return status;
>> + return nfs4_fixup_status(status, hdr.status);
>> }
>
> NACK.
>
> This is still screwed up: if the getattr above fails, then your change
> will propagate that error despite the fact that we don't care. Please
> see the same comments on earlier drafts of this patch.
>
Trond, sorry, but I don't see the problem.
If all ops before the getattr succeeded then "status" will be zero
regardless if getattr failed or not and nfs4_fixup_status will
return success.
If any other op before the getattr failed then getfattr will not get
processed and hdr.status will contain the latest failure.
Benny
--
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
