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

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2025-01-08 at 17:54 -0500, 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.
> 

The question is: do we want to mirror that particular quirk in the
interface? I'm not sure if there was a logical reason for making +4
work that way in the /proc interface, so it's not clear to me that we
want to replicate that here.

Honestly, it may be better to just require explicit minorversions in
this interface and not worry about trying to interpret what +4
means. You'd need to specify "+4.1 +4.2" instead of just saying "+4",
but that doesn't seem too onerous.

Thoughts?

> 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);
>  	}

-- 
Jeff Layton <jlayton@kernel.org>