Re: [nfs-utils PATCH] nfsdctl: tweak the version subcommand behavior

[Steve Dickson <steved@redhat.com>]

On 1/15/25 7:56 AM, Scott Mayhew wrote:
> On Wed, 15 Jan 2025, Steve Dickson wrote:
> 
>>
>>
>> On 1/8/25 5:54 PM, Scott Mayhew wrote:
>>> The section for the 'nfsdctl version' subcommand on the man page states
>>> that the minorversion is optional, and if omitted it will cause all
>>> minorversions to be enabled/disabled, but it currently doesn't work that
>>> way.
>>>
>>> Make it work that way, with one exception.  If v4.0 is disabled, then
>>> 'nfsdctl version +4' will not re-enable it; instead it must be
>>> explicitly re-enabled via 'nfsdctl version +4.0'.  This mirrors the way
>>> /proc/fs/nfsd/versions works.
>>>
>>> Link: https://issues.redhat.com/browse/RHEL-72477
>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>> ---
>>>    utils/nfsdctl/nfsdctl.8    |  9 ++++--
>>>    utils/nfsdctl/nfsdctl.adoc |  5 +++-
>>>    utils/nfsdctl/nfsdctl.c    | 58 +++++++++++++++++++++++++++++++++++---
>>>    3 files changed, 64 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
>>> index b08fe803..835d60b4 100644
>>> --- a/utils/nfsdctl/nfsdctl.8
>>> +++ b/utils/nfsdctl/nfsdctl.8
>>> @@ -2,12 +2,12 @@
>>>    .\"     Title: nfsdctl
>>>    .\"    Author: Jeff Layton
>>>    .\" Generator: Asciidoctor 2.0.20
>>> -.\"      Date: 2024-12-30
>>> +.\"      Date: 2025-01-08
>>>    .\"    Manual: \ \&
>>>    .\"    Source: \ \&
>>>    .\"  Language: English
>>>    .\"
>>> -.TH "NFSDCTL" "8" "2024-12-30" "\ \&" "\ \&"
>>> +.TH "NFSDCTL" "8" "2025-01-08" "\ \&" "\ \&"
>>>    .ie \n(.g .ds Aq \(aq
>>>    .el       .ds Aq '
>>>    .ss \n[.ss] 0
>>> @@ -172,7 +172,10 @@ MINOR: the minor version integer value
>>>    .nf
>>>    .fam C
>>>    The minorversion field is optional. If not given, it will disable or enable
>>> -all minorversions for that major version.
>>> +all minorversions for that major version.  Note however that if NFSv4.0 was
>>> +previously disabled, it can only be re\-enabled by explicitly specifying the
>>> +minorversion (this mirrors the behavior of the /proc/fs/nfsd/versions
>>> +interface).
>>>    .fam
>>>    .fi
>>>    .if n .RE
>>> diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
>>> index c5921458..20e9bf8e 100644
>>> --- a/utils/nfsdctl/nfsdctl.adoc
>>> +++ b/utils/nfsdctl/nfsdctl.adoc
>>> @@ -91,7 +91,10 @@ Each subcommand can also accept its own set of options and arguments. The
>>>        MINOR: the minor version integer value
>>>      The minorversion field is optional. If not given, it will disable or enable
>>> -  all minorversions for that major version.
>>> +  all minorversions for that major version.  Note however that if NFSv4.0 was
>>> +  previously disabled, it can only be re-enabled by explicitly specifying the
>>> +  minorversion (this mirrors the behavior of the /proc/fs/nfsd/versions
>>> +  interface).
>>>      Note that versions can only be set when there are no nfsd threads running.
>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>> index 722bf4a0..d86ff80e 100644
>>> --- a/utils/nfsdctl/nfsdctl.c
>>> +++ b/utils/nfsdctl/nfsdctl.c
>>> @@ -761,6 +761,32 @@ static int update_nfsd_version(int major, int minor, bool enabled)
>>>    	return -EINVAL;
>>>    }
>>> +static bool v40_is_disabled(void)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < MAX_NFS_VERSIONS; ++i) {
>>> +		if (nfsd_versions[i].major == 0)
>>> +			break;
>>> +		if (nfsd_versions[i].major == 4 && nfsd_versions[i].minor == 0)
>>> +			return !nfsd_versions[i].enabled;
>>> +	}
>>> +	return false;
>>> +}
>>> +
>>> +static int get_max_minorversion(void)
>>> +{
>>> +	int i, max = 0;
>>> +
>>> +	for (i = 0; i < MAX_NFS_VERSIONS; ++i) {
>>> +		if (nfsd_versions[i].major == 0)
>>> +			break;
>>> +		if (nfsd_versions[i].major == 4 && nfsd_versions[i].minor > max)
>>> +			max = nfsd_versions[i].minor;
>>> +	}
>>> +	return max;
>>> +}
>>> +
>>>    static void version_usage(void)
>>>    {
>>>    	printf("Usage: %s version { {+,-}major.minor } ...\n", taskname);
>>> @@ -778,7 +804,8 @@ static void version_usage(void)
>>>    static int version_func(struct nl_sock *sock, int argc, char ** argv)
>>>    {
>>> -	int ret, i;
>>> +	int ret, i, j, max_minor;
>>> +	bool v40_disabled;
>>>    	/* help is only valid as first argument after command */
>>>    	if (argc > 1 &&
>>> @@ -792,6 +819,9 @@ static int version_func(struct nl_sock *sock, int argc, char ** argv)
>>>    		return ret;
>>>    	if (argc > 1) {
>>> +		v40_disabled = v40_is_disabled();
>>> +		max_minor = get_max_minorversion();
>>> +
>>>    		for (i = 1; i < argc; ++i) {
>>>    			int ret, major, minor = 0;
>>>    			char sign = '\0', *str = argv[i];
>>> @@ -815,9 +845,29 @@ static int version_func(struct nl_sock *sock, int argc, char ** argv)
>>>    				return -EINVAL;
>>>    			}
>>> -			ret = update_nfsd_version(major, minor, enabled);
>>> -			if (ret)
>>> -				return ret;
>>> +			/*
>>> +			 * The minorversion field is optional. If omitted, it should
>>> +			 * cause all the minor versions for that major version to be
>>> +			 * enabled/disabled.
>>> +			 *
>>> +			 * HOWEVER, we do not enable v4.0 in this manner if it was
>>> +			 * previously disabled - it has to be explicitly enabled
>>> +			 * instead.  This is to retain the behavior of the old
>>> +			 * /proc/fs/nfsd/versions interface.
>>> +			 */
>>> +			if (major == 4 && ret == 2) {
>>> +				for (j = 0; j <= max_minor; ++j) {
>>> +					if (j == 0 && enabled && v40_disabled)
>>> +						continue;
>>> +					ret = update_nfsd_version(major, j, enabled);
>>> +					if (ret)
>>> +						return ret;
>>> +				}
>>> +			} else {
>>> +				ret = update_nfsd_version(major, minor, enabled);
>>> +				if (ret)
>>> +					return ret;
>>> +			}
>>>    		}
>>>    		return set_nfsd_versions(sock);
>>>    	}
>> So nfsdctl version -4 turns of all v4 versions
>> and nfsdctl version +4 only turns on +4.1 +4.2.
>> nfsdctl version +4.0 is needed to turn on 4.0
>> leaving the rest of the minor versions on
>>
>> Is this intended?
> 
> No.  Jeff and I decided not to retain that behavior.  You should be
> looking at the v3 patchset at this point.
Got it... thanks!

steved.