NFS4ERR_SYMLINK error

NFS4ERR_SYMLINK error

[Ni Wenjuan]

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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
in the spec, or something we need to fix?

Re: NFS4ERR_SYMLINK error

[Benny Halevy]

On Mar. 05, 2009, 11:32 +0200, Ni Wenjuan <niwj@cn.fujitsu.com> wrote:
> 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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
> NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
> in the spec, or something we need to fix?

Although NFSv4.1 adds NFS4ERR_SYMLINK to LINK's allowed errors list
(and this might be an indication for it being an oversight in rfc3530),
NFSv4.0 doesn't allow it so this needs to be fixed.

Can you please test the following patch?

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---

git diff --stat -p fs/nfsd/vfs.c
 fs/nfsd/vfs.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 6e50aaa..9165b1f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1637,6 +1637,9 @@ out_dput:
 out_unlock:
 	fh_unlock(ffhp);
 out:
+	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
+	if (err == nfserr_symlink)
+		err = nfserr_notdir;
 	return err;
 
 out_nfserr:


> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS4ERR_SYMLINK error

[J. Bruce Fields]

On Thu, Mar 05, 2009 at 12:09:40PM +0200, Benny Halevy wrote:
> On Mar. 05, 2009, 11:32 +0200, Ni Wenjuan <niwj@cn.fujitsu.com> wrote:
> > 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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
> > NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
> > in the spec, or something we need to fix?
> 
> Although NFSv4.1 adds NFS4ERR_SYMLINK to LINK's allowed errors list
> (and this might be an indication for it being an oversight in rfc3530),

The error lists in rfc3530 are known to be incomplete in some cases, so
before adding an exception like this I'd like something more.  (E.g.:
does this cause any client or application to fail?  Is there some
logical reason notdir is a more useful error than symlink?)

--b.

> NFSv4.0 doesn't allow it so this needs to be fixed.
> 
> Can you please test the following patch?
> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> 
> git diff --stat -p fs/nfsd/vfs.c
>  fs/nfsd/vfs.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6e50aaa..9165b1f 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1637,6 +1637,9 @@ out_dput:
>  out_unlock:
>  	fh_unlock(ffhp);
>  out:
> +	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
> +	if (err == nfserr_symlink)
> +		err = nfserr_notdir;
>  	return err;
>  
>  out_nfserr:
> 
> 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS4ERR_SYMLINK error

[Benny Halevy]

On Mar. 06, 2009, 23:32 +0200, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Thu, Mar 05, 2009 at 12:09:40PM +0200, Benny Halevy wrote:
>> On Mar. 05, 2009, 11:32 +0200, Ni Wenjuan <niwj@cn.fujitsu.com> wrote:
>>> 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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
>>> NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
>>> in the spec, or something we need to fix?
>> Although NFSv4.1 adds NFS4ERR_SYMLINK to LINK's allowed errors list
>> (and this might be an indication for it being an oversight in rfc3530),
> 
> The error lists in rfc3530 are known to be incomplete in some cases, so
> before adding an exception like this I'd like something more.  (E.g.:
> does this cause any client or application to fail?  Is there some
> logical reason notdir is a more useful error than symlink?)

FWIW, the linux nfs client translates NFS4ERR_SYMLINK to -ELOOP
which is awkward and less descriptive to the app / user than
-ENOTDIR.  That said, I don't think a careful client implementation
should ever get NFS4ERR_SYMLINK if it stats the directory it operates
on before sending the link op (or lookup, create, rename, etc.) to make
sure it is indeed a directory, right?.

Benny

> 
> --b.
> 
>> NFSv4.0 doesn't allow it so this needs to be fixed.
>>
>> Can you please test the following patch?
>>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>
>> git diff --stat -p fs/nfsd/vfs.c
>>  fs/nfsd/vfs.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 6e50aaa..9165b1f 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -1637,6 +1637,9 @@ out_dput:
>>  out_unlock:
>>  	fh_unlock(ffhp);
>>  out:
>> +	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
>> +	if (err == nfserr_symlink)
>> +		err = nfserr_notdir;
>>  	return err;
>>  
>>  out_nfserr:
>>
>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS4ERR_SYMLINK error

[J. Bruce Fields]

