[PATCH 0/1] minorversion= is being ignored

[PATCH 0/1] minorversion= is being ignored

[Steve Dickson]

This patch ensure "mount -t" mounts that have the 
minorversion= flag set, the given minor version
is used. 

There has been some talk about deprecating the
minorversion option which it appears to be fairly 
simple in the kernel, but I think would be pretty
difficult in the userland, do to all the per-existing
scripts that used the option. 

To make the deprecation possible in the kernel, 
code was added to convert minorversion= into
vers=4.x in the string that passed to the kernel.  

Steve Dickson (1):
  mount.nfs: minorversion setting is being ignored with -t flag

 utils/mount/network.c | 15 ++++++++++-----
 utils/mount/stropts.c | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

-- 
2.14.3

[PATCH 1/1] mount.nfs: minorversion setting is being ignored with -t flag

[Steve Dickson]

mount -t nfs or mount -t nfs4 and setting minorversion
should set the v4 minor version. This patch adds a few
checks to make sure the minor version is set.

The patch also translate the minorversion= option
to vers=4.x in the arguments passed to the kernel.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/mount/network.c | 15 ++++++++++-----
 utils/mount/stropts.c | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 8ab5be8..8d6e4c6 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1275,8 +1275,8 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
 		}
 	}
 
-	if (!found && strcmp(type, "nfs4") == 0)
-		version_val = type + 3;
+	if (!found && strncmp(type, "nfs", 3) == 0)
+		version_val = "4";
 	else if (!found)
 		return 1;
 	else if (i <= 2 ) {
@@ -1308,9 +1308,14 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
 		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')
-		version->v_mode = V_GENERAL;
-
+	} else if (version->major > 3 && *cptr == '\0') {
+		version_val = po_get(options, "minorversion");
+		if (version_val != NULL) {
+			version->minor = strtol(version_val, &cptr, 10);
+			version->v_mode = V_SPECIFIC;
+		} else 
+			version->v_mode = V_GENERAL;
+	}
 	if (*cptr != '\0')
 		goto ret_error;
 
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 777de39..6bbfd71 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -767,6 +767,20 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
 			mi->version.minor);
 #pragma GCC diagnostic warning "-Wformat-nonliteral"
 
