Re: [ 117/135] NFS: return error from decode_getfh in decode open

[Myklebust, Trond]

On Wed, 2012-09-19 at 04:39 +0100, Ben Hutchings wrote:
> On Tue, 2012-09-18 at 22:26 -0300, Herton Ronaldo Krzesinski wrote:
> > On Mon, Sep 17, 2012 at 01:38:22AM +0100, Ben Hutchings wrote:
> > > 3.2-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > I'm not sure whether my expansion of the fix is correct here.
> > > 
> > > ------------------
> > > 
> > > From: Weston Andros Adamson <dros@xxxxxxxxxx>
> > > 
> > > commit 01913b49cf1dc6409a07dd2a4cc6af2e77f3c410 upstream.
> > > 
> > > If decode_getfh failed, nfs4_xdr_dec_open would return 0 since the last
> > > decode_* call must have succeeded.
> > > 
> > > Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxx>
> > > Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> > > [bwh: Backported to 3.2: this function makes two more function calls
> > >  (no longer present in mainline) with the same issue, so fix them up
> > >  similarly.]
> > > Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > > ---
> > >  fs/nfs/nfs4xdr.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > --- a/fs/nfs/nfs4xdr.c
> > > +++ b/fs/nfs/nfs4xdr.c
> > > @@ -6113,12 +6113,15 @@ static int nfs4_xdr_dec_open(struct rpc_
> > >  	status = decode_open(xdr, res);
> > >  	if (status)
> > >  		goto out;
> > > -	if (decode_getfh(xdr, &res->fh) != 0)
> > > +	status = decode_getfh(xdr, &res->fh);
> > > +	if (status)
> > >  		goto out;
> > > -	if (decode_getfattr(xdr, res->f_attr, res->server,
> > > -				!RPC_IS_ASYNC(rqstp->rq_task)) != 0)
> > > +	status = decode_getfattr(xdr, res->f_attr, res->server,
> > > +				 !RPC_IS_ASYNC(rqstp->rq_task));
> > 
> > I'm also not sure about the expansion of the fix here, not knowing very
> > much about nfs. It seems that the code in some cases want to discard the
> > status from decode_getfattr, for example nfs4_xdr_dec_rename is one case
> > which does the same. Is it ok to return the status of decode_getfattr on
> > nfs4_xdr_dec_open here? Or should it remain like it was before?
> 
> It looks like we try to get the file and directory attributes, and those
> are nice to have but the open operation is still successful even if we
> can't get them.  So my extra 'fixes' here are wrong.  Trond, is this
> right?

Hi Ben,

We do not want to fail the RPC call in the case where the attribute
decode failed; we already have ways to recover from that situation in
the higher layers of the open code (we send a separate getattr RPC
call). If, on the other hand, the filehandle decode fails, then we are
screwed since we can't know exactly which file was just opened and this
would be the reason for Dros's patch.

So please do leave the decode_getfattr() line as it was, and apply the
change to the decode_getfh() line only.

Cheers
  Trond


> Ben.
> 
> > > +	if (status)
> > >  		goto out;
> > > -	if (decode_restorefh(xdr) != 0)
> > > +	status = decode_restorefh(xdr);
> > > +	if (status)
> > >  		goto out;
> > >  	decode_getfattr(xdr, res->dir_attr, res->server,
> > >  			!RPC_IS_ASYNC(rqstp->rq_task));
> > 
> 

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_