Re: [PATCH 2/2 V2] mount.nfs: Use default minor version when -o v4 is specified

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/12/2017 09:19 PM, NeilBrown wrote:
> On Fri, Jun 09 2017, Steve Dickson wrote:
> 
>> When v4 is specified on the command line the
>> default minor version needs to be used.
>>
>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
>> ---
>>  utils/mount/stropts.c | 27 ++++++++++++++++++++-------
>>  1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 81fb945..d2d303f 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -119,14 +119,22 @@ static void nfs_default_version(struct nfsmount_info *mi)
>>  	if (mi->version.v_mode == V_DEFAULT &&
>>  		config_default_vers.v_mode != V_DEFAULT) {
>>  		mi->version.major = config_default_vers.major;
>> -		mi->version.minor = config_default_vers.minor;
>> +		if (config_default_vers.minor)
>> +			mi->version.minor = config_default_vers.minor;
>> +		else if (!mi->version.minor)
>> +			mi->version.minor = NFS_DEFAULT_MINOR;
> 
> No, this looks wrong.  A minor number of '0' can be perfectly valid.
> Deciding to turn a minor of '0' into '2' is simply wrong.
The only way to have a minor version of '0' is to use 
the -o v4.0 flag. That would causes v_mode to be V_SPECIFIC.
Also note, in nfs_nfs_version() when major is < 4 
the v_mode is also set to V_SPECIFIC (aka -o v3)
For both those case nfs_default_version() is not called. 

So the only way we can get into this code is if v_mode 
is V_GENERAL or V_DEFAULT. So it is a v4 mount and the
minor version needs to be set. So a minor == 0 means
the minor version has not be set so set it to the
default minor version. 

So I think we are good with this...

steved.

> 
> You can only tell if .minor is valid by looking at .v_mode.
> If .v_mode is V_GENERAL or V_DEFAULT, then .minor is not valid
> and a default should be used.  If it is V_SPECIFIC, then .minor is
> either valid or irrelevant, depending on .major.
> 
> 
>>  		return;
>>  	}
>>  
>>  	if (mi->version.v_mode == V_GENERAL) {
>>  		if (config_default_vers.v_mode != V_DEFAULT &&
>> -		    mi->version.major == config_default_vers.major)
>> -			mi->version.minor = config_default_vers.minor;
>> +		    mi->version.major == config_default_vers.major) {
>> +			if (mi->version.minor)
>> +				mi->version.minor = config_default_vers.minor;
> 
> Again, don't test version.minor like this.
> 
>> +			else if (!mi->version.minor)
>> +				mi->version.minor = NFS_DEFAULT_MINOR;
>> +		} else if (!mi->version.minor)
>> +			mi->version.minor = NFS_DEFAULT_MINOR;
>>  		return;
>>  	}
>>  
>> @@ -740,10 +748,15 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
>>  	}
>>  
>>  	if (mi->version.v_mode != V_SPECIFIC) {
>> -		if (mi->version.v_mode == V_GENERAL)
>> -			snprintf(version_opt, sizeof(version_opt) - 1,
>> -				"vers=%lu", mi->version.major);
>> -		else
>> +		if (mi->version.v_mode == V_GENERAL) {
>> +			if (mi->version.major > 3)
> 
> This test is pointless as .v_mode is only ever V_GENERAL when
>  .major == 4
> 
> And given that this is nfs_do_mount_v4(), we can be certain that
> version.major == 4.
> 
> Prior to
> Commit: 0d71b058092f ("NFS: Extend the -overs= mount option to allow 4.x minorversions")
> 
> in Linux 3.4, writing "vers=4.1" wasn't supported.
> So I think it would be safest to use
>   vers=4                   if minor == 0
>   vers=4,minorversion=1    if minor == 1
>   vers=4.2                 if minor == 2
> 
> This is despite the fact that the comment in the kernel says
>                                                   In future,
>                  * the mount program should always supply
>                  * a NFSv4 minor version number.
> 
> I think vers=4.%d is appropriate for v4.2 and later, but not for earlier
> versions.
> 
> Alternately we could test the kernel version and behave differently, but
> I think that is worse.
> 
> Thanks,
> NeilBrown
> 
>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>> +					"vers=%lu.%lu", mi->version.major,
>> +					mi->version.minor);
>> +			else 
>> +				snprintf(version_opt, sizeof(version_opt) - 1,
>> +					"vers=%lu", mi->version.major);
>> +		} else
>>  			snprintf(version_opt, sizeof(version_opt) - 1,
>>  				"vers=%lu.%lu", mi->version.major,
>>  				mi->version.minor);
>> -- 
>> 2.9.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux