From: Trond Myklebust Subject: Re: compound header status Date: Thu, 21 Feb 2008 16:15:40 -0500 Message-ID: <1203628540.8258.28.camel@heimdal.trondhjem.org> References: Mime-Version: 1.0 Content-Type: text/plain Cc: nfsv4@linux-nfs.org, pnfs@linux-nfs.org, linux-nfs@vger.kernel.org To: "Halevy, Benny" Return-path: Received: from mx2.netapp.com ([216.240.18.37]:39066 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753708AbYBUVPm (ORCPT ); Thu, 21 Feb 2008 16:15:42 -0500 In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: 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