On Sat, Mar 07, 2009 at 08:59:16PM +0200, Benny Halevy wrote:
> On Mar. 06, 2009, 23:32 +0200, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Thu, Mar 05, 2009 at 12:09:40PM +0200, Benny Halevy wrote:
> >> On Mar. 05, 2009, 11:32 +0200, Ni Wenjuan <niwj@cn.fujitsu.com> wrote:
> >>> 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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
> >>> NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
> >>> in the spec, or something we need to fix?
> >> Although NFSv4.1 adds NFS4ERR_SYMLINK to LINK's allowed errors list
> >> (and this might be an indication for it being an oversight in rfc3530),
> > 
> > The error lists in rfc3530 are known to be incomplete in some cases, so
> > before adding an exception like this I'd like something more.  (E.g.:
> > does this cause any client or application to fail?  Is there some
> > logical reason notdir is a more useful error than symlink?)
> 
> FWIW, the linux nfs client translates NFS4ERR_SYMLINK to -ELOOP
> which is awkward and less descriptive to the app / user than
> -ENOTDIR.

Hm, OK.  If we fix this will -ELOOP then become reasonable for our
remaining NFS4ERR_SYMLINK returns?

> That said, I don't think a careful client implementation
> should ever get NFS4ERR_SYMLINK if it stats the directory it operates
> on before sending the link op (or lookup, create, rename, etc.) to make
> sure it is indeed a directory, right?.

That sounds racy.

--b.

Re: NFS4ERR_SYMLINK error

[Trond Myklebust]

On Mon, 2009-03-09 at 14:06 -0400, J. Bruce Fields wrote:
> > That said, I don't think a careful client implementation
> > should ever get NFS4ERR_SYMLINK if it stats the directory it operates
> > on before sending the link op (or lookup, create, rename, etc.) to make
> > sure it is indeed a directory, right?.
> 
> That sounds racy.

The LINK op identifies the target directory by its filehandle, so I
can't see that there can be any race: the server isn't supposed to be
able to morph a directory into a symlink without changing its
filehandle.

Trond

Re: NFS4ERR_SYMLINK error

[Yang Hongyang]

J. Bruce Fields wrote:
> On Sat, Mar 07, 2009 at 08:59:16PM +0200, Benny Halevy wrote:
>> On Mar. 06, 2009, 23:32 +0200, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>> On Thu, Mar 05, 2009 at 12:09:40PM +0200, Benny Halevy wrote:
>>>> On Mar. 05, 2009, 11:32 +0200, Ni Wenjuan <niwj@cn.fujitsu.com> wrote:
>>>>> 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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
>>>>> NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
>>>>> in the spec, or something we need to fix?
>>>> Although NFSv4.1 adds NFS4ERR_SYMLINK to LINK's allowed errors list
>>>> (and this might be an indication for it being an oversight in rfc3530),
>>> The error lists in rfc3530 are known to be incomplete in some cases, so
>>> before adding an exception like this I'd like something more.  (E.g.:
>>> does this cause any client or application to fail?  Is there some
>>> logical reason notdir is a more useful error than symlink?)
>> FWIW, the linux nfs client translates NFS4ERR_SYMLINK to -ELOOP
>> which is awkward and less descriptive to the app / user than
>> -ENOTDIR.
> 
> Hm, OK.  If we fix this will -ELOOP then become reasonable for our
> remaining NFS4ERR_SYMLINK returns?

Bruce,Do you think we do not need to fix this?

> 
>> That said, I don't think a careful client implementation
>> should ever get NFS4ERR_SYMLINK if it stats the directory it operates
>> on before sending the link op (or lookup, create, rename, etc.) to make
>> sure it is indeed a directory, right?.
> 
> That sounds racy.
> 
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


-- 
Regards
Yang Hongyang

Re: NFS4ERR_SYMLINK error

[J. Bruce Fields]

