| Message ID | 20180226191542.31447-2-steved@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- 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
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
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. -- 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
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;
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(-)