| Message ID | 20250108225439.814872-1-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [nfs-utils] nfsdctl: tweak the version subcommand behavior | expand |
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); > }
On Thu, 09 Jan 2025, Jeff Layton wrote: > 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. Digging thru my email archive it seems that the reason for that was to deal with really old kernels that understood +4/-4 but not +4.0/-4.0. Since old kernels obviously won't have a netlink interface for nfsd it seems we can drop this quirk at least. > > 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? It seems to me main alternatives are 1. Drop the special handling for v4.0 but allow +4/-4 to always enable/disable all minor versions, including v4.0. So a small adjustment to this patch. 2. Require the minor versions to be explicitly specified for v4. So allow +2/-2/+3/-3, but emit an error if +4/-4 is specified. Maybe emit an error if +2.0/-2.0/+3.0/-3.0 is specified. 3. Leave the code alone (so +4/-4 would be interpreted as +4.0/-4.1) and just update the man page. 4. Make the 'nfsdctl version' behavior consistent with the nfs.conf option handling in 'nfsdctl autostart'. Currently thats: * vers4=y turns on all minor versions. vers4.x=n on top of that will turn off those specific minor versions. * vers4=n turns off all minor versions. vers4.x=y on top of that has no effect. I don't really have a strong preference as long as the behavior matches whatever the man page says it is. If I had to vote I'd just say go with option 3, but if it's important for the behaviors of the different subcommands to be consistent, then we really don't have a choice and have to go with option 4. > > > 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> >
On Thu, 2025-01-09 at 10:32 -0500, Scott Mayhew wrote: > On Thu, 09 Jan 2025, Jeff Layton wrote: > > > 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. > > Digging thru my email archive it seems that the reason for that was to > deal with really old kernels that understood +4/-4 but not +4.0/-4.0. > Since old kernels obviously won't have a netlink interface for nfsd it > seems we can drop this quirk at least. > > > > 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? > > It seems to me main alternatives are > > 1. Drop the special handling for v4.0 but allow +4/-4 to always > enable/disable all minor versions, including v4.0. So a small > adjustment to this patch. > > 2. Require the minor versions to be explicitly specified for v4. So allow > +2/-2/+3/-3, but emit an error if +4/-4 is specified. Maybe emit an error > if +2.0/-2.0/+3.0/-3.0 is specified. > > 3. Leave the code alone (so +4/-4 would be interpreted as +4.0/-4.1) and > just update the man page. > > 4. Make the 'nfsdctl version' behavior consistent with the nfs.conf option > handling in 'nfsdctl autostart'. Currently thats: > > * vers4=y turns on all minor versions. vers4.x=n on top of that > will turn off those specific minor versions. > * vers4=n turns off all minor versions. vers4.x=y on top of > that has no effect. > > I don't really have a strong preference as long as the behavior matches > whatever the man page says it is. If I had to vote I'd just say go with > option 3, but if it's important for the behaviors of the different > subcommands to be consistent, then we really don't have a choice and > have to go with option 4. > Now that you've laid them out, I think option #1 sounds best. Having to specify a minorversion is a departure from the current interface, and probably more effort than it's worth. The behavior in #1 also seems the most intuitive if we're going to keep the ability to accept a value that doesn't have a minorversion. > > > 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> > > >
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); }
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(-)