On Fri, Mar 13, 2009 at 04:13:39PM +0800, Yang Hongyang wrote:
> J. Bruce Fields wrote:
> > On Sat, Mar 07, 2009 at 08:59:16PM +0200, Benny Halevy wrote:
> >> On Mar. 06, 2009, 23:32 +0200, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >>> On Thu, Mar 05, 2009 at 12:09:40PM +0200, Benny Halevy wrote:
> >>>> On Mar. 05, 2009, 11:32 +0200, Ni Wenjuan <niwj@cn.fujitsu.com> wrote:
> >>>>> 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  don't list NFS4ERR_SYMLINK as valid errors in the spec. But
> >>>>> NFS4ERR_SYMLINK seems like a reasonable error.  Is this an oversight
> >>>>> in the spec, or something we need to fix?
> >>>> Although NFSv4.1 adds NFS4ERR_SYMLINK to LINK's allowed errors list
> >>>> (and this might be an indication for it being an oversight in rfc3530),
> >>> The error lists in rfc3530 are known to be incomplete in some cases, so
> >>> before adding an exception like this I'd like something more.  (E.g.:
> >>> does this cause any client or application to fail?  Is there some
> >>> logical reason notdir is a more useful error than symlink?)
> >> FWIW, the linux nfs client translates NFS4ERR_SYMLINK to -ELOOP
> >> which is awkward and less descriptive to the app / user than
> >> -ENOTDIR.
> > 
> > Hm, OK.  If we fix this will -ELOOP then become reasonable for our
> > remaining NFS4ERR_SYMLINK returns?
> 
> Bruce,Do you think we do not need to fix this?

Given the spec and the client behavior, I guess it sounds reasonable to
fix it, if Benny could just resend with a new comment and signed-off-by.

--b.

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

[Benny Halevy]

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].

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/vfs.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 6e50aaa..9165b1f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1637,6 +1637,9 @@ out_dput:
 out_unlock:
 	fh_unlock(ffhp);
 out:
+	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
+	if (err == nfserr_symlink)
+		err = nfserr_notdir;
 	return err;
 
 out_nfserr:

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

[Yang Hongyang]

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.^!^

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/vfs.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6e50aaa..9165b1f 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1637,6 +1637,9 @@ out_dput:
>  out_unlock:
>  	fh_unlock(ffhp);
>  out:
> +	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
> +	if (err == nfserr_symlink)
> +		err = nfserr_notdir;
>  	return err;
>  
>  out_nfserr:
> 
> 


-- 
Regards
Yang Hongyang

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

[Benny Halevy]

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!

Benny

> 
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/vfs.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 6e50aaa..9165b1f 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -1637,6 +1637,9 @@ out_dput:
>>  out_unlock:
>>  	fh_unlock(ffhp);
>>  out:
>> +	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
>> +	if (err == nfserr_symlink)
>> +		err = nfserr_notdir;
>>  	return err;
>>  
>>  out_nfserr:
>>
>>
> 
> 

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

[Yang Hongyang]

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.

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:
-- 
1.6.0.3


> 
> Benny
> 
>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>> ---
>>>  fs/nfsd/vfs.c |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 6e50aaa..9165b1f 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -1637,6 +1637,9 @@ out_dput:
>>>  out_unlock:
>>>  	fh_unlock(ffhp);
>>>  out:
>>> +	/* nfserr_symlink returned from fh_verify is inappropriate for LINK */
>>> +	if (err == nfserr_symlink)
>>> +		err = nfserr_notdir;
>>>  	return err;
>>>  
>>>  out_nfserr:
>>>
>>>
>>
> 
> 


-- 
Regards
Yang Hongyang

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

[Benny Halevy]

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:

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

[Yang Hongyang]

Benny Halevy wrote:
> 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?

Sorry,Do not quite understand you:(What do you mean?

> 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 "*/"...

Thanks Benny,will fix it.

> 
> 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:
> 
> 


-- 
Regards
Yang Hongyang

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

[Yang Hongyang]

Benny Halevy wrote:
> 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.

By the way,you can add SOF if you agree with the patch:)

> 
> 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:
> 
> 


-- 
Regards
Yang Hongyang

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

[Yang Hongyang]

Benny Halevy wrote:
> 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
> 
v1->v2:update some code style problem
-------------------------

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.

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..9aaecaa 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..28e4688 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..015a655 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:
-- 
1.6.0.3


-- 
Regards
Yang Hongyang

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

[Benny Halevy]

