Re: [PATCH] NFSD: do not return nfserr_symlink for the LINK operation

[Benny Halevy <bhalevy@panasas.com>]

On Mar. 19, 2009, 10:18 +0200, Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
> Benny Halevy wrote:
>> On Mar. 19, 2009, 8:59 +0200, Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>>> Benny Halevy wrote:
>>>> As reported by Ni Wenjuan <niwj@cn.fujitsu.com> on 2009-03-05:
>>>>> the result of newpynfs test case of  LINK4a .  if you link with target directoty
>>>>> is a symbole file,it should get NFS4ERR_NOTDIR ,instead got NFS4ERR_SYMLINK.
>>>> THE LINK operation doesn't list NFS4ERR_SYMLINK as a valid error in the spec.
>>>> Although NFS4ERR_SYMLINK seems like a reasonable error and even though
>>>> it was added for LINK in NFSv4.1 we should still return NFSERR_NOTDIR for
>>>> nfsv[234].
>>> HI,Benny:
>>> There are other places that have the same problem,i'm preparing a all-in-one
>>> patch.^!^
>> Cool.
>> Bruce, please ignore this patch then.
>>
>> Thanks!
> 
> There are four placees that returned inappropriate err nfserr_symlink accroding to 
> newpynfs test #LINK4a#LOOKP2a#OPCF3a#SATT12a.nfserr_symlink do not listed
> in these operations's err list in the spec.
> For LINK and LOOKUPP operation,nfserr_notdir should be returned.
> For OPEN_CONFIRM and SETATTR operation,nfserr_inval should be returned.

Is the issue with SETATTR limited to length-changing ones?
Is so, I think it should be indicated in the commit message.

Other than that, minor style nit: there seems to be missing
space character before all closing comments "*/"...

Benny

> 
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> 
> ---
>  fs/nfsd/nfs4proc.c  |    6 +++++-
>  fs/nfsd/nfs4state.c |    6 +++++-
>  fs/nfsd/vfs.c       |    6 ++++++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 9fa60a3..d25e7cf 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -493,8 +493,12 @@ nfsd4_lookupp(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		return nfserr_noent;
>  	}
>  	fh_put(&tmp_fh);
> -	return nfsd_lookup(rqstp, &cstate->current_fh,
> +	ret = nfsd_lookup(rqstp, &cstate->current_fh,
>  			   "..", 2, &cstate->current_fh);
> +	/* nfserr_symlink returned is inappropriate for LOOKUPP*/
> +	if (ret == nfserr_symlink)
> +		ret = nfserr_notdir;
> +	return ret;
>  }
>  
>  static __be32
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b6f60f4..fe274fc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2234,8 +2234,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  			cstate->current_fh.fh_dentry->d_name.name);
>  
>  	status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0);
> -	if (status)
> +	if (status) {
> +		/* nfserr_symlink returned is inappropriate for OPEN_CONFIRM*/
> +		if (status == nfserr_symlink)
> +			status = nfserr_inval;
>  		return status;
> +	}
>  
>  	nfs4_lock_state();
>  
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6e50aaa..69eba75 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -397,6 +397,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  		if (EX_ISSYNC(fhp->fh_export))
>  			write_inode_now(inode, 1);
>  out:
> +	/* nfserr_symlink returned is inappropriate for SETATTR*/
> +	if (err == nfserr_symlink)
> +		err = nfserr_inval;
>  	return err;
>  
>  out_nfserr:
> @@ -1637,6 +1640,9 @@ out_dput:
>  out_unlock:
>  	fh_unlock(ffhp);
>  out:
> +	/* nfserr_symlink returned is inappropriate for LINK*/
> +	if (err == nfserr_symlink)
> +		err = nfserr_notdir;
>  	return err;
>  
>  out_nfserr: