Don't try NFSv4 if any MNT protocol related options were presented by
the user.
Signed-off-by: Chuck Lever <[email protected]>
---
utils/mount/stropts.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0685caa..3401f63 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
}
if (mi->version == 0) {
+ if (po_contains(options, "mounthost") ||
+ po_contains(options, "mountaddr") ||
+ po_contains(options, "mountvers") ||
+ po_contains(options, "mountproto")) {
+ errno = EPROTONOSUPPORT;
+ goto out_fail;
+ }
if (po_append(options, "vers=4") == PO_FAILED) {
errno = EINVAL;
goto out_fail;
On Thu, 2009-10-08 at 13:37 -0400, Chuck Lever wrote:
> Don't try NFSv4 if any MNT protocol related options were presented by
> the user.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> utils/mount/stropts.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 0685caa..3401f63 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info *mi)
> }
>
> if (mi->version == 0) {
> + if (po_contains(options, "mounthost") ||
> + po_contains(options, "mountaddr") ||
> + po_contains(options, "mountvers") ||
> + po_contains(options, "mountproto")) {
> + errno = EPROTONOSUPPORT;
Shouldn't this be EINVAL ?
> + goto out_fail;
> + }
> if (po_append(options, "vers=4") == PO_FAILED) {
> errno = EINVAL;
> goto out_fail;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 8, 2009, at 1:45 PM, Trond Myklebust wrote:
> On Thu, 2009-10-08 at 13:37 -0400, Chuck Lever wrote:
>> Don't try NFSv4 if any MNT protocol related options were presented by
>> the user.
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> utils/mount/stropts.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 0685caa..3401f63 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct
>> nfsmount_info *mi)
>> }
>>
>> if (mi->version == 0) {
>> + if (po_contains(options, "mounthost") ||
>> + po_contains(options, "mountaddr") ||
>> + po_contains(options, "mountvers") ||
>> + po_contains(options, "mountproto")) {
>> + errno = EPROTONOSUPPORT;
>
> Shouldn't this be EINVAL ?
Since this is behind the mi->version == 0 check, we know that the user
didn't specify an NFS version. In any other v4 case, the kernel's
mount option parser will kick these out with EINVAL. But here, we
just want to avoid trying to negotiate NFSv4. So the EPROTONOSUPPORT
return code will cause the logic in nfs_try_mount() to try v3/v2 next.
Basically the bug is this: before, if I didn't specify an NFS
version, but added some mountfoo= option, the mount.nfs command will
try negotiating NFSv2/v3. Now, it will try NFSv4 first, but these are
illegal options for NFSv4, and the mount command will fail. So,
regression. This new logic simply skips trying NFSv4 in this case.
>
>> + goto out_fail;
>> + }
>> if (po_append(options, "vers=4") == PO_FAILED) {
>> errno = EINVAL;
>> goto out_fail;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-
>> nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Oct 9, 2009, at 9:29 AM, Steve Dickson wrote:
>
>
> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>> Don't try NFSv4 if any MNT protocol related options were presented by
>> the user.
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> utils/mount/stropts.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 0685caa..3401f63 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct
>> nfsmount_info *mi)
>> }
>>
>> if (mi->version == 0) {
>> + if (po_contains(options, "mounthost") ||
>> + po_contains(options, "mountaddr") ||
>> + po_contains(options, "mountvers") ||
>> + po_contains(options, "mountproto")) {
>> + errno = EPROTONOSUPPORT;
>> + goto out_fail;
>> + }
> I think it make senses to assume the version negation should
> start version 3 when mountXXXX options exist instead of
> failing a mount they probably didn't want..
Yes, that's exactly what this patch does. NFSv4 negotiation is
skipped if any mountfoo options are presented by the user.
It's arguable where to put this check. This seemed like the most
straightforward way to deal with it.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Oct 9, 2009, at 11:12 AM, Steve Dickson wrote:
>
>
> On 10/09/2009 11:03 AM, Chuck Lever wrote:
>>
>> On Oct 9, 2009, at 9:29 AM, Steve Dickson wrote:
>>
>>>
>>>
>>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>>> Don't try NFSv4 if any MNT protocol related options were
>>>> presented by
>>>> the user.
>>>>
>>>> Signed-off-by: Chuck Lever <[email protected]>
>>>> ---
>>>>
>>>> utils/mount/stropts.c | 7 +++++++
>>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>>> index 0685caa..3401f63 100644
>>>> --- a/utils/mount/stropts.c
>>>> +++ b/utils/mount/stropts.c
>>>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct
>>>> nfsmount_info
>>>> *mi)
>>>> }
>>>>
>>>> if (mi->version == 0) {
>>>> + if (po_contains(options, "mounthost") ||
>>>> + po_contains(options, "mountaddr") ||
>>>> + po_contains(options, "mountvers") ||
>>>> + po_contains(options, "mountproto")) {
>>>> + errno = EPROTONOSUPPORT;
>>>> + goto out_fail;
>>>> + }
>>> I think it make senses to assume the version negation should
>>> start version 3 when mountXXXX options exist instead of
>>> failing a mount they probably didn't want..
>>
>> Yes, that's exactly what this patch does. NFSv4 negotiation is
>> skipped
>> if any mountfoo options are presented by the user.
>>
>> It's arguable where to put this check. This seemed like the most
>> straightforward way to deal with it.
> I guess a comment would have made it a bit clear...
Yes, it does need a comment. I thought that placing this inside the
mi->version == 0 check would have been clear enough, but I guess
not. :-)
> and I was thinking
> the check should be made before the nfs_try_mount_v4() verses having
> nfs_try_mount_v4() fail in a recoverable way...
I'll cobble up a version that does this check in nfs_try_mount()
instead, just to see what it looks like.
Alternately we can do this before starting the retry loop, but setting
mi->version = 3 would be less clear, I think, then skipping the v4
negotiation in the loop.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On 10/09/2009 11:03 AM, Chuck Lever wrote:
>
> On Oct 9, 2009, at 9:29 AM, Steve Dickson wrote:
>
>>
>>
>> On 10/08/2009 01:37 PM, Chuck Lever wrote:
>>> Don't try NFSv4 if any MNT protocol related options were presented by
>>> the user.
>>>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>>
>>> utils/mount/stropts.c | 7 +++++++
>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 0685caa..3401f63 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -564,6 +564,13 @@ static int nfs_try_mount_v4(struct nfsmount_info
>>> *mi)
>>> }
>>>
>>> if (mi->version == 0) {
>>> + if (po_contains(options, "mounthost") ||
>>> + po_contains(options, "mountaddr") ||
>>> + po_contains(options, "mountvers") ||
>>> + po_contains(options, "mountproto")) {
>>> + errno = EPROTONOSUPPORT;
>>> + goto out_fail;
>>> + }
>> I think it make senses to assume the version negation should
>> start version 3 when mountXXXX options exist instead of
>> failing a mount they probably didn't want..
>
> Yes, that's exactly what this patch does. NFSv4 negotiation is skipped
> if any mountfoo options are presented by the user.
>
> It's arguable where to put this check. This seemed like the most
> straightforward way to deal with it.
I guess a comment would have made it a bit clear... and I was thinking
the check should be made before the nfs_try_mount_v4() verses having
nfs_try_mount_v4() fail in a recoverable way...
steved.