On Mar. 19, 2009, 11:30 +0200, Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
> Benny Halevy wrote:
>> 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?
> 
> Sorry,Do not quite understand you:(What do you mean?

It seems like nfserr_symlink would have been returned for
SETATTR only if the client set the size attribute, where
SETATTR must only performed on a regular file (or a named
attribute, I believe).

[Sigh, looking at the code - it looks like we'll return
NFS4ERR_ISDIR for a length-changing SETATTR operating
on a directory.  This is fine in NFSv4.0 but this error
was removed for SETATTR in nfs4.1.  Note to self: revise
this in the nfs41 tree]

Benny

> 
>> 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 "*/"...
> 
> Thanks Benny,will fix it.
> 
>> 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:
>>
> 
> 

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

[Benny Halevy]

On Mar. 19, 2009, 11:34 +0200, Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
> Benny Halevy wrote:
>> 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.
> 
> By the way,you can add SOF if you agree with the patch:)

SOF meaning signoff?
Reviewed-by: Benny Halevy <bhalevy@panasas.com>
might be more appropriate in this case since
I had very little with developing the patch.

Benny

> 
>> 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:
>>
> 
> 

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

[J. Bruce Fields]

On Thu, Mar 19, 2009 at 05:46:45PM +0800, Yang Hongyang wrote:
> v1->v2:update some code style problem
> -------------------------
> 
> 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.

I thought Benny found that this also caused the linux client to return a
better error in one of these cases--could you confirm that and add a
mention of it in the commit message?

(I'm reluctant to take patches like this based *only* on the spec
language, partly because rfc 3530 is known to have a few oversights in
the error listings.)

I definitely appreciate people going through the pynfs tests and
investigating the results, but I don't want patch whose only
justification is that they quiet pynfs--we need to think about the
likely effect on real clients too.

--b.

> 
> 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..9aaecaa 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..28e4688 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..015a655 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:
> -- 
> 1.6.0.3
> 
> 
> -- 
> Regards
> Yang Hongyang

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

[J. Bruce Fields]

On Thu, Mar 19, 2009 at 04:00:35PM -0400, bfields wrote:
> On Thu, Mar 19, 2009 at 05:46:45PM +0800, Yang Hongyang wrote:
> > v1->v2:update some code style problem
> > -------------------------
> > 
> > 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.
> 
> I thought Benny found that this also caused the linux client to return a
> better error in one of these cases--could you confirm that and add a
> mention of it in the commit message?
> 
> (I'm reluctant to take patches like this based *only* on the spec
> language, partly because rfc 3530 is known to have a few oversights in
> the error listings.)
> 
> I definitely appreciate people going through the pynfs tests and
> investigating the results, but I don't want patch whose only
> justification is that they quiet pynfs--we need to think about the
> likely effect on real clients too.

Also, for example:

> > @@ -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;
> > +	}

open_confirm is done with the same filehandle that was returned from a
previous OPEN.  But an OPEN should never return the filehandle for a
symlink.  That means for us to reach this case, either the client or our
filesystem has a very serious bug.  Therefore, I'm not convinced that
getting the error return correct in this case is worth the trouble.

--b.

By the way, I have some old notes on the pynfs tests, below; they may be
wrong, or out of date, but perhaps they're of some use.


--fixme:

CID6: returning EXPIRED on OPEN with unconfirmed client, should return STALE.
(Why?)

--bugs, but probably not important:

OPCF3a, LINK4a, LOOKP2a, SATT12a: These 4 operations don't seem to
support NFS4ERR_SYMLINK, so we shouldn't be returning it.  This error
appears to be intentionally just for LOOKUP and OPEN, probably to help
implement O_NOFOLLOW or something.  It's fh_verify that's producing this
error.  There are some special cases in nfs4proc.c that map this error
to einval, but we should probably figure out some better way to deal
with this than special casing every other fh_verify error return....

CR14: create of dir with / in the name should return BADNAME or BADCHAR, not
INVAL.

CR13, LINK9, RNM11, CR13, LINK9, RNM11: We're returning the wrong error on use
of '.' as a filename.

CR12, OPEN15: attempt to set acl on create should return
NFS4ERR_ATTRNOTSUPP on fs that doesn't support acls, instead it returns
OK.

--might care:

LOOK6, LOOK9, OPEN17, RDDR11, RDDR12: permitting operations on directories of
mode 0.  This is probably a result of some logic that allows certain requests
on files and directories whose permissions would otherwise deny it, if the
requester is the owner of the file or directory.  I'm not sure this is a big
deal; the client can enforce this requirement, and there's no big security
hole as long as the owner could chmod the file anyway.

OPEN17: similar to above, but this on open of a file.  This seems odd.


--probably don't care:

COMP6: should return RESOURCE not GARBAGE_ARGS on a compound with 150 ops.

WRT5: connection reset doing a big write.

LINK8, OPEN13, RM5, RNM8, RNM9, LOOK7: these result from our decision to
continue treating filenames as opaque for now instead of insisting on utf-8 as
the spec requires.

--bugs, out of scope for now:

SATT14: change attribute not affected by SETATTR(mode): this is another
symptom of the ext2/3 time resolution problem.

RNM16: RENAME dir1 into existing, nonempty dir2 should return NFS4ERR_EXIST,
instead got NFS4ERR_NOTEMPTY


--I consider these dealt with:

SATT8: Andy thought we should be picky about something here where I thought we
should just allow it; as a result I've been reluctant to either submit or drop
the patch that make us less picky.  I've given up and dropped it.  Python
should stop complaining.

LOCK8c: we're allowing nonzero lockseqid's in the open_to_lockowner struct; see
discussion on this list last week.  This test should probably be removed or at
least not run by default.

COMP3: Python is checking whether tags are utf-8 or not.  Could we just remove
or disable this test?  It is true that the spec requires this, but it's a very
silly requirement since tags are only debugging aids anyway.

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

[Yang Hongyang]

J. Bruce Fields wrote:
> On Thu, Mar 19, 2009 at 05:46:45PM +0800, Yang Hongyang wrote:
>> v1->v2:update some code style problem
>> -------------------------
>>
>> 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.
> 
> I thought Benny found that this also caused the linux client to return a
> better error in one of these cases--could you confirm that and add a
> mention of it in the commit message?

Yes,i'll do that.

> 
> (I'm reluctant to take patches like this based *only* on the spec
> language, partly because rfc 3530 is known to have a few oversights in
> the error listings.)
> 
> I definitely appreciate people going through the pynfs tests and
> investigating the results, but I don't want patch whose only
> justification is that they quiet pynfs--we need to think about the
> likely effect on real clients too.
> 
> --b.
> 
>> 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..9aaecaa 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..28e4688 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..015a655 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:
>> -- 
>> 1.6.0.3
>>
>>
>> -- 
>> Regards
>> Yang Hongyang
> 
> 


-- 
Regards
Yang Hongyang

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

[Yang Hongyang]

J. Bruce Fields wrote:
> On Thu, Mar 19, 2009 at 04:00:35PM -0400, bfields wrote:
>> On Thu, Mar 19, 2009 at 05:46:45PM +0800, Yang Hongyang wrote:
>>> v1->v2:update some code style problem
>>> -------------------------
>>>
>>> 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.
>> I thought Benny found that this also caused the linux client to return a
>> better error in one of these cases--could you confirm that and add a
>> mention of it in the commit message?
>>
>> (I'm reluctant to take patches like this based *only* on the spec
>> language, partly because rfc 3530 is known to have a few oversights in
>> the error listings.)
>>
>> I definitely appreciate people going through the pynfs tests and
>> investigating the results, but I don't want patch whose only
>> justification is that they quiet pynfs--we need to think about the
>> likely effect on real clients too.
> 
> Also, for example:
> 
>>> @@ -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;
>>> +	}
> 
> open_confirm is done with the same filehandle that was returned from a
> previous OPEN.  But an OPEN should never return the filehandle for a
> symlink.  That means for us to reach this case, either the client or our
> filesystem has a very serious bug.  Therefore, I'm not convinced that
> getting the error return correct in this case is worth the trouble.

Agreed,Maybe the err will never be returned.It's even do not worth to
fix it,but people may continuing sending patchs to fix these errors...

Maybe we should think about a better solution.

> 
> --b.
> 
> By the way, I have some old notes on the pynfs tests, below; they may be
> wrong, or out of date, but perhaps they're of some use.