+		if (po_append(options, version_opt) == PO_FAILED) {
+			errno = EINVAL;
+			goto out_fail;
+		}
+	} else if (po_get(options, "minorversion")) {
+		/*
+	 	 * convert minorversion= into vers=4.x
+	 	 */
+		po_remove_all(options, "minorversion");
+
+		snprintf(version_opt, sizeof(version_opt) - 1,
+			"vers=%lu.%lu", mi->version.major,
+			mi->version.minor);
+
 		if (po_append(options, version_opt) == PO_FAILED) {
 			errno = EINVAL;
 			goto out_fail;
-- 
2.14.3

Re: [PATCH 1/1] mount.nfs: minorversion setting is being ignored with -t flag

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]

On Mon, Feb 26 2018, Steve Dickson wrote:

> mount -t nfs or mount -t nfs4 and setting minorversion
> should set the v4 minor version. This patch adds a few
> checks to make sure the minor version is set.
>
> The patch also translate the minorversion= option
> to vers=4.x in the arguments passed to the kernel.
>

If you do this, then you break compatibility with v3.4 and earlier.
(anything without Commit: 0d71b058092f ("NFS: Extend the -overs= mount
option to allow 4.x minorversions")).

Maybe that is OK, but I think that fact should be clearly stated in the
commit log, and preferably the configure script should warn if you try
to build on an unsupported kernel.

Do we have any sort of policy on the oldest kernel the nfs-utils still
supports?  Should we?

Thanks,
NeilBrown


> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  utils/mount/network.c | 15 ++++++++++-----
>  utils/mount/stropts.c | 14 ++++++++++++++
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index 8ab5be8..8d6e4c6 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -1275,8 +1275,8 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
>  		}
>  	}
>  
> -	if (!found && strcmp(type, "nfs4") == 0)
> -		version_val = type + 3;
> +	if (!found && strncmp(type, "nfs", 3) == 0)
> +		version_val = "4";
>  	else if (!found)
>  		return 1;
>  	else if (i <= 2 ) {
> @@ -1308,9 +1308,14 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
>  		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')
> -		version->v_mode = V_GENERAL;
> -
> +	} else if (version->major > 3 && *cptr == '\0') {
> +		version_val = po_get(options, "minorversion");
> +		if (version_val != NULL) {
> +			version->minor = strtol(version_val, &cptr, 10);
> +			version->v_mode = V_SPECIFIC;
> +		} else 
> +			version->v_mode = V_GENERAL;
> +	}
>  	if (*cptr != '\0')
>  		goto ret_error;
>  
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 777de39..6bbfd71 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -767,6 +767,20 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
>  			mi->version.minor);
>  #pragma GCC diagnostic warning "-Wformat-nonliteral"
>  
> +		if (po_append(options, version_opt) == PO_FAILED) {
> +			errno = EINVAL;
> +			goto out_fail;
> +		}
> +	} else if (po_get(options, "minorversion")) {
> +		/*
> +	 	 * convert minorversion= into vers=4.x
> +	 	 */
> +		po_remove_all(options, "minorversion");
> +
> +		snprintf(version_opt, sizeof(version_opt) - 1,
> +			"vers=%lu.%lu", mi->version.major,
> +			mi->version.minor);
> +
>  		if (po_append(options, version_opt) == PO_FAILED) {
>  			errno = EINVAL;
>  			goto out_fail;
> -- 
> 2.14.3
>
> --
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/1] mount.nfs: minorversion setting is being ignored with -t flag

[Steve Dickson]

On 02/26/2018 09:12 PM, NeilBrown wrote:
> On Mon, Feb 26 2018, Steve Dickson wrote:
> 
>> mount -t nfs or mount -t nfs4 and setting minorversion
>> should set the v4 minor version. This patch adds a few
>> checks to make sure the minor version is set.
>>
>> The patch also translate the minorversion= option
>> to vers=4.x in the arguments passed to the kernel.
>>
> 
> If you do this, then you break compatibility with v3.4 and earlier.
> (anything without Commit: 0d71b058092f ("NFS: Extend the -overs= mount
> option to allow 4.x minorversions")).
Yeah... I did realized that... 
> 
> Maybe that is OK, but I think that fact should be clearly stated in the
> commit log, and preferably the configure script should warn if you try
> to build on an unsupported kernel.Well I just took a look and it's easy enough to put
a kernel version check in... We do that all over the mount code.
> 
> Do we have any sort of policy on the oldest kernel the nfs-utils still
> supports?  Should we?
No... and IDK... I think it is more of a compilation issue. 
Getting the code to compile on older systems is not easy.
You have to disable a number features as well as hack up
the configure.ac file. 

Question, do packages like glibc have these types of policies?

steved. 
> 
> Thanks,
> NeilBrown
> 
> 
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>>  utils/mount/network.c | 15 ++++++++++-----
>>  utils/mount/stropts.c | 14 ++++++++++++++
>>  2 files changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index 8ab5be8..8d6e4c6 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -1275,8 +1275,8 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
>>  		}
>>  	}
>>  
>> -	if (!found && strcmp(type, "nfs4") == 0)
>> -		version_val = type + 3;
>> +	if (!found && strncmp(type, "nfs", 3) == 0)
>> +		version_val = "4";
>>  	else if (!found)
>>  		return 1;
>>  	else if (i <= 2 ) {
>> @@ -1308,9 +1308,14 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
>>  		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')
>> -		version->v_mode = V_GENERAL;
>> -
>> +	} else if (version->major > 3 && *cptr == '\0') {
>> +		version_val = po_get(options, "minorversion");
>> +		if (version_val != NULL) {
>> +			version->minor = strtol(version_val, &cptr, 10);
>> +			version->v_mode = V_SPECIFIC;
>> +		} else 
>> +			version->v_mode = V_GENERAL;
>> +	}
>>  	if (*cptr != '\0')
>>  		goto ret_error;
>>  
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 777de39..6bbfd71 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -767,6 +767,20 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
>>  			mi->version.minor);
>>  #pragma GCC diagnostic warning "-Wformat-nonliteral"
>>  
>> +		if (po_append(options, version_opt) == PO_FAILED) {
>> +			errno = EINVAL;
>> +			goto out_fail;
>> +		}
>> +	} else if (po_get(options, "minorversion")) {
>> +		/*
>> +	 	 * convert minorversion= into vers=4.x
>> +	 	 */
>> +		po_remove_all(options, "minorversion");
>> +
>> +		snprintf(version_opt, sizeof(version_opt) - 1,
>> +			"vers=%lu.%lu", mi->version.major,
>> +			mi->version.minor);
>> +
>>  		if (po_append(options, version_opt) == PO_FAILED) {
>>  			errno = EINVAL;
>>  			goto out_fail;
>> -- 
>> 2.14.3
>>
>> --
>> 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: [PATCH 1/1] mount.nfs: minorversion setting is being ignored with -t flag

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4830 bytes --]

On Tue, Feb 27 2018, Steve Dickson wrote:

> On 02/26/2018 09:12 PM, NeilBrown wrote:
>> On Mon, Feb 26 2018, Steve Dickson wrote:
>> 
>>> mount -t nfs or mount -t nfs4 and setting minorversion
>>> should set the v4 minor version. This patch adds a few
>>> checks to make sure the minor version is set.
>>>
>>> The patch also translate the minorversion= option
>>> to vers=4.x in the arguments passed to the kernel.
>>>
>> 
>> If you do this, then you break compatibility with v3.4 and earlier.
>> (anything without Commit: 0d71b058092f ("NFS: Extend the -overs= mount
>> option to allow 4.x minorversions")).
> Yeah... I did realized that... 
>> 
>> Maybe that is OK, but I think that fact should be clearly stated in the
>> commit log, and preferably the configure script should warn if you try
>> to build on an unsupported kernel.Well I just took a look and it's easy enough to put
> a kernel version check in... We do that all over the mount code.
>> 
>> Do we have any sort of policy on the oldest kernel the nfs-utils still
>> supports?  Should we?
> No... and IDK... I think it is more of a compilation issue. 
> Getting the code to compile on older systems is not easy.
> You have to disable a number features as well as hack up
> the configure.ac file. 
>
> Question, do packages like glibc have these types of policies?

glibc documents in README the minimum kernel version.
In Apr 2014 it was raised to 2.6.32.
In Map 2017 it was raised to 3.2
Though some archs have different rules
  git grep arch_minimum_kernel sysdeps
in the glibc source.

systemd also sets a minimum, currently:
REQUIREMENTS:
        Linux kernel >= 3.13
        Linux kernel >= 4.2 for unified cgroup hierarchy support

according to README.

So if we want to require 3.5 or later, then I have not problem with
that.
I just think it should be clearly announced in the git commit, and
documented in a README or similar.

Thanks,
NeilBrown


>
> steved. 
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>>  utils/mount/network.c | 15 ++++++++++-----
>>>  utils/mount/stropts.c | 14 ++++++++++++++
>>>  2 files changed, 24 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>> index 8ab5be8..8d6e4c6 100644
>>> --- a/utils/mount/network.c
>>> +++ b/utils/mount/network.c
>>> @@ -1275,8 +1275,8 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
>>>  		}
>>>  	}
>>>  
>>> -	if (!found && strcmp(type, "nfs4") == 0)
>>> -		version_val = type + 3;
>>> +	if (!found && strncmp(type, "nfs", 3) == 0)
>>> +		version_val = "4";
>>>  	else if (!found)
>>>  		return 1;
>>>  	else if (i <= 2 ) {
>>> @@ -1308,9 +1308,14 @@ nfs_nfs_version(char *type, struct mount_options *options, struct nfs_version *v
>>>  		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')
>>> -		version->v_mode = V_GENERAL;
>>> -
>>> +	} else if (version->major > 3 && *cptr == '\0') {
>>> +		version_val = po_get(options, "minorversion");
>>> +		if (version_val != NULL) {
>>> +			version->minor = strtol(version_val, &cptr, 10);
>>> +			version->v_mode = V_SPECIFIC;
>>> +		} else 
>>> +			version->v_mode = V_GENERAL;
>>> +	}
>>>  	if (*cptr != '\0')
>>>  		goto ret_error;
>>>  
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 777de39..6bbfd71 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -767,6 +767,20 @@ static int nfs_do_mount_v4(struct nfsmount_info *mi,
>>>  			mi->version.minor);
>>>  #pragma GCC diagnostic warning "-Wformat-nonliteral"
>>>  
>>> +		if (po_append(options, version_opt) == PO_FAILED) {
>>> +			errno = EINVAL;
>>> +			goto out_fail;
>>> +		}
>>> +	} else if (po_get(options, "minorversion")) {
>>> +		/*
>>> +	 	 * convert minorversion= into vers=4.x
>>> +	 	 */
>>> +		po_remove_all(options, "minorversion");
>>> +
>>> +		snprintf(version_opt, sizeof(version_opt) - 1,
>>> +			"vers=%lu.%lu", mi->version.major,
>>> +			mi->version.minor);
>>> +
>>>  		if (po_append(options, version_opt) == PO_FAILED) {
>>>  			errno = EINVAL;
>>>  			goto out_fail;
>>> -- 
>>> 2.14.3
>>>
>>> --
>>> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/1] mount.nfs: minorversion setting is being ignored with -t flag

[Steve Dickson]

On 02/27/2018 03:49 PM, NeilBrown wrote:
> On Tue, Feb 27 2018, Steve Dickson wrote:
>> Question, do packages like glibc have these types of policies?
> 
> glibc documents in README the minimum kernel version.
> In Apr 2014 it was raised to 2.6.32.
> In Map 2017 it was raised to 3.2
> Though some archs have different rules
>   git grep arch_minimum_kernel sysdeps
> in the glibc source.
> 
> systemd also sets a minimum, currently:
> REQUIREMENTS:
>         Linux kernel >= 3.13
>         Linux kernel >= 4.2 for unified cgroup hierarchy support
> 
> according to README.
> 
> So if we want to require 3.5 or later, then I have not problem with
> that.
> I just think it should be clearly announced in the git commit, and
> documented in a README or similar.
Thanks for pointing this out... 

I'm thinking we should probably create this kernel requirement
once the option is deprecated... 

Since the precedent has already been set to check the kernel version in 
the mounting code, I'm going to use that to fix the bug, so I can back 
port the fix to other releases... 

Again, thanks for the info and the suggestions! 

steved.