On Tue, 31 Aug 2010 17:21:31 -0400
Chuck Lever <[email protected]> wrote:
> The logic that manages negotiating NFS version and protocol settings
> is getting more complex over time, so let's split this out of
> nfs_try_mount().
>
> This should make Bruce a little happier, as it eliminates the silent
> switch case fall through in nfs_try_mount(). And it should help Neil
> fix some bugs he's found in this logic.
Hi Chuck,
thanks for these..
One question....
.....
> +{
> + int result;
> +
> + /*
> + * Before 2.6.32, the kernel NFS client didn't support
> + * "-t nfs vers=4" mounts, so NFS version 4 cannot be
> + * included when autonegotiating while running on
> + * those kernels.
> + */
> + if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
> + goto fall_back;
> +
> + errno = 0;
> + result = nfs_try_mount_v4(mi);
> + switch (errno) {
Should we not have e.g.
if (result)
return result;
before the switch(errno)?? Typically errno is 'undefined' in no error is
reported.
I realise the current code doesn't have that test either, but I still think
it is wrong not to.
Thanks
NeilBrown
> + case EPROTONOSUPPORT:
> + /* A clear indication that the server or our
> + * client does not support NFS version 4. */
> + goto fall_back;
> + case ENOENT:
> + /* Legacy Linux servers don't export an NFS
> + * version 4 pseudoroot. */
> + goto fall_back;
> + case EPERM:
> + /* Linux servers prior to 2.6.25 may return
> + * EPERM when NFS version 4 is not supported. */
> + goto fall_back;
> + default:
> + return result;
> + }
> +
> +fall_back:
> + return nfs_try_mount_v3v2(mi);
> +}
> +
> +/*
> * This is a single pass through the fg/bg loop.
> *
> * Returns TRUE if successful, otherwise FALSE.
> @@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>
> switch (mi->version) {
> case 0:
> - if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
> - errno = 0;
> - result = nfs_try_mount_v4(mi);
> - if (errno != EPROTONOSUPPORT) {
> - /*
> - * To deal with legacy Linux servers that don't
> - * automatically export a pseudo root, retry
> - * ENOENT errors using version 3. And for
> - * Linux servers prior to 2.6.25, retry EPERM
> - */
> - if (errno != ENOENT && errno != EPERM)
> - break;
> - }
> - }
> + result = nfs_autonegotiate(mi);
> + break;
> case 2:
> case 3:
> result = nfs_try_mount_v3v2(mi);
On Sep 6, 2010, at 9:12 PM, Neil Brown wrote:
> On Tue, 31 Aug 2010 17:21:31 -0400
> Chuck Lever <[email protected]> wrote:
>
>> The logic that manages negotiating NFS version and protocol settings
>> is getting more complex over time, so let's split this out of
>> nfs_try_mount().
>>
>> This should make Bruce a little happier, as it eliminates the silent
>> switch case fall through in nfs_try_mount(). And it should help Neil
>> fix some bugs he's found in this logic.
>
> Hi Chuck,
> thanks for these..
> One question....
>
> .....
>> +{
>> + int result;
>> +
>> + /*
>> + * Before 2.6.32, the kernel NFS client didn't support
>> + * "-t nfs vers=4" mounts, so NFS version 4 cannot be
>> + * included when autonegotiating while running on
>> + * those kernels.
>> + */
>> + if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
>> + goto fall_back;
>> +
>> + errno = 0;
>> + result = nfs_try_mount_v4(mi);
>> + switch (errno) {
>
> Should we not have e.g.
>
> if (result)
> return result;
>
> before the switch(errno)?? Typically errno is 'undefined' in no error is
> reported.
>
> I realise the current code doesn't have that test either, but I still think
> it is wrong not to.
We have
errno = 0;
just before the nfs_try_mount_v4(mi); call. This defines the value of errno even if nothing in nfs_try_mount_v4() sets it.
Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?
> Thanks
> NeilBrown
>
>
>
>> + case EPROTONOSUPPORT:
>> + /* A clear indication that the server or our
>> + * client does not support NFS version 4. */
>> + goto fall_back;
>> + case ENOENT:
>> + /* Legacy Linux servers don't export an NFS
>> + * version 4 pseudoroot. */
>> + goto fall_back;
>> + case EPERM:
>> + /* Linux servers prior to 2.6.25 may return
>> + * EPERM when NFS version 4 is not supported. */
>> + goto fall_back;
>> + default:
>> + return result;
>> + }
>> +
>> +fall_back:
>> + return nfs_try_mount_v3v2(mi);
>> +}
>> +
>> +/*
>> * This is a single pass through the fg/bg loop.
>> *
>> * Returns TRUE if successful, otherwise FALSE.
>> @@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>>
>> switch (mi->version) {
>> case 0:
>> - if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
>> - errno = 0;
>> - result = nfs_try_mount_v4(mi);
>> - if (errno != EPROTONOSUPPORT) {
>> - /*
>> - * To deal with legacy Linux servers that don't
>> - * automatically export a pseudo root, retry
>> - * ENOENT errors using version 3. And for
>> - * Linux servers prior to 2.6.25, retry EPERM
>> - */
>> - if (errno != ENOENT && errno != EPERM)
>> - break;
>> - }
>> - }
>> + result = nfs_autonegotiate(mi);
>> + break;
>> case 2:
>> case 3:
>> result = nfs_try_mount_v3v2(mi);
>
--
chuck[dot]lever[at]oracle[dot]com
On Thu, 9 Sep 2010 13:27:38 -0400
Chuck Lever <[email protected]> wrote:
>
> On Sep 6, 2010, at 9:12 PM, Neil Brown wrote:
>
> > On Tue, 31 Aug 2010 17:21:31 -0400
> > Chuck Lever <[email protected]> wrote:
> >
> >> The logic that manages negotiating NFS version and protocol settings
> >> is getting more complex over time, so let's split this out of
> >> nfs_try_mount().
> >>
> >> This should make Bruce a little happier, as it eliminates the silent
> >> switch case fall through in nfs_try_mount(). And it should help Neil
> >> fix some bugs he's found in this logic.
> >
> > Hi Chuck,
> > thanks for these..
> > One question....
> >
> > .....
> >> +{
> >> + int result;
> >> +
> >> + /*
> >> + * Before 2.6.32, the kernel NFS client didn't support
> >> + * "-t nfs vers=4" mounts, so NFS version 4 cannot be
> >> + * included when autonegotiating while running on
> >> + * those kernels.
> >> + */
> >> + if (linux_version_code() <= MAKE_VERSION(2, 6, 31))
> >> + goto fall_back;
> >> +
> >> + errno = 0;
> >> + result = nfs_try_mount_v4(mi);
> >> + switch (errno) {
> >
> > Should we not have e.g.
> >
> > if (result)
> > return result;
> >
> > before the switch(errno)?? Typically errno is 'undefined' in no error is
> > reported.
> >
> > I realise the current code doesn't have that test either, but I still think
> > it is wrong not to.
>
> We have
>
> errno = 0;
>
> just before the nfs_try_mount_v4(mi); call. This defines the value of errno even if nothing in nfs_try_mount_v4() sets it.
True, but what if something in nfs_try_mount_v4 does set it, but success is
finally returned? That is certainly possible if mi has multiple addresses
and the first is unreachable.
I think you must *never* test errno unless the most recent call reported an
error.
>
> Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?
He's like a guardian angel sitting on you shoulder whispering "too subtle"
all the time :-) Definitely true this time.
Thanks,
NeilBrown
>
> > Thanks
> > NeilBrown
> >
> >
> >
> >> + case EPROTONOSUPPORT:
> >> + /* A clear indication that the server or our
> >> + * client does not support NFS version 4. */
> >> + goto fall_back;
> >> + case ENOENT:
> >> + /* Legacy Linux servers don't export an NFS
> >> + * version 4 pseudoroot. */
> >> + goto fall_back;
> >> + case EPERM:
> >> + /* Linux servers prior to 2.6.25 may return
> >> + * EPERM when NFS version 4 is not supported. */
> >> + goto fall_back;
> >> + default:
> >> + return result;
> >> + }
> >> +
> >> +fall_back:
> >> + return nfs_try_mount_v3v2(mi);
> >> +}
> >> +
> >> +/*
> >> * This is a single pass through the fg/bg loop.
> >> *
> >> * Returns TRUE if successful, otherwise FALSE.
> >> @@ -758,20 +801,8 @@ static int nfs_try_mount(struct nfsmount_info *mi)
> >>
> >> switch (mi->version) {
> >> case 0:
> >> - if (linux_version_code() > MAKE_VERSION(2, 6, 31)) {
> >> - errno = 0;
> >> - result = nfs_try_mount_v4(mi);
> >> - if (errno != EPROTONOSUPPORT) {
> >> - /*
> >> - * To deal with legacy Linux servers that don't
> >> - * automatically export a pseudo root, retry
> >> - * ENOENT errors using version 3. And for
> >> - * Linux servers prior to 2.6.25, retry EPERM
> >> - */
> >> - if (errno != ENOENT && errno != EPERM)
> >> - break;
> >> - }
> >> - }
> >> + result = nfs_autonegotiate(mi);
> >> + break;
> >> case 2:
> >> case 3:
> >> result = nfs_try_mount_v3v2(mi);
> >
>
On Fri, 2010-09-10 at 07:23 +1000, Neil Brown wrote:
> On Thu, 9 Sep 2010 13:27:38 -0400
> Chuck Lever <[email protected]> wrote:
> >
> > Perhaps that violates Bruce's "too subtle" rule and I should replace it with your suggested post-call check?
>
> He's like a guardian angel sitting on you shoulder whispering "too subtle"
> all the time :-) Definitely true this time.
The mind boggles at the thought of Bruce perching on anyone's
shoulder...
Cheers
Trond