Great,They're of great help to me,Thank you very much.::)

> 
> 
> --fixme:
> 
> CID6: returning EXPIRED on OPEN with unconfirmed client, should return STALE.
> (Why?)
> 
> --bugs, but probably not important:
> 
> OPCF3a, LINK4a, LOOKP2a, SATT12a: These 4 operations don't seem to
> support NFS4ERR_SYMLINK, so we shouldn't be returning it.  This error
> appears to be intentionally just for LOOKUP and OPEN, probably to help
> implement O_NOFOLLOW or something.  It's fh_verify that's producing this
> error.  There are some special cases in nfs4proc.c that map this error
> to einval, but we should probably figure out some better way to deal
> with this than special casing every other fh_verify error return....
> 
> CR14: create of dir with / in the name should return BADNAME or BADCHAR, not
> INVAL.
> 
> CR13, LINK9, RNM11, CR13, LINK9, RNM11: We're returning the wrong error on use
> of '.' as a filename.
> 
> CR12, OPEN15: attempt to set acl on create should return
> NFS4ERR_ATTRNOTSUPP on fs that doesn't support acls, instead it returns
> OK.
> 
> --might care:
> 
> LOOK6, LOOK9, OPEN17, RDDR11, RDDR12: permitting operations on directories of
> mode 0.  This is probably a result of some logic that allows certain requests
> on files and directories whose permissions would otherwise deny it, if the
> requester is the owner of the file or directory.  I'm not sure this is a big
> deal; the client can enforce this requirement, and there's no big security
> hole as long as the owner could chmod the file anyway.
> 
> OPEN17: similar to above, but this on open of a file.  This seems odd.
> 
> 
> --probably don't care:
> 
> COMP6: should return RESOURCE not GARBAGE_ARGS on a compound with 150 ops.
> 
> WRT5: connection reset doing a big write.
> 
> LINK8, OPEN13, RM5, RNM8, RNM9, LOOK7: these result from our decision to
> continue treating filenames as opaque for now instead of insisting on utf-8 as
> the spec requires.
> 
> --bugs, out of scope for now:
> 
> SATT14: change attribute not affected by SETATTR(mode): this is another
> symptom of the ext2/3 time resolution problem.
> 
> RNM16: RENAME dir1 into existing, nonempty dir2 should return NFS4ERR_EXIST,
> instead got NFS4ERR_NOTEMPTY
> 
> 
> --I consider these dealt with:
> 
> SATT8: Andy thought we should be picky about something here where I thought we
> should just allow it; as a result I've been reluctant to either submit or drop
> the patch that make us less picky.  I've given up and dropped it.  Python
> should stop complaining.
> 
> LOCK8c: we're allowing nonzero lockseqid's in the open_to_lockowner struct; see
> discussion on this list last week.  This test should probably be removed or at
> least not run by default.
> 
> COMP3: Python is checking whether tags are utf-8 or not.  Could we just remove
> or disable this test?  It is true that the spec requires this, but it's a very
> silly requirement since tags are only debugging aids anyway.
> 
> 


-- 
Regards
Yang Hongyang

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

[Yang Hongyang]

J. Bruce Fields wrote:
> On Thu, Mar 19, 2009 at 05:46:45PM +0800, Yang Hongyang wrote:
>> v1->v2:update some code style problem
>> -------------------------
>>
>> 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.
> 
> I thought Benny found that this also caused the linux client to return a
> better error in one of these cases--could you confirm that and add a
> mention of it in the commit message?
> 
> (I'm reluctant to take patches like this based *only* on the spec
> language, partly because rfc 3530 is known to have a few oversights in
> the error listings.)
> 
> I definitely appreciate people going through the pynfs tests and
> investigating the results, but I don't want patch whose only
> justification is that they quiet pynfs--we need to think about the
> likely effect on real clients too.
> 
> --b.
> 

Just as Bruce said:
open_confirm is done with the same filehandle that was returned from a
previous OPEN.  But an OPEN should never return the filehandle for a
symlink.  That means for us to reach this case, either the client or our
filesystem has a very serious bug.  Therefore, I'm not convinced that
getting the error return correct in this case is worth the trouble.

OPEN_CONFIRM may never hit the error return.

