| Message ID | 877f18jsx5.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sorry for the delayed response... that damn flux capacitor broke... again! ;-) On 05/22/2017 08:52 PM, NeilBrown wrote: > On Mon, May 22 2017, Steve Dickson wrote: > >> On 05/21/2017 11:03 PM, NeilBrown wrote: >>> On Fri, May 19 2017, Steve Dickson wrote: >>> >>>> When the pseudo root is set with fsid=0, explicit >>>> v4 mounts (via the -o flag) should fail when >>>> the incorrect export is tried instead of rolling >>>> back to v3. >>> >>> Hi Steve, >>> I think this patch makes sense, but the above description doesn't. >>> Where does fsid=0 fit in anywhere here? >> It sets the export to be the pseudo root >> /home *(rw,fsid=0,sec=sys:krb5:krb5i:krb5p) >> >> so when then that export using either -t nfs4 or -o v4 >> mount -o v4.0 127.0.0.1:/home /mnt >> >> the mount should fail instead of rolling back to v3 >> Basically its be used to cause the error. >> >>> >>> I think you want to say >>> >>> When the protocol is set with "-t nfs4", we should behave just like >>> with do with "-o vers=4" and not fall back to v3. >> Actually the first patch fixes the -o vers=4 case since >> that too was rolling back to v3 in the above scenario >> >>> >>> Is that what you were really trying to say? >> How about >> >> When the protocol is set the "-o v4" flag, >> and the mount fails due to a pseudo root >> issue, the mount should fail not, roll >> back to v3. > > Better, but I don't think the pseudo root has any relevance. > If you ask for v4, you should get v4, not v3. How the server may or may > not behave differently between v3 and v4 is irrelevant. You should get > what you asked for. Fine. > > But now that I look at the code again... I don't understand. > > You say this is for the "-o v4" case. > > In that case, the current code in nfs_nfs_version() will find the "v4" > entry in nfs_version_opttbl[] and set > version_val = "4"; > version->v_mode = V_SPECIFIC; > then > version_major = 4; > then as *cptr == '\0' and ->major > 4, > version->v_mode = V_GENERAL; > > Your change skips that last step so it finished with > v_mod == V_SPECIFIC. Right.. If v_mode has already set don't reset it. > > According to the extra comments you have added for the modes: > >>>> + V_GENERAL, /* single digit => 4 */ >>>> + V_SPECIFIC, /* single digit < 4 or decimal included */ > > And it seems to me that "v4" should be V_GENERAL, not V_SPECIFIC. > So I think the current code is correct. Personally I don't see a needed for V_GENERAL v_mode type I guess it has to do with the specifying minor version or not but if any thing is specified on the command line or nfsmount.conf then it is V_SPECIFIC... IMHO. > > Except... nfs_try_mount() will then call nfs_autonegotiate(), > and nfs_autonegotiate() isn't very consistent about how it > interprets V_GENERAL and V_SPECIFIC. > For EINVAL, it gets the difference right, for other errors it doesn't. > > So I think that this is the fix you want: > > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > index 0fbb37569ef9..98cf813fe439 100644 > --- a/utils/mount/stropts.c > +++ b/utils/mount/stropts.c > @@ -838,9 +838,6 @@ check_result: > case EINVAL: > /* A less clear indication that our client > * does not support NFSv4 minor version. */ > - if (mi->version.v_mode == V_GENERAL && > - mi->version.minor == 0) > - return result; > if (mi->version.v_mode != V_SPECIFIC) { > if (mi->version.minor > 0) { > mi->version.minor--; > @@ -862,6 +859,9 @@ check_result: > /* UDP-Only servers won't support v4, but maybe it > * just isn't ready yet. So try v3, but double-check > * with rpcbind for v4. */ > + if (mi->version.v_mode == V_GENERAL) > + /* Mustn't try v2,v3 */ > + return result; > result = nfs_try_mount_v3v2(mi, TRUE); > if (result == 0 && errno == EAGAIN) { > /* v4 server seems to be registered now. */ > @@ -878,6 +878,9 @@ check_result: > } > > fall_back: > + if (mi->version.v_mode == V_GENERAL) > + /* v2,3 fallback not allowed */ > + return result; > return nfs_try_mount_v3v2(mi, FALSE); > } > > > > I haven't even compile-tested of course :-) I have and it does compile and work... Would you mind reposting the patch in the proper format? You can added Tested-by: Steve Dickson <steved@redhat.com> Note: The second patch should probably use V_GENERAL as well. tia, steved. > > Thanks, > NeilBrown > > >> >> steved. >> >>> >>> Thanks, >>> NeilBrown >>> >>> >>>> >>>> Signed-off-by: Steve Dickson <steved@redhat.com> >>>> --- >>>> utils/mount/network.c | 3 ++- >>>> utils/mount/network.h | 8 ++++---- >>>> 2 files changed, 6 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>>> index 281e935..e39263e 100644 >>>> --- a/utils/mount/network.c >>>> +++ b/utils/mount/network.c >>>> @@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version) >>>> if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val) >>>> goto ret_error; >>>> version->v_mode = V_SPECIFIC; >>>> - } else if (version->major > 3 && *cptr == '\0') >>>> + } else if (version->major > 3 && *cptr == '\0' && >>>> + version->v_mode == V_DEFAULT) /* v_mode has not been set */ >>>> version->v_mode = V_GENERAL; >>>> >>>> if (*cptr != '\0') >>>> diff --git a/utils/mount/network.h b/utils/mount/network.h >>>> index 9cc5dec..45e2b24 100644 >>>> --- a/utils/mount/network.h >>>> +++ b/utils/mount/network.h >>>> @@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long, >>>> struct mount_options; >>>> >>>> enum { >>>> - V_DEFAULT = 0, >>>> - V_GENERAL, >>>> - V_SPECIFIC, >>>> - V_PARSE_ERR, >>>> + V_DEFAULT = 0, /* not set */ >>>> + V_GENERAL, /* single digit => 4 */ >>>> + V_SPECIFIC, /* single digit < 4 or decimal included */ >>>> + V_PARSE_ERR, /* miss all others */ >>>> }; >>>> >>>> struct nfs_version { >>>> -- >>>> 2.9.4 >>>> >>>> -- >>>> 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 -- 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
On Wed, May 31 2017, Steve Dickson wrote: > Sorry for the delayed response... that damn > flux capacitor broke... again! ;-) That's what you get for buying it on e-bay?? >> >> According to the extra comments you have added for the modes: >> >>>>> + V_GENERAL, /* single digit => 4 */ >>>>> + V_SPECIFIC, /* single digit < 4 or decimal included */ >> >> And it seems to me that "v4" should be V_GENERAL, not V_SPECIFIC. >> So I think the current code is correct. > Personally I don't see a needed for V_GENERAL v_mode type > I guess it has to do with the specifying minor version or not > but if any thing is specified on the command line or > nfsmount.conf then it is V_SPECIFIC... IMHO. Maybe V_GENERAL should be V_MAJOR or V_SPECIFIC_MAJOR. The v4 code assumes that if V_SPECIFIC is set, then the version option provided to the mount command can be passed unchanged to the kernel. So it sometimes means V_NO_NEGOTIATE. >> I haven't even compile-tested of course :-) > I have and it does compile and work... Would you > mind reposting the patch in the proper format? > You can added Tested-by: Steve Dickson <steved@redhat.com> Done. > > Note: The second patch should probably use V_GENERAL as well. Yes, definitely. Thanks, NeilBrown
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c index 0fbb37569ef9..98cf813fe439 100644 --- a/utils/mount/stropts.c +++ b/utils/mount/stropts.c @@ -838,9 +838,6 @@ check_result: case EINVAL: /* A less clear indication that our client * does not support NFSv4 minor version. */ - if (mi->version.v_mode == V_GENERAL && - mi->version.minor == 0) - return result; if (mi->version.v_mode != V_SPECIFIC) { if (mi->version.minor > 0) { mi->version.minor--; @@ -862,6 +859,9 @@ check_result: /* UDP-Only servers won't support v4, but maybe it * just isn't ready yet. So try v3, but double-check * with rpcbind for v4. */ + if (mi->version.v_mode == V_GENERAL) + /* Mustn't try v2,v3 */ + return result; result = nfs_try_mount_v3v2(mi, TRUE); if (result == 0 && errno == EAGAIN) { /* v4 server seems to be registered now. */ @@ -878,6 +878,9 @@ check_result: } fall_back: + if (mi->version.v_mode == V_GENERAL) + /* v2,3 fallback not allowed */ + return result; return nfs_try_mount_v3v2(mi, FALSE); }