* [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command (take 2) @ 2010-06-03 13:02 Steve Dickson 2010-06-03 13:02 ` [PATCH 1/2] mount: silently fails when bad option values are given Steve Dickson 2010-06-03 13:02 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson 0 siblings, 2 replies; 11+ messages in thread From: Steve Dickson @ 2010-06-03 13:02 UTC (permalink / raw) To: Linux NFS Mailing List [I incorporated the comments from the code review of the previous patches] The following two patches add better error diagnostics when invalid protocol version are given and when the network protocol can not be determined. Steve Dickson (2): mount: silently fails when bad option values are given mount.nfs: silently fails when the network protocol is not found utils/mount/network.c | 39 ++++++++++++++++++++++++++++++++++----- utils/mount/nfsumount.c | 4 +--- utils/mount/stropts.c | 16 +++++++++++----- 3 files changed, 46 insertions(+), 13 deletions(-) ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] mount: silently fails when bad option values are given 2010-06-03 13:02 [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command (take 2) Steve Dickson @ 2010-06-03 13:02 ` Steve Dickson 2010-06-03 14:04 ` Chuck Lever 2010-06-03 13:02 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson 1 sibling, 1 reply; 11+ messages in thread From: Steve Dickson @ 2010-06-03 13:02 UTC (permalink / raw) To: Linux NFS Mailing List mount.nfs should not only fail when an invalid option values are supplied (as it does), it should also print a diagnostic message identifying the problem Signed-off-by: Steve Dickson <steved@redhat.com> --- utils/mount/network.c | 20 ++++++++++++++++++-- utils/mount/nfsumount.c | 4 +--- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/utils/mount/network.c b/utils/mount/network.c index c541257..d9903ed 100644 --- a/utils/mount/network.c +++ b/utils/mount/network.c @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, unsigned long *program) return 1; } case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'nfsprog=' option"), + progname); return 0; } @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) } return 0; case PO_NOT_FOUND: - nfs_error(_("%s: option parsing error\n"), + nfs_error(_("%s: parsing error on 'vers=' option\n"), progname); + return 0; case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'vers=' option"), + progname); return 0; } case 4: /* nfsvers */ @@ -1265,9 +1270,12 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) } return 0; case PO_NOT_FOUND: - nfs_error(_("%s: option parsing error\n"), + nfs_error(_("%s: parsing error on 'nfsvers=' option\n"), progname); + return 0; case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'nfsvers=' option"), + progname); return 0; } } @@ -1336,6 +1344,8 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port) return 1; } case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'port=' option"), + progname); return 0; } @@ -1423,6 +1433,8 @@ nfs_mount_program(struct mount_options *options, unsigned long *program) return 1; } case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'mountprog=' option"), + progname); return 0; } @@ -1452,6 +1464,8 @@ nfs_mount_version(struct mount_options *options, unsigned long *version) return 1; } case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'mountvers=' option"), + progname); return 0; } @@ -1510,6 +1524,8 @@ nfs_mount_port(struct mount_options *options, unsigned long *port) return 1; } case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'mountport=' option"), + progname); return 0; } diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c index 9d798a2..1514340 100644 --- a/utils/mount/nfsumount.c +++ b/utils/mount/nfsumount.c @@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct mount_options *options, struct pmap nfs_pmap, mnt_pmap; sa_family_t family; - if (!nfs_options2pmap(options, &nfs_pmap, &mnt_pmap)) { - nfs_error(_("%s: bad mount options"), progname); + if (!nfs_options2pmap(options, &nfs_pmap, &mnt_pmap)) return EX_FAIL; - } /* Skip UMNT call for vers=4 mounts */ if (nfs_pmap.pm_vers == 4) -- 1.6.6.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mount: silently fails when bad option values are given 2010-06-03 13:02 ` [PATCH 1/2] mount: silently fails when bad option values are given Steve Dickson @ 2010-06-03 14:04 ` Chuck Lever 2010-06-03 14:36 ` Steve Dickson 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever @ 2010-06-03 14:04 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing List On 06/ 3/10 09:02 AM, Steve Dickson wrote: > mount.nfs should not only fail when an invalid option values > are supplied (as it does), it should also print a diagnostic > message identifying the problem > > Signed-off-by: Steve Dickson<steved@redhat.com> > --- > utils/mount/network.c | 20 ++++++++++++++++++-- > utils/mount/nfsumount.c | 4 +--- > 2 files changed, 19 insertions(+), 5 deletions(-) > > diff --git a/utils/mount/network.c b/utils/mount/network.c > index c541257..d9903ed 100644 > --- a/utils/mount/network.c > +++ b/utils/mount/network.c > @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, unsigned long *program) > return 1; > } Another missed fall-through. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'nfsprog=' option"), > + progname); > return 0; > } > > @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) > } > return 0; > case PO_NOT_FOUND: > - nfs_error(_("%s: option parsing error\n"), > + nfs_error(_("%s: parsing error on 'vers=' option\n"), > progname); > + return 0; > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'vers=' option"), > + progname); > return 0; > } What I meant before is that, with this new code, this error diagnostic is displayed for "vers=booger" but not for "vers=12". I think it should be displayed in both cases. > case 4: /* nfsvers */ > @@ -1265,9 +1270,12 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) > } > return 0; > case PO_NOT_FOUND: > - nfs_error(_("%s: option parsing error\n"), > + nfs_error(_("%s: parsing error on 'nfsvers=' option\n"), > progname); > + return 0; > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'nfsvers=' option"), > + progname); > return 0; > } > } > @@ -1336,6 +1344,8 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port) > return 1; > } Another missed fall-through. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'port=' option"), > + progname); > return 0; > } And here, an error diagnostic is displayed for "port=crikey" but not for "port=-17". > @@ -1423,6 +1433,8 @@ nfs_mount_program(struct mount_options *options, unsigned long *program) > return 1; > } Another missed fall-through. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'mountprog=' option"), > + progname); > return 0; > } > > @@ -1452,6 +1464,8 @@ nfs_mount_version(struct mount_options *options, unsigned long *version) > return 1; > } Ditto. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'mountvers=' option"), > + progname); > return 0; > } > > @@ -1510,6 +1524,8 @@ nfs_mount_port(struct mount_options *options, unsigned long *port) > return 1; > } Ditto. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'mountport=' option"), > + progname); > return 0; > } > > diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c > index 9d798a2..1514340 100644 > --- a/utils/mount/nfsumount.c > +++ b/utils/mount/nfsumount.c > @@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct mount_options *options, > struct pmap nfs_pmap, mnt_pmap; > sa_family_t family; > > - if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap)) { > - nfs_error(_("%s: bad mount options"), progname); > + if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap)) > return EX_FAIL; > - } > > /* Skip UMNT call for vers=4 mounts */ > if (nfs_pmap.pm_vers == 4) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mount: silently fails when bad option values are given 2010-06-03 14:04 ` Chuck Lever @ 2010-06-03 14:36 ` Steve Dickson [not found] ` <4C07BE09.3060602-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Steve Dickson @ 2010-06-03 14:36 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List On 06/03/2010 10:04 AM, Chuck Lever wrote: > On 06/ 3/10 09:02 AM, Steve Dickson wrote: >> mount.nfs should not only fail when an invalid option values >> are supplied (as it does), it should also print a diagnostic >> message identifying the problem >> >> Signed-off-by: Steve Dickson<steved@redhat.com> >> --- >> utils/mount/network.c | 20 ++++++++++++++++++-- >> utils/mount/nfsumount.c | 4 +--- >> 2 files changed, 19 insertions(+), 5 deletions(-) >> >> diff --git a/utils/mount/network.c b/utils/mount/network.c >> index c541257..d9903ed 100644 >> --- a/utils/mount/network.c >> +++ b/utils/mount/network.c >> @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, >> unsigned long *program) >> return 1; >> } > > Another missed fall-through. I realized this.. but if tmp <= 0, then the given value is invalid so an error message should be displayed. > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'nfsprog=' option"), >> + progname); >> return 0; >> } >> >> @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, >> unsigned long *version) >> } >> return 0; >> case PO_NOT_FOUND: >> - nfs_error(_("%s: option parsing error\n"), >> + nfs_error(_("%s: parsing error on 'vers=' option\n"), >> progname); >> + return 0; >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'vers=' option"), >> + progname); >> return 0; >> } > > What I meant before is that, with this new code, this error diagnostic > is displayed for "vers=booger" but not for "vers=12". I think it should > be displayed in both cases. ah... This is not only routine where PO_FOUND is returned but the value is invalid... > >> case 4: /* nfsvers */ >> @@ -1265,9 +1270,12 @@ nfs_nfs_version(struct mount_options *options, >> unsigned long *version) >> } >> return 0; >> case PO_NOT_FOUND: >> - nfs_error(_("%s: option parsing error\n"), >> + nfs_error(_("%s: parsing error on 'nfsvers=' option\n"), >> progname); >> + return 0; >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'nfsvers=' option"), >> + progname); >> return 0; >> } >> } >> @@ -1336,6 +1344,8 @@ nfs_nfs_port(struct mount_options *options, >> unsigned long *port) >> return 1; >> } > > Another missed fall-through. again known... > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'port=' option"), >> + progname); >> return 0; >> } > > And here, an error diagnostic is displayed for "port=crikey" but not for > "port=-17". > >> @@ -1423,6 +1433,8 @@ nfs_mount_program(struct mount_options *options, >> unsigned long *program) >> return 1; >> } > > Another missed fall-through. Same as above... > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'mountprog=' option"), >> + progname); >> return 0; >> } >> >> @@ -1452,6 +1464,8 @@ nfs_mount_version(struct mount_options *options, >> unsigned long *version) >> return 1; >> } > > Ditto. Ditto... :-) > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'mountvers=' option"), >> + progname); >> return 0; >> } >> >> @@ -1510,6 +1524,8 @@ nfs_mount_port(struct mount_options *options, >> unsigned long *port) >> return 1; >> } > > Ditto. Double Ditto.. :-) steved. > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'mountport=' option"), >> + progname); >> return 0; >> } >> >> diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c >> index 9d798a2..1514340 100644 >> --- a/utils/mount/nfsumount.c >> +++ b/utils/mount/nfsumount.c >> @@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct >> mount_options *options, >> struct pmap nfs_pmap, mnt_pmap; >> sa_family_t family; >> >> - if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap)) { >> - nfs_error(_("%s: bad mount options"), progname); >> + if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap)) >> return EX_FAIL; >> - } >> >> /* Skip UMNT call for vers=4 mounts */ >> if (nfs_pmap.pm_vers == 4) > ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <4C07BE09.3060602-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 1/2] mount: silently fails when bad option values are given [not found] ` <4C07BE09.3060602-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> @ 2010-06-03 15:55 ` Chuck Lever 2010-06-03 16:32 ` Steve Dickson 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever @ 2010-06-03 15:55 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing List On 06/ 3/10 10:36 AM, Steve Dickson wrote: > > > On 06/03/2010 10:04 AM, Chuck Lever wrote: >> On 06/ 3/10 09:02 AM, Steve Dickson wrote: >>> mount.nfs should not only fail when an invalid option values >>> are supplied (as it does), it should also print a diagnostic >>> message identifying the problem >>> >>> Signed-off-by: Steve Dickson<steved@redhat.com> >>> --- >>> utils/mount/network.c | 20 ++++++++++++++++++-- >>> utils/mount/nfsumount.c | 4 +--- >>> 2 files changed, 19 insertions(+), 5 deletions(-) >>> >>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>> index c541257..d9903ed 100644 >>> --- a/utils/mount/network.c >>> +++ b/utils/mount/network.c >>> @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, >>> unsigned long *program) >>> return 1; >>> } >> >> Another missed fall-through. > I realized this.. but if tmp<= 0, then the given value is invalid > so an error message should be displayed. > >> >>> case PO_BAD_VALUE: >>> + nfs_error(_("%s: invalid value for 'nfsprog=' option"), >>> + progname); >>> return 0; >>> } >>> >>> @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, >>> unsigned long *version) >>> } >>> return 0; >>> case PO_NOT_FOUND: >>> - nfs_error(_("%s: option parsing error\n"), >>> + nfs_error(_("%s: parsing error on 'vers=' option\n"), >>> progname); >>> + return 0; >>> case PO_BAD_VALUE: >>> + nfs_error(_("%s: invalid value for 'vers=' option"), >>> + progname); >>> return 0; >>> } >> >> What I meant before is that, with this new code, this error diagnostic >> is displayed for "vers=booger" but not for "vers=12". I think it should >> be displayed in both cases. > ah... This is not only routine where PO_FOUND is returned but the > value is invalid... PO_FOUND here means the option was a keyword/value pair, and the value was numeric (but not necessarily a legal value for this option, so the caller has to do some range checking). PO_BAD_VALUE means the option was a keyword/value pair, and the value wasn't numeric, and is thus definitely not valid. PO_NOT_FOUND probably means the option was found, but the option isn't specified as a keyword/value; ie. "vers" by itself rather than "vers=n". (Although you should check that, my recollection may be rusty). Also invalid, and should be reported. Or, PO_NOT_FOUND could mean the option wasn't found at all, but since po_rightmost() found it, that would be a software bug in this case. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mount: silently fails when bad option values are given 2010-06-03 15:55 ` Chuck Lever @ 2010-06-03 16:32 ` Steve Dickson [not found] ` <4C07D922.7030302-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Steve Dickson @ 2010-06-03 16:32 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List On 06/03/2010 11:55 AM, Chuck Lever wrote: > On 06/ 3/10 10:36 AM, Steve Dickson wrote: >> >> >> On 06/03/2010 10:04 AM, Chuck Lever wrote: >>> On 06/ 3/10 09:02 AM, Steve Dickson wrote: >>>> mount.nfs should not only fail when an invalid option values >>>> are supplied (as it does), it should also print a diagnostic >>>> message identifying the problem >>>> >>>> Signed-off-by: Steve Dickson<steved@redhat.com> >>>> --- >>>> utils/mount/network.c | 20 ++++++++++++++++++-- >>>> utils/mount/nfsumount.c | 4 +--- >>>> 2 files changed, 19 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>>> index c541257..d9903ed 100644 >>>> --- a/utils/mount/network.c >>>> +++ b/utils/mount/network.c >>>> @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, >>>> unsigned long *program) >>>> return 1; >>>> } >>> >>> Another missed fall-through. >> I realized this.. but if tmp<= 0, then the given value is invalid >> so an error message should be displayed. >> >>> >>>> case PO_BAD_VALUE: >>>> + nfs_error(_("%s: invalid value for 'nfsprog=' option"), >>>> + progname); >>>> return 0; >>>> } >>>> >>>> @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, >>>> unsigned long *version) >>>> } >>>> return 0; >>>> case PO_NOT_FOUND: >>>> - nfs_error(_("%s: option parsing error\n"), >>>> + nfs_error(_("%s: parsing error on 'vers=' option\n"), >>>> progname); >>>> + return 0; >>>> case PO_BAD_VALUE: >>>> + nfs_error(_("%s: invalid value for 'vers=' option"), >>>> + progname); >>>> return 0; >>>> } >>> >>> What I meant before is that, with this new code, this error diagnostic >>> is displayed for "vers=booger" but not for "vers=12". I think it should >>> be displayed in both cases. >> ah... This is not only routine where PO_FOUND is returned but the >> value is invalid... > > PO_FOUND here means the option was a keyword/value pair, and the value > was numeric (but not necessarily a legal value for this option, so the > caller has to do some range checking). PO_BAD_VALUE means the option > was a keyword/value pair, and the value wasn't numeric, and is thus > definitely not valid. > > PO_NOT_FOUND probably means the option was found, but the option isn't > specified as a keyword/value; ie. "vers" by itself rather than "vers=n". > (Although you should check that, my recollection may be rusty). Also > invalid, and should be reported. > > Or, PO_NOT_FOUND could mean the option wasn't found at all, but since > po_rightmost() found it, that would be a software bug in this case. I believe I'm understanding the logic... Whether the given value is either a PO_BAD_VALUE (should be an integer and its not) or a value that is out of range (the PO_FOUND cause), the given value is still "invalid"... PO_NOT_FOUND value is basically a parsing error and if its not recoverable as with some cases, we should generate a message... So as long as we identify the above three cases and give a pointer to the incorrect option, I think that will be fine... steved. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <4C07D922.7030302-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 1/2] mount: silently fails when bad option values are given [not found] ` <4C07D922.7030302-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> @ 2010-06-03 17:38 ` Chuck Lever 2010-06-03 18:18 ` Steve Dickson 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever @ 2010-06-03 17:38 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing List On 06/ 3/10 12:32 PM, Steve Dickson wrote: > > > On 06/03/2010 11:55 AM, Chuck Lever wrote: >> On 06/ 3/10 10:36 AM, Steve Dickson wrote: >>> >>> >>> On 06/03/2010 10:04 AM, Chuck Lever wrote: >>>> On 06/ 3/10 09:02 AM, Steve Dickson wrote: >>>>> mount.nfs should not only fail when an invalid option values >>>>> are supplied (as it does), it should also print a diagnostic >>>>> message identifying the problem >>>>> >>>>> Signed-off-by: Steve Dickson<steved@redhat.com> >>>>> --- >>>>> utils/mount/network.c | 20 ++++++++++++++++++-- >>>>> utils/mount/nfsumount.c | 4 +--- >>>>> 2 files changed, 19 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>>>> index c541257..d9903ed 100644 >>>>> --- a/utils/mount/network.c >>>>> +++ b/utils/mount/network.c >>>>> @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, >>>>> unsigned long *program) >>>>> return 1; >>>>> } >>>> >>>> Another missed fall-through. >>> I realized this.. but if tmp<= 0, then the given value is invalid >>> so an error message should be displayed. >>> >>>> >>>>> case PO_BAD_VALUE: >>>>> + nfs_error(_("%s: invalid value for 'nfsprog=' option"), >>>>> + progname); >>>>> return 0; >>>>> } >>>>> >>>>> @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, >>>>> unsigned long *version) >>>>> } >>>>> return 0; >>>>> case PO_NOT_FOUND: >>>>> - nfs_error(_("%s: option parsing error\n"), >>>>> + nfs_error(_("%s: parsing error on 'vers=' option\n"), >>>>> progname); >>>>> + return 0; >>>>> case PO_BAD_VALUE: >>>>> + nfs_error(_("%s: invalid value for 'vers=' option"), >>>>> + progname); >>>>> return 0; >>>>> } >>>> >>>> What I meant before is that, with this new code, this error diagnostic >>>> is displayed for "vers=booger" but not for "vers=12". I think it should >>>> be displayed in both cases. >>> ah... This is not only routine where PO_FOUND is returned but the >>> value is invalid... >> >> PO_FOUND here means the option was a keyword/value pair, and the value >> was numeric (but not necessarily a legal value for this option, so the >> caller has to do some range checking). PO_BAD_VALUE means the option >> was a keyword/value pair, and the value wasn't numeric, and is thus >> definitely not valid. >> >> PO_NOT_FOUND probably means the option was found, but the option isn't >> specified as a keyword/value; ie. "vers" by itself rather than "vers=n". >> (Although you should check that, my recollection may be rusty). Also >> invalid, and should be reported. >> >> Or, PO_NOT_FOUND could mean the option wasn't found at all, but since >> po_rightmost() found it, that would be a software bug in this case. > I believe I'm understanding the logic... Whether the given > value is either a PO_BAD_VALUE (should be an integer and its not) > or a value that is out of range (the PO_FOUND cause), the given value > is still "invalid"... > > PO_NOT_FOUND value is basically a parsing error and if its not recoverable as > with some cases, we should generate a message... > > So as long as we identify the above three cases and give a pointer to the > incorrect option, I think that will be fine... Agreed. At this point in nfs_nfs_version() and friends, though, I don't think there's any difference between any of these cases, so you might be OK with an even simpler patch that just does: /*FALLTHROUGH*/ default: nfs_error(_("%s: bad xxx option"), progname); What do you think? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mount: silently fails when bad option values are given 2010-06-03 17:38 ` Chuck Lever @ 2010-06-03 18:18 ` Steve Dickson 0 siblings, 0 replies; 11+ messages in thread From: Steve Dickson @ 2010-06-03 18:18 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List On 06/03/2010 01:38 PM, Chuck Lever wrote: > On 06/ 3/10 12:32 PM, Steve Dickson wrote: >> >> >> On 06/03/2010 11:55 AM, Chuck Lever wrote: >>> On 06/ 3/10 10:36 AM, Steve Dickson wrote: >>>> >>>> >>>> On 06/03/2010 10:04 AM, Chuck Lever wrote: >>>>> On 06/ 3/10 09:02 AM, Steve Dickson wrote: >>>>>> mount.nfs should not only fail when an invalid option values >>>>>> are supplied (as it does), it should also print a diagnostic >>>>>> message identifying the problem >>>>>> >>>>>> Signed-off-by: Steve Dickson<steved@redhat.com> >>>>>> --- >>>>>> utils/mount/network.c | 20 ++++++++++++++++++-- >>>>>> utils/mount/nfsumount.c | 4 +--- >>>>>> 2 files changed, 19 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>>>>> index c541257..d9903ed 100644 >>>>>> --- a/utils/mount/network.c >>>>>> +++ b/utils/mount/network.c >>>>>> @@ -1212,6 +1212,8 @@ nfs_nfs_program(struct mount_options *options, >>>>>> unsigned long *program) >>>>>> return 1; >>>>>> } >>>>> >>>>> Another missed fall-through. >>>> I realized this.. but if tmp<= 0, then the given value is invalid >>>> so an error message should be displayed. >>>> >>>>> >>>>>> case PO_BAD_VALUE: >>>>>> + nfs_error(_("%s: invalid value for 'nfsprog=' option"), >>>>>> + progname); >>>>>> return 0; >>>>>> } >>>>>> >>>>>> @@ -1251,9 +1253,12 @@ nfs_nfs_version(struct mount_options *options, >>>>>> unsigned long *version) >>>>>> } >>>>>> return 0; >>>>>> case PO_NOT_FOUND: >>>>>> - nfs_error(_("%s: option parsing error\n"), >>>>>> + nfs_error(_("%s: parsing error on 'vers=' option\n"), >>>>>> progname); >>>>>> + return 0; >>>>>> case PO_BAD_VALUE: >>>>>> + nfs_error(_("%s: invalid value for 'vers=' option"), >>>>>> + progname); >>>>>> return 0; >>>>>> } >>>>> >>>>> What I meant before is that, with this new code, this error diagnostic >>>>> is displayed for "vers=booger" but not for "vers=12". I think it >>>>> should >>>>> be displayed in both cases. >>>> ah... This is not only routine where PO_FOUND is returned but the >>>> value is invalid... >>> >>> PO_FOUND here means the option was a keyword/value pair, and the value >>> was numeric (but not necessarily a legal value for this option, so the >>> caller has to do some range checking). PO_BAD_VALUE means the option >>> was a keyword/value pair, and the value wasn't numeric, and is thus >>> definitely not valid. >>> >>> PO_NOT_FOUND probably means the option was found, but the option isn't >>> specified as a keyword/value; ie. "vers" by itself rather than "vers=n". >>> (Although you should check that, my recollection may be rusty). Also >>> invalid, and should be reported. >>> >>> Or, PO_NOT_FOUND could mean the option wasn't found at all, but since >>> po_rightmost() found it, that would be a software bug in this case. >> I believe I'm understanding the logic... Whether the given >> value is either a PO_BAD_VALUE (should be an integer and its not) >> or a value that is out of range (the PO_FOUND cause), the given value >> is still "invalid"... >> >> PO_NOT_FOUND value is basically a parsing error and if its not >> recoverable as >> with some cases, we should generate a message... >> >> So as long as we identify the above three cases and give a pointer to the >> incorrect option, I think that will be fine... > > Agreed. At this point in nfs_nfs_version() and friends, though, I don't > think there's any difference between any of these cases, so you might be > OK with an even simpler patch that just does: > > /*FALLTHROUGH*/ > default: > nfs_error(_("%s: bad xxx option"), progname); > > What do you think? Basically that's what I did... Please see the latest version... steved. P.S. Thanks for taking the time!! ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found 2010-06-03 13:02 [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command (take 2) Steve Dickson 2010-06-03 13:02 ` [PATCH 1/2] mount: silently fails when bad option values are given Steve Dickson @ 2010-06-03 13:02 ` Steve Dickson 2010-06-03 14:13 ` Chuck Lever 1 sibling, 1 reply; 11+ messages in thread From: Steve Dickson @ 2010-06-03 13:02 UTC (permalink / raw) To: Linux NFS Mailing List mount.nfs should display some type of error diagnostics when the network protocol can not be determined. Signed-off-by: Steve Dickson <steved@redhat.com> --- utils/mount/network.c | 19 ++++++++++++++++--- utils/mount/stropts.c | 16 +++++++++++----- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/utils/mount/network.c b/utils/mount/network.c index d9903ed..ffb18ab 100644 --- a/utils/mount/network.c +++ b/utils/mount/network.c @@ -1311,6 +1311,8 @@ nfs_nfs_protocol(struct mount_options *options, unsigned long *protocol) if (option != NULL) { if (!nfs_get_proto(option, &family, protocol)) { errno = EPROTONOSUPPORT; + nfs_error(_("%s: Failed to find '%s' protocol"), + progname, option); return 0; } return 1; @@ -1399,8 +1401,13 @@ int nfs_nfs_proto_family(struct mount_options *options, case 2: /* proto */ option = po_get(options, "proto"); if (option != NULL && - !nfs_get_proto(option, &tmp_family, &protocol)) - goto out_err; + !nfs_get_proto(option, &tmp_family, &protocol)) { + + nfs_error(_("%s: Failed to find '%s' protocol"), + progname, option); + errno = EPROTONOSUPPORT; + return 0; + } } if (!nfs_verify_family(tmp_family)) @@ -1492,6 +1499,8 @@ nfs_mount_protocol(struct mount_options *options, unsigned long *protocol) if (option != NULL) { if (!nfs_get_proto(option, &family, protocol)) { errno = EPROTONOSUPPORT; + nfs_error(_("%s: Failed to find '%s' protocol"), + progname, option); return 0; } return 1; @@ -1551,8 +1560,12 @@ int nfs_mount_proto_family(struct mount_options *options, option = po_get(options, "mountproto"); if (option != NULL) { - if (!nfs_get_proto(option, &tmp_family, &protocol)) + if (!nfs_get_proto(option, &tmp_family, &protocol)) { + nfs_error(_("%s: Failed to find '%s' protocol"), + progname, option); + errno = EPROTONOSUPPORT; goto out_err; + } if (!nfs_verify_family(tmp_family)) goto out_err; *family = tmp_family; diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c index 98557d2..0241400 100644 --- a/utils/mount/stropts.c +++ b/utils/mount/stropts.c @@ -538,7 +538,10 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options) if (!nfs_construct_new_options(options, nfs_saddr, &nfs_pmap, mnt_saddr, &mnt_pmap)) { - errno = EINVAL; + if (rpc_createerr.cf_stat == RPC_UNKNOWNPROTO) + errno = EPROTONOSUPPORT; + else + errno = EINVAL; return 0; } @@ -586,18 +589,21 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi, errno = ENOMEM; return result; } - + errno = 0; if (!nfs_append_addr_option(sap, salen, options)) { - errno = EINVAL; + if (errno == 0) + errno = EINVAL; goto out_fail; } if (!nfs_fix_mounthost_option(options, mi->hostname)) { - errno = EINVAL; + if (errno == 0) + errno = EINVAL; goto out_fail; } if (!mi->fake && !nfs_verify_lock_option(options)) { - errno = EINVAL; + if (errno == 0) + errno = EINVAL; goto out_fail; } -- 1.6.6.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found 2010-06-03 13:02 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson @ 2010-06-03 14:13 ` Chuck Lever 2010-06-03 16:42 ` Steve Dickson 0 siblings, 1 reply; 11+ messages in thread From: Chuck Lever @ 2010-06-03 14:13 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing List On 06/ 3/10 09:02 AM, Steve Dickson wrote: > mount.nfs should display some type of error diagnostics when > the network protocol can not be determined. > > Signed-off-by: Steve Dickson<steved@redhat.com> > --- > utils/mount/network.c | 19 ++++++++++++++++--- > utils/mount/stropts.c | 16 +++++++++++----- > 2 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/utils/mount/network.c b/utils/mount/network.c > index d9903ed..ffb18ab 100644 > --- a/utils/mount/network.c > +++ b/utils/mount/network.c > @@ -1311,6 +1311,8 @@ nfs_nfs_protocol(struct mount_options *options, unsigned long *protocol) > if (option != NULL) { > if (!nfs_get_proto(option,&family, protocol)) { > errno = EPROTONOSUPPORT; > + nfs_error(_("%s: Failed to find '%s' protocol"), > + progname, option); Set the errno _after_ calling nfs_error(), which could set it to something else. nfs_error() does call C library functions which may alter errno, after all. > return 0; > } > return 1; > @@ -1399,8 +1401,13 @@ int nfs_nfs_proto_family(struct mount_options *options, > case 2: /* proto */ > option = po_get(options, "proto"); > if (option != NULL&& > - !nfs_get_proto(option,&tmp_family,&protocol)) > - goto out_err; > + !nfs_get_proto(option,&tmp_family,&protocol)) { > + > + nfs_error(_("%s: Failed to find '%s' protocol"), > + progname, option); > + errno = EPROTONOSUPPORT; > + return 0; > + } > } > > if (!nfs_verify_family(tmp_family)) > @@ -1492,6 +1499,8 @@ nfs_mount_protocol(struct mount_options *options, unsigned long *protocol) > if (option != NULL) { > if (!nfs_get_proto(option,&family, protocol)) { > errno = EPROTONOSUPPORT; > + nfs_error(_("%s: Failed to find '%s' protocol"), > + progname, option); Ditto. > return 0; > } > return 1; > @@ -1551,8 +1560,12 @@ int nfs_mount_proto_family(struct mount_options *options, > > option = po_get(options, "mountproto"); > if (option != NULL) { > - if (!nfs_get_proto(option,&tmp_family,&protocol)) > + if (!nfs_get_proto(option,&tmp_family,&protocol)) { > + nfs_error(_("%s: Failed to find '%s' protocol"), > + progname, option); > + errno = EPROTONOSUPPORT; > goto out_err; > + } Does out_err also clobber errno here as well? > if (!nfs_verify_family(tmp_family)) > goto out_err; > *family = tmp_family; > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > index 98557d2..0241400 100644 > --- a/utils/mount/stropts.c > +++ b/utils/mount/stropts.c > @@ -538,7 +538,10 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options) > > if (!nfs_construct_new_options(options, nfs_saddr,&nfs_pmap, > mnt_saddr,&mnt_pmap)) { > - errno = EINVAL; > + if (rpc_createerr.cf_stat == RPC_UNKNOWNPROTO) > + errno = EPROTONOSUPPORT; > + else > + errno = EINVAL; > return 0; > } > > @@ -586,18 +589,21 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi, > errno = ENOMEM; > return result; > } > - > + errno = 0; > if (!nfs_append_addr_option(sap, salen, options)) { > - errno = EINVAL; > + if (errno == 0) > + errno = EINVAL; > goto out_fail; What I meant before is that the errno should really be set right where the error is detected, not long afterwards in some other part of the code. Currently it looks like nfs_append_addr_option() sets the errno sometimes, and sometimes it doesn't and the errno is set here. I think the code would make more sense overall if the errno was always appropriately set by the time nfs_append_addr_option() returns. Likewise for nfs_construct_new_options() above. > } > > if (!nfs_fix_mounthost_option(options, mi->hostname)) { > - errno = EINVAL; > + if (errno == 0) > + errno = EINVAL; > goto out_fail; > } > if (!mi->fake&& !nfs_verify_lock_option(options)) { > - errno = EINVAL; > + if (errno == 0) > + errno = EINVAL; > goto out_fail; > } > Likewise for these two. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found 2010-06-03 14:13 ` Chuck Lever @ 2010-06-03 16:42 ` Steve Dickson 0 siblings, 0 replies; 11+ messages in thread From: Steve Dickson @ 2010-06-03 16:42 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List On 06/03/2010 10:13 AM, Chuck Lever wrote: > On 06/ 3/10 09:02 AM, Steve Dickson wrote: >> mount.nfs should display some type of error diagnostics when >> the network protocol can not be determined. >> >> Signed-off-by: Steve Dickson<steved@redhat.com> >> --- >> utils/mount/network.c | 19 ++++++++++++++++--- >> utils/mount/stropts.c | 16 +++++++++++----- >> 2 files changed, 27 insertions(+), 8 deletions(-) >> >> diff --git a/utils/mount/network.c b/utils/mount/network.c >> index d9903ed..ffb18ab 100644 >> --- a/utils/mount/network.c >> +++ b/utils/mount/network.c >> @@ -1311,6 +1311,8 @@ nfs_nfs_protocol(struct mount_options *options, >> unsigned long *protocol) >> if (option != NULL) { >> if (!nfs_get_proto(option,&family, protocol)) { >> errno = EPROTONOSUPPORT; >> + nfs_error(_("%s: Failed to find '%s' protocol"), >> + progname, option); > > Set the errno _after_ calling nfs_error(), which could set it to > something else. nfs_error() does call C library functions which may > alter errno, after all. point. > >> return 0; >> } >> return 1; >> @@ -1399,8 +1401,13 @@ int nfs_nfs_proto_family(struct mount_options >> *options, >> case 2: /* proto */ >> option = po_get(options, "proto"); >> if (option != NULL&& >> - !nfs_get_proto(option,&tmp_family,&protocol)) >> - goto out_err; >> + !nfs_get_proto(option,&tmp_family,&protocol)) { >> + >> + nfs_error(_("%s: Failed to find '%s' protocol"), >> + progname, option); >> + errno = EPROTONOSUPPORT; >> + return 0; >> + } >> } >> >> if (!nfs_verify_family(tmp_family)) >> @@ -1492,6 +1499,8 @@ nfs_mount_protocol(struct mount_options >> *options, unsigned long *protocol) >> if (option != NULL) { >> if (!nfs_get_proto(option,&family, protocol)) { >> errno = EPROTONOSUPPORT; >> + nfs_error(_("%s: Failed to find '%s' protocol"), >> + progname, option); > > Ditto. > >> return 0; >> } >> return 1; >> @@ -1551,8 +1560,12 @@ int nfs_mount_proto_family(struct mount_options >> *options, >> >> option = po_get(options, "mountproto"); >> if (option != NULL) { >> - if (!nfs_get_proto(option,&tmp_family,&protocol)) >> + if (!nfs_get_proto(option,&tmp_family,&protocol)) { >> + nfs_error(_("%s: Failed to find '%s' protocol"), >> + progname, option); >> + errno = EPROTONOSUPPORT; >> goto out_err; >> + } > > Does out_err also clobber errno here as well? Ah... I thought I fixed that... > >> if (!nfs_verify_family(tmp_family)) >> goto out_err; >> *family = tmp_family; >> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c >> index 98557d2..0241400 100644 >> --- a/utils/mount/stropts.c >> +++ b/utils/mount/stropts.c >> @@ -538,7 +538,10 @@ nfs_rewrite_pmap_mount_options(struct >> mount_options *options) >> >> if (!nfs_construct_new_options(options, nfs_saddr,&nfs_pmap, >> mnt_saddr,&mnt_pmap)) { >> - errno = EINVAL; >> + if (rpc_createerr.cf_stat == RPC_UNKNOWNPROTO) >> + errno = EPROTONOSUPPORT; >> + else >> + errno = EINVAL; >> return 0; >> } >> >> @@ -586,18 +589,21 @@ static int nfs_do_mount_v3v2(struct >> nfsmount_info *mi, >> errno = ENOMEM; >> return result; >> } >> - >> + errno = 0; >> if (!nfs_append_addr_option(sap, salen, options)) { >> - errno = EINVAL; >> + if (errno == 0) >> + errno = EINVAL; >> goto out_fail; > > What I meant before is that the errno should really be set right where > the error is detected, not long afterwards in some other part of the code. > > Currently it looks like nfs_append_addr_option() sets the errno > sometimes, and sometimes it doesn't and the errno is set here. I think > the code would make more sense overall if the errno was always > appropriately set by the time nfs_append_addr_option() returns. > > Likewise for nfs_construct_new_options() above. > >> } >> >> if (!nfs_fix_mounthost_option(options, mi->hostname)) { >> - errno = EINVAL; >> + if (errno == 0) >> + errno = EINVAL; >> goto out_fail; >> } >> if (!mi->fake&& !nfs_verify_lock_option(options)) { >> - errno = EINVAL; >> + if (errno == 0) >> + errno = EINVAL; >> goto out_fail; >> } >> > > Likewise for these two. Understood... Please remember the original patch was for when the network protocol can not be found... While I agree with the need of the clean, that is a bit out of the scope the original patch... I'm all for going back and doing this clean up... but a this particular moment I need to fix this bug and move on.. steved. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-06-03 18:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-03 13:02 [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command (take 2) Steve Dickson
2010-06-03 13:02 ` [PATCH 1/2] mount: silently fails when bad option values are given Steve Dickson
2010-06-03 14:04 ` Chuck Lever
2010-06-03 14:36 ` Steve Dickson
[not found] ` <4C07BE09.3060602-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org>
2010-06-03 15:55 ` Chuck Lever
2010-06-03 16:32 ` Steve Dickson
[not found] ` <4C07D922.7030302-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org>
2010-06-03 17:38 ` Chuck Lever
2010-06-03 18:18 ` Steve Dickson
2010-06-03 13:02 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson
2010-06-03 14:13 ` Chuck Lever
2010-06-03 16:42 ` Steve Dickson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).