And i did a test through following commands on a nfs4 fs:
#touch test
#ln -s test 1
#ln 1 2

It just creat a symlink 2 to test as on the local fs.Accroding to
this,I think link op will never hit the nfserr_symlink err return
either.

For the reasons above,There seems to be only one op *LOOKUPP* that
needs to be fixed.But still,I consider we should fix it all even the error
return won't be triggered through real client use cauz there can be
chances that a specially designed programme can triggered the bug.
If the bug is not the return value issue but a memory overflow or some
other strictness,the server may down by such attack.

-------------------------------------------------------------------------------------------
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.
Benny Halevy pointed out  that the linux nfs client translates NFS4ERR_SYMLINK 
to -ELOOP which is awkward and less descriptive to the app/user than
-ENOTDIR. So a careful client implementation should never get NFS4ERR_SYMLINK
if it stats the directory it operates on before sending the link op (or lookup, create,
 rename, etc.) to make sure it is indeed a directory.
[Sigh, looking at the code - it looks like we'll return NFS4ERR_ISDIR for a 
length-changing SETATTR operating on a directory.  This is fine in NFSv4.0 
but this error was removed for SETATTR in nfs4.1.  Note to self: revise
this in the nfs41 tree]

Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
Reviewed-by: Benny Halevy <bhalevy@panasas.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..9aaecaa 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..28e4688 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..015a655 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:
-- 
1.6.0.3

-- 
Regards
Yang Hongyang

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

[Yang Hongyang]

Hi,Bruce:
	what do you think of this patch,is this reasonable?

Yang Hongyang wrote:
> J. Bruce Fields wrote:
>> On Thu, Mar 19, 2009 at 05:46:45PM +0800, Yang Hongyang wrote:
>>> v1->v2:update some code style problem
>>> -------------------------
>>>
>>> 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.
>> I thought Benny found that this also caused the linux client to return a
>> better error in one of these cases--could you confirm that and add a
>> mention of it in the commit message?
>>
>> (I'm reluctant to take patches like this based *only* on the spec
>> language, partly because rfc 3530 is known to have a few oversights in
>> the error listings.)
>>
>> I definitely appreciate people going through the pynfs tests and
>> investigating the results, but I don't want patch whose only
>> justification is that they quiet pynfs--we need to think about the
>> likely effect on real clients too.
>>
>> --b.
>>
> 
> Just as Bruce said:
> open_confirm is done with the same filehandle that was returned from a
> previous OPEN.  But an OPEN should never return the filehandle for a
> symlink.  That means for us to reach this case, either the client or our
> filesystem has a very serious bug.  Therefore, I'm not convinced that
> getting the error return correct in this case is worth the trouble.
> 
> OPEN_CONFIRM may never hit the error return.
> 
> And i did a test through following commands on a nfs4 fs:
> #touch test
> #ln -s test 1
> #ln 1 2
> 
> It just creat a symlink 2 to test as on the local fs.Accroding to
> this,I think link op will never hit the nfserr_symlink err return
> either.
> 
> For the reasons above,There seems to be only one op *LOOKUPP* that
> needs to be fixed.But still,I consider we should fix it all even the error
> return won't be triggered through real client use cauz there can be
> chances that a specially designed programme can triggered the bug.
> If the bug is not the return value issue but a memory overflow or some
> other strictness,the server may down by such attack.
> 
> -------------------------------------------------------------------------------------------
> 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.
> Benny Halevy pointed out  that the linux nfs client translates NFS4ERR_SYMLINK 
> to -ELOOP which is awkward and less descriptive to the app/user than
> -ENOTDIR. So a careful client implementation should never get NFS4ERR_SYMLINK
> if it stats the directory it operates on before sending the link op (or lookup, create,
>  rename, etc.) to make sure it is indeed a directory.
> [Sigh, looking at the code - it looks like we'll return NFS4ERR_ISDIR for a 
> length-changing SETATTR operating on a directory.  This is fine in NFSv4.0 
> but this error was removed for SETATTR in nfs4.1.  Note to self: revise
> this in the nfs41 tree]
> 
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Reviewed-by: Benny Halevy <bhalevy@panasas.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..9aaecaa 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..28e4688 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..015a655 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:


-- 
Regards
Yang Hongyang