* [PATCH] NFS: Invalid mount option values should fail even with "sloppy" @ 2009-06-12 22:19 Chuck Lever [not found] ` <20090612221659.25437.19858.stgit-r85ClMMopbrwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Chuck Lever @ 2009-06-12 22:19 UTC (permalink / raw) To: trond.myklebust; +Cc: linux-nfs Ian Kent reports: "I've noticed a couple of other regressions with the options vers and proto option of mount.nfs(8). The commands: mount -t nfs -o vers=<invalid version> <server>:/<path> /<mountpoint> mount -t nfs -o proto=<invalid proto> <server>:/<path> /<mountpoint> both immediately fail. But if the "-s" option is also used they both succeed with the mount falling back to defaults (by the look of it). In the past these failed even when the sloppy option was given, as I think they should. I believe the sloppy option is meant to allow the mount command to still function for mount options (for example in shared autofs maps) that exist on other Unix implementations but aren't present in the Linux mount.nfs(8). So, an invalid value specified for a known mount option is different to an unknown mount option and should fail appropriately." See RH bugzilla 486266. Address the problem by separating parsing errors into two categories. Invalid options will fail only if sloppy isn't set, but invalid values will always fail. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- Trond- Request For Comments only. Ian hasn't tested this yet, but I was hoping it could find its way into 2.6.31 at some point, if it looks correct to you. fs/nfs/super.c | 57 +++++++++++++++++++++++++++++++------------------------- 1 files changed, 32 insertions(+), 25 deletions(-) diff --git a/fs/nfs/super.c b/fs/nfs/super.c index 66ffd5d..e88c527 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -957,7 +957,7 @@ static int nfs_parse_mount_options(char *raw, struct nfs_parsed_mount_data *mnt) { char *p, *string, *secdata; - int rc, sloppy = 0, errors = 0; + int rc, sloppy = 0, invalid_options = 0, invalid_values = 0; if (!raw) { dfprintk(MOUNT, "NFS: mount options string was NULL.\n"); @@ -1092,77 +1092,77 @@ static int nfs_parse_mount_options(char *raw, case Opt_port: if (match_int(args, &option) || option < 0 || option > USHORT_MAX) { - errors++; + invalid_values++; nfs_parse_invalid_value("port"); } else mnt->nfs_server.port = option; break; case Opt_rsize: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("rsize"); } else mnt->rsize = option; break; case Opt_wsize: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("wsize"); } else mnt->wsize = option; break; case Opt_bsize: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("bsize"); } else mnt->bsize = option; break; case Opt_timeo: if (match_int(args, &option) || option <= 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("timeo"); } else mnt->timeo = option; break; case Opt_retrans: if (match_int(args, &option) || option <= 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("retrans"); } else mnt->retrans = option; break; case Opt_acregmin: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("acregmin"); } else mnt->acregmin = option; break; case Opt_acregmax: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("acregmax"); } else mnt->acregmax = option; break; case Opt_acdirmin: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("acdirmin"); } else mnt->acdirmin = option; break; case Opt_acdirmax: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("acdirmax"); } else mnt->acdirmax = option; break; case Opt_actimeo: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("actimeo"); } else mnt->acregmin = mnt->acregmax = @@ -1170,7 +1170,7 @@ static int nfs_parse_mount_options(char *raw, break; case Opt_namelen: if (match_int(args, &option) || option < 0) { - errors++; + invalid_values++; nfs_parse_invalid_value("namlen"); } else mnt->namlen = option; @@ -1178,7 +1178,7 @@ static int nfs_parse_mount_options(char *raw, case Opt_mountport: if (match_int(args, &option) || option < 0 || option > USHORT_MAX) { - errors++; + invalid_values++; nfs_parse_invalid_value("mountport"); } else mnt->mount_server.port = option; @@ -1187,14 +1187,14 @@ static int nfs_parse_mount_options(char *raw, if (match_int(args, &option) || option < NFS_MNT_VERSION || option > NFS_MNT3_VERSION) { - errors++; + invalid_values++; nfs_parse_invalid_value("mountvers"); } else mnt->mount_server.version = option; break; case Opt_nfsvers: if (match_int(args, &option)) { - errors++; + invalid_values++; nfs_parse_invalid_value("nfsvers"); break; } @@ -1206,7 +1206,7 @@ static int nfs_parse_mount_options(char *raw, mnt->flags |= NFS_MOUNT_VER3; break; default: - errors++; + invalid_values++; nfs_parse_invalid_value("nfsvers"); } break; @@ -1221,7 +1221,7 @@ static int nfs_parse_mount_options(char *raw, rc = nfs_parse_security_flavors(string, mnt); kfree(string); if (!rc) { - errors++; + invalid_values++; dfprintk(MOUNT, "NFS: unrecognized " "security flavor\n"); } @@ -1249,7 +1249,7 @@ static int nfs_parse_mount_options(char *raw, xprt_load_transport(string); break; default: - errors++; + invalid_values++; dfprintk(MOUNT, "NFS: unrecognized " "transport protocol\n"); } @@ -1272,7 +1272,7 @@ static int nfs_parse_mount_options(char *raw, break; case Opt_xprt_rdma: /* not used for side protocols */ default: - errors++; + invalid_values++; dfprintk(MOUNT, "NFS: unrecognized " "transport protocol\n"); } @@ -1330,7 +1330,7 @@ static int nfs_parse_mount_options(char *raw, mnt->flags |= NFS_MOUNT_LOOKUP_CACHE_NONEG|NFS_MOUNT_LOOKUP_CACHE_NONE; break; default: - errors++; + invalid_values++; dfprintk(MOUNT, "NFS: invalid " "lookupcache argument\n"); }; @@ -1350,15 +1350,22 @@ static int nfs_parse_mount_options(char *raw, break; default: - errors++; + invalid_options++; dfprintk(MOUNT, "NFS: unrecognized mount option " "'%s'\n", p); } } - if (errors > 0) { - dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n", - errors, (errors == 1 ? "" : "s")); + if (invalid_values > 0) { + dfprintk(MOUNT, "NFS: parsing encountered %d invalid value%s\n", + invalid_values, + invalid_values == 1 ? "" : "s"); + return 0; + } + if (invalid_options > 0) { + dfprintk(MOUNT, "NFS: parsing encountered %d invalid option%s\n", + invalid_options, + invalid_options == 1 ? "" : "s"); if (!sloppy) return 0; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
[parent not found: <20090612221659.25437.19858.stgit-r85ClMMopbrwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>]
* Re: [PATCH] NFS: Invalid mount option values should fail even with "sloppy" [not found] ` <20090612221659.25437.19858.stgit-r85ClMMopbrwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org> @ 2009-06-13 17:07 ` Trond Myklebust [not found] ` <1244912830.6803.4.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Trond Myklebust @ 2009-06-13 17:07 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs On Fri, 2009-06-12 at 18:19 -0400, Chuck Lever wrote: > Ian Kent reports: > > "I've noticed a couple of other regressions with the options vers > and proto option of mount.nfs(8). > > The commands: > > mount -t nfs -o vers=<invalid version> <server>:/<path> /<mountpoint> > mount -t nfs -o proto=<invalid proto> <server>:/<path> /<mountpoint> > > both immediately fail. > > But if the "-s" option is also used they both succeed with the > mount falling back to defaults (by the look of it). > > In the past these failed even when the sloppy option was given, as > I think they should. I believe the sloppy option is meant to allow > the mount command to still function for mount options (for example > in shared autofs maps) that exist on other Unix implementations but > aren't present in the Linux mount.nfs(8). So, an invalid value > specified for a known mount option is different to an unknown mount > option and should fail appropriately." > > See RH bugzilla 486266. > > Address the problem by separating parsing errors into two categories. > Invalid options will fail only if sloppy isn't set, but invalid > values will always fail. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > > Trond- > > Request For Comments only. Ian hasn't tested this yet, but I was hoping > it could find its way into 2.6.31 at some point, if it looks correct to > you. > > fs/nfs/super.c | 57 +++++++++++++++++++++++++++++++------------------------- > 1 files changed, 32 insertions(+), 25 deletions(-) > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > index 66ffd5d..e88c527 100644 > --- a/fs/nfs/super.c > +++ b/fs/nfs/super.c > @@ -957,7 +957,7 @@ static int nfs_parse_mount_options(char *raw, > struct nfs_parsed_mount_data *mnt) > { > char *p, *string, *secdata; > - int rc, sloppy = 0, errors = 0; > + int rc, sloppy = 0, invalid_options = 0, invalid_values = 0; Why add a counter? Why not just add a variable to hold the return value int ret = 0; ...and have the invalid values set ret=1? > if (!raw) { > dfprintk(MOUNT, "NFS: mount options string was NULL.\n"); > @@ -1092,77 +1092,77 @@ static int nfs_parse_mount_options(char *raw, > case Opt_port: > if (match_int(args, &option) || > option < 0 || option > USHORT_MAX) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("port"); > } else > mnt->nfs_server.port = option; > break; > case Opt_rsize: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("rsize"); > } else > mnt->rsize = option; > break; > case Opt_wsize: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("wsize"); > } else > mnt->wsize = option; > break; > case Opt_bsize: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("bsize"); > } else > mnt->bsize = option; > break; > case Opt_timeo: > if (match_int(args, &option) || option <= 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("timeo"); > } else > mnt->timeo = option; > break; > case Opt_retrans: > if (match_int(args, &option) || option <= 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("retrans"); > } else > mnt->retrans = option; > break; > case Opt_acregmin: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("acregmin"); > } else > mnt->acregmin = option; > break; > case Opt_acregmax: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("acregmax"); > } else > mnt->acregmax = option; > break; > case Opt_acdirmin: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("acdirmin"); > } else > mnt->acdirmin = option; > break; > case Opt_acdirmax: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("acdirmax"); > } else > mnt->acdirmax = option; > break; > case Opt_actimeo: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("actimeo"); > } else > mnt->acregmin = mnt->acregmax = > @@ -1170,7 +1170,7 @@ static int nfs_parse_mount_options(char *raw, > break; > case Opt_namelen: > if (match_int(args, &option) || option < 0) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("namlen"); > } else > mnt->namlen = option; > @@ -1178,7 +1178,7 @@ static int nfs_parse_mount_options(char *raw, > case Opt_mountport: > if (match_int(args, &option) || > option < 0 || option > USHORT_MAX) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("mountport"); > } else > mnt->mount_server.port = option; > @@ -1187,14 +1187,14 @@ static int nfs_parse_mount_options(char *raw, > if (match_int(args, &option) || > option < NFS_MNT_VERSION || > option > NFS_MNT3_VERSION) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("mountvers"); > } else > mnt->mount_server.version = option; > break; > case Opt_nfsvers: > if (match_int(args, &option)) { > - errors++; > + invalid_values++; > nfs_parse_invalid_value("nfsvers"); > break; > } > @@ -1206,7 +1206,7 @@ static int nfs_parse_mount_options(char *raw, > mnt->flags |= NFS_MOUNT_VER3; > break; > default: > - errors++; > + invalid_values++; > nfs_parse_invalid_value("nfsvers"); > } > break; > @@ -1221,7 +1221,7 @@ static int nfs_parse_mount_options(char *raw, > rc = nfs_parse_security_flavors(string, mnt); > kfree(string); > if (!rc) { > - errors++; > + invalid_values++; > dfprintk(MOUNT, "NFS: unrecognized " > "security flavor\n"); > } > @@ -1249,7 +1249,7 @@ static int nfs_parse_mount_options(char *raw, > xprt_load_transport(string); > break; > default: > - errors++; > + invalid_values++; > dfprintk(MOUNT, "NFS: unrecognized " > "transport protocol\n"); > } > @@ -1272,7 +1272,7 @@ static int nfs_parse_mount_options(char *raw, > break; > case Opt_xprt_rdma: /* not used for side protocols */ > default: > - errors++; > + invalid_values++; > dfprintk(MOUNT, "NFS: unrecognized " > "transport protocol\n"); > } > @@ -1330,7 +1330,7 @@ static int nfs_parse_mount_options(char *raw, > mnt->flags |= NFS_MOUNT_LOOKUP_CACHE_NONEG|NFS_MOUNT_LOOKUP_CACHE_NONE; > break; > default: > - errors++; > + invalid_values++; > dfprintk(MOUNT, "NFS: invalid " > "lookupcache argument\n"); > }; > @@ -1350,15 +1350,22 @@ static int nfs_parse_mount_options(char *raw, > break; > > default: > - errors++; > + invalid_options++; > dfprintk(MOUNT, "NFS: unrecognized mount option " > "'%s'\n", p); > } > } > > - if (errors > 0) { > - dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n", > - errors, (errors == 1 ? "" : "s")); > + if (invalid_values > 0) { > + dfprintk(MOUNT, "NFS: parsing encountered %d invalid value%s\n", > + invalid_values, > + invalid_values == 1 ? "" : "s"); > + return 0; > + } > + if (invalid_options > 0) { > + dfprintk(MOUNT, "NFS: parsing encountered %d invalid option%s\n", > + invalid_options, > + invalid_options == 1 ? "" : "s"); > if (!sloppy) > return 0; > } > > -- > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <1244912830.6803.4.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: [PATCH] NFS: Invalid mount option values should fail even with "sloppy" [not found] ` <1244912830.6803.4.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> @ 2009-06-13 21:55 ` Chuck Lever 2009-06-13 22:13 ` Trond Myklebust 0 siblings, 1 reply; 4+ messages in thread From: Chuck Lever @ 2009-06-13 21:55 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Jun 13, 2009, at 1:07 PM, Trond Myklebust wrote: > On Fri, 2009-06-12 at 18:19 -0400, Chuck Lever wrote: >> Ian Kent reports: >> >> "I've noticed a couple of other regressions with the options vers >> and proto option of mount.nfs(8). >> >> The commands: >> >> mount -t nfs -o vers=<invalid version> <server>:/<path> /<mountpoint> >> mount -t nfs -o proto=<invalid proto> <server>:/<path> /<mountpoint> >> >> both immediately fail. >> >> But if the "-s" option is also used they both succeed with the >> mount falling back to defaults (by the look of it). >> >> In the past these failed even when the sloppy option was given, as >> I think they should. I believe the sloppy option is meant to allow >> the mount command to still function for mount options (for example >> in shared autofs maps) that exist on other Unix implementations but >> aren't present in the Linux mount.nfs(8). So, an invalid value >> specified for a known mount option is different to an unknown mount >> option and should fail appropriately." >> >> See RH bugzilla 486266. >> >> Address the problem by separating parsing errors into two categories. >> Invalid options will fail only if sloppy isn't set, but invalid >> values will always fail. >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> >> Trond- >> >> Request For Comments only. Ian hasn't tested this yet, but I was >> hoping >> it could find its way into 2.6.31 at some point, if it looks >> correct to >> you. >> >> fs/nfs/super.c | 57 ++++++++++++++++++++++++++++++ >> +------------------------- >> 1 files changed, 32 insertions(+), 25 deletions(-) >> >> diff --git a/fs/nfs/super.c b/fs/nfs/super.c >> index 66ffd5d..e88c527 100644 >> --- a/fs/nfs/super.c >> +++ b/fs/nfs/super.c >> @@ -957,7 +957,7 @@ static int nfs_parse_mount_options(char *raw, >> struct nfs_parsed_mount_data *mnt) >> { >> char *p, *string, *secdata; >> - int rc, sloppy = 0, errors = 0; >> + int rc, sloppy = 0, invalid_options = 0, invalid_values = 0; > > Why add a counter? Why not just add a variable to hold the return > value > > int ret = 0; > > ...and have the invalid values set ret=1? That would probably work for invalid values, which should always cause a failure. I guess I preferred treating the two as similarly as possible until the logic at the end. That seemed a little more clear to me. We still need to sort invalid options if sloppy is set. Renaming the variable @errors seems like a good way to document what exactly we're counting. I'll respin. >> if (!raw) { >> dfprintk(MOUNT, "NFS: mount options string was NULL.\n"); >> @@ -1092,77 +1092,77 @@ static int nfs_parse_mount_options(char *raw, >> case Opt_port: >> if (match_int(args, &option) || >> option < 0 || option > USHORT_MAX) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("port"); >> } else >> mnt->nfs_server.port = option; >> break; >> case Opt_rsize: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("rsize"); >> } else >> mnt->rsize = option; >> break; >> case Opt_wsize: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("wsize"); >> } else >> mnt->wsize = option; >> break; >> case Opt_bsize: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("bsize"); >> } else >> mnt->bsize = option; >> break; >> case Opt_timeo: >> if (match_int(args, &option) || option <= 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("timeo"); >> } else >> mnt->timeo = option; >> break; >> case Opt_retrans: >> if (match_int(args, &option) || option <= 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("retrans"); >> } else >> mnt->retrans = option; >> break; >> case Opt_acregmin: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("acregmin"); >> } else >> mnt->acregmin = option; >> break; >> case Opt_acregmax: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("acregmax"); >> } else >> mnt->acregmax = option; >> break; >> case Opt_acdirmin: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("acdirmin"); >> } else >> mnt->acdirmin = option; >> break; >> case Opt_acdirmax: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("acdirmax"); >> } else >> mnt->acdirmax = option; >> break; >> case Opt_actimeo: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("actimeo"); >> } else >> mnt->acregmin = mnt->acregmax = >> @@ -1170,7 +1170,7 @@ static int nfs_parse_mount_options(char *raw, >> break; >> case Opt_namelen: >> if (match_int(args, &option) || option < 0) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("namlen"); >> } else >> mnt->namlen = option; >> @@ -1178,7 +1178,7 @@ static int nfs_parse_mount_options(char *raw, >> case Opt_mountport: >> if (match_int(args, &option) || >> option < 0 || option > USHORT_MAX) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("mountport"); >> } else >> mnt->mount_server.port = option; >> @@ -1187,14 +1187,14 @@ static int nfs_parse_mount_options(char *raw, >> if (match_int(args, &option) || >> option < NFS_MNT_VERSION || >> option > NFS_MNT3_VERSION) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("mountvers"); >> } else >> mnt->mount_server.version = option; >> break; >> case Opt_nfsvers: >> if (match_int(args, &option)) { >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("nfsvers"); >> break; >> } >> @@ -1206,7 +1206,7 @@ static int nfs_parse_mount_options(char *raw, >> mnt->flags |= NFS_MOUNT_VER3; >> break; >> default: >> - errors++; >> + invalid_values++; >> nfs_parse_invalid_value("nfsvers"); >> } >> break; >> @@ -1221,7 +1221,7 @@ static int nfs_parse_mount_options(char *raw, >> rc = nfs_parse_security_flavors(string, mnt); >> kfree(string); >> if (!rc) { >> - errors++; >> + invalid_values++; >> dfprintk(MOUNT, "NFS: unrecognized " >> "security flavor\n"); >> } >> @@ -1249,7 +1249,7 @@ static int nfs_parse_mount_options(char *raw, >> xprt_load_transport(string); >> break; >> default: >> - errors++; >> + invalid_values++; >> dfprintk(MOUNT, "NFS: unrecognized " >> "transport protocol\n"); >> } >> @@ -1272,7 +1272,7 @@ static int nfs_parse_mount_options(char *raw, >> break; >> case Opt_xprt_rdma: /* not used for side protocols */ >> default: >> - errors++; >> + invalid_values++; >> dfprintk(MOUNT, "NFS: unrecognized " >> "transport protocol\n"); >> } >> @@ -1330,7 +1330,7 @@ static int nfs_parse_mount_options(char *raw, >> mnt->flags |= NFS_MOUNT_LOOKUP_CACHE_NONEG| >> NFS_MOUNT_LOOKUP_CACHE_NONE; >> break; >> default: >> - errors++; >> + invalid_values++; >> dfprintk(MOUNT, "NFS: invalid " >> "lookupcache argument\n"); >> }; >> @@ -1350,15 +1350,22 @@ static int nfs_parse_mount_options(char *raw, >> break; >> >> default: >> - errors++; >> + invalid_options++; >> dfprintk(MOUNT, "NFS: unrecognized mount option " >> "'%s'\n", p); >> } >> } >> >> - if (errors > 0) { >> - dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n", >> - errors, (errors == 1 ? "" : "s")); >> + if (invalid_values > 0) { >> + dfprintk(MOUNT, "NFS: parsing encountered %d invalid value%s\n", >> + invalid_values, >> + invalid_values == 1 ? "" : "s"); >> + return 0; >> + } >> + if (invalid_options > 0) { >> + dfprintk(MOUNT, "NFS: parsing encountered %d invalid option%s\n", >> + invalid_options, >> + invalid_options == 1 ? "" : "s"); >> if (!sloppy) >> return 0; >> } >> >> -- >> 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 > > -- Chuck Lever chuck[dot]lever[at]oracle[dot]com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] NFS: Invalid mount option values should fail even with "sloppy" 2009-06-13 21:55 ` Chuck Lever @ 2009-06-13 22:13 ` Trond Myklebust 0 siblings, 0 replies; 4+ messages in thread From: Trond Myklebust @ 2009-06-13 22:13 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs On Sat, 2009-06-13 at 17:55 -0400, Chuck Lever wrote: > On Jun 13, 2009, at 1:07 PM, Trond Myklebust wrote: > > > On Fri, 2009-06-12 at 18:19 -0400, Chuck Lever wrote: > >> Ian Kent reports: > >> > >> "I've noticed a couple of other regressions with the options vers > >> and proto option of mount.nfs(8). > >> > >> The commands: > >> > >> mount -t nfs -o vers=<invalid version> <server>:/<path> /<mountpoint> > >> mount -t nfs -o proto=<invalid proto> <server>:/<path> /<mountpoint> > >> > >> both immediately fail. > >> > >> But if the "-s" option is also used they both succeed with the > >> mount falling back to defaults (by the look of it). > >> > >> In the past these failed even when the sloppy option was given, as > >> I think they should. I believe the sloppy option is meant to allow > >> the mount command to still function for mount options (for example > >> in shared autofs maps) that exist on other Unix implementations but > >> aren't present in the Linux mount.nfs(8). So, an invalid value > >> specified for a known mount option is different to an unknown mount > >> option and should fail appropriately." > >> > >> See RH bugzilla 486266. > >> > >> Address the problem by separating parsing errors into two categories. > >> Invalid options will fail only if sloppy isn't set, but invalid > >> values will always fail. > >> > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> > >> Trond- > >> > >> Request For Comments only. Ian hasn't tested this yet, but I was > >> hoping > >> it could find its way into 2.6.31 at some point, if it looks > >> correct to > >> you. > >> > >> fs/nfs/super.c | 57 ++++++++++++++++++++++++++++++ > >> +------------------------- > >> 1 files changed, 32 insertions(+), 25 deletions(-) > >> > >> diff --git a/fs/nfs/super.c b/fs/nfs/super.c > >> index 66ffd5d..e88c527 100644 > >> --- a/fs/nfs/super.c > >> +++ b/fs/nfs/super.c > >> @@ -957,7 +957,7 @@ static int nfs_parse_mount_options(char *raw, > >> struct nfs_parsed_mount_data *mnt) > >> { > >> char *p, *string, *secdata; > >> - int rc, sloppy = 0, errors = 0; > >> + int rc, sloppy = 0, invalid_options = 0, invalid_values = 0; > > > > Why add a counter? Why not just add a variable to hold the return > > value > > > > int ret = 0; > > > > ...and have the invalid values set ret=1? > > That would probably work for invalid values, which should always cause > a failure. I guess I preferred treating the two as similarly as > possible until the logic at the end. That seemed a little more clear > to me. They're really quite different. In the case of an invalid value, you know that you can immediately exit the parser and return an error, whereas in the case of an unknown option you have to continue parsing until you know whether or not 'sloppy' has been set. > We still need to sort invalid options if sloppy is set. Renaming the > variable @errors seems like a good way to document what exactly we're > counting. Agreed. Cheers Trond ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-06-13 22:13 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-12 22:19 [PATCH] NFS: Invalid mount option values should fail even with "sloppy" Chuck Lever [not found] ` <20090612221659.25437.19858.stgit-r85ClMMopbrwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org> 2009-06-13 17:07 ` Trond Myklebust [not found] ` <1244912830.6803.4.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> 2009-06-13 21:55 ` Chuck Lever 2009-06-13 22:13 ` Trond Myklebust
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.