* [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command @ 2010-06-02 13:41 Steve Dickson 2010-06-02 13:41 ` [PATCH 1/2] mount.nfs: silently fails with bad version arguments Steve Dickson 2010-06-02 13:41 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson 0 siblings, 2 replies; 8+ messages in thread From: Steve Dickson @ 2010-06-02 13:41 UTC (permalink / raw) To: Linux NFS Mailing List 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.nfs: silently fails with bad version arguments mount.nfs: silently fails when the network protocol is not found utils/mount/network.c | 21 +++++++++++++++++++-- utils/mount/stropts.c | 16 +++++++++++----- 2 files changed, 30 insertions(+), 7 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] mount.nfs: silently fails with bad version arguments 2010-06-02 13:41 [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command Steve Dickson @ 2010-06-02 13:41 ` Steve Dickson 2010-06-02 21:34 ` Chuck Lever 2010-06-02 13:41 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson 1 sibling, 1 reply; 8+ messages in thread From: Steve Dickson @ 2010-06-02 13:41 UTC (permalink / raw) To: Linux NFS Mailing List mount.nfs should not only fail when an invalid protocol option is used (as it does), it should also print a diagnostic identifying the problem. Signed-off-by: Steve Dickson <steved@redhat.com> --- utils/mount/network.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/utils/mount/network.c b/utils/mount/network.c index c541257..de1014d 100644 --- a/utils/mount/network.c +++ b/utils/mount/network.c @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) nfs_error(_("%s: option parsing error\n"), progname); case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'vers=' option"), + progname); return 0; } case 4: /* nfsvers */ @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) nfs_error(_("%s: option parsing error\n"), progname); case PO_BAD_VALUE: + nfs_error(_("%s: invalid value for 'nfsvers=' option"), + progname); return 0; } } -- 1.6.6.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments 2010-06-02 13:41 ` [PATCH 1/2] mount.nfs: silently fails with bad version arguments Steve Dickson @ 2010-06-02 21:34 ` Chuck Lever 2010-06-03 12:11 ` Steve Dickson 0 siblings, 1 reply; 8+ messages in thread From: Chuck Lever @ 2010-06-02 21:34 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing List On 06/ 2/10 09:41 AM, Steve Dickson wrote: > mount.nfs should not only fail when an invalid protocol > option is used (as it does), it should also print a > diagnostic identifying the problem. > > Signed-off-by: Steve Dickson<steved@redhat.com> > --- > utils/mount/network.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/utils/mount/network.c b/utils/mount/network.c > index c541257..de1014d 100644 > --- a/utils/mount/network.c > +++ b/utils/mount/network.c > @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) > nfs_error(_("%s: option parsing error\n"), > progname); Watch out for case fall-through. You need to add: return 0; here. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'vers=' option"), > + progname); > return 0; > } > case 4: /* nfsvers */ > @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version) > nfs_error(_("%s: option parsing error\n"), > progname); Case fall-through here as well. > case PO_BAD_VALUE: > + nfs_error(_("%s: invalid value for 'nfsvers=' option"), > + progname); Why wouldn't you also print a diagnostic if a numeric value was used, but the value was out of range? And, what about similar cases in nfs_nfs_port(), nfs_nfs_program(), nfs_mount_program(), nfs_mount_version(), and nfs_mount_port() ? > return 0; > } > } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments 2010-06-02 21:34 ` Chuck Lever @ 2010-06-03 12:11 ` Steve Dickson [not found] ` <4C079BE8.5010509-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Steve Dickson @ 2010-06-03 12:11 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List On 06/02/2010 05:34 PM, Chuck Lever wrote: > On 06/ 2/10 09:41 AM, Steve Dickson wrote: >> mount.nfs should not only fail when an invalid protocol >> option is used (as it does), it should also print a >> diagnostic identifying the problem. >> >> Signed-off-by: Steve Dickson<steved@redhat.com> >> --- >> utils/mount/network.c | 4 ++++ >> 1 files changed, 4 insertions(+), 0 deletions(-) >> >> diff --git a/utils/mount/network.c b/utils/mount/network.c >> index c541257..de1014d 100644 >> --- a/utils/mount/network.c >> +++ b/utils/mount/network.c >> @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options, >> unsigned long *version) >> nfs_error(_("%s: option parsing error\n"), >> progname); > > Watch out for case fall-through. You need to add: > > return 0; > > here. Ah I didn't see that... good catch... > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'vers=' option"), >> + progname); >> return 0; >> } >> case 4: /* nfsvers */ >> @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options, >> unsigned long *version) >> nfs_error(_("%s: option parsing error\n"), >> progname); > > Case fall-through here as well. > >> case PO_BAD_VALUE: >> + nfs_error(_("%s: invalid value for 'nfsvers=' option"), >> + progname); > > Why wouldn't you also print a diagnostic if a numeric value was used, > but the value was out of range? It did not appear there was any guarantee that &tmp would have been filled with anything useful, especially with something like nfsvers=v3 is given. > > And, what about similar cases in nfs_nfs_port(), nfs_nfs_program(), > nfs_mount_program(), nfs_mount_version(), and nfs_mount_port() ? I was just looking at the version and protocol options and I really didn't want to make things too verbose. Meaning we don't need to be printing two or three messages for one error... But I do see the need of going back and taking a good hard look at all the diagnostics that do (and do not) come out of the mount command in error conditions... since it appears the diagnostics messages are a bit light at times.. steved. ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <4C079BE8.5010509-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments [not found] ` <4C079BE8.5010509-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> @ 2010-06-03 12:32 ` Steve Dickson 0 siblings, 0 replies; 8+ messages in thread From: Steve Dickson @ 2010-06-03 12:32 UTC (permalink / raw) To: Steve Dickson; +Cc: Chuck Lever, Linux NFS Mailing List On 06/03/2010 08:11 AM, Steve Dickson wrote: > > > On 06/02/2010 05:34 PM, Chuck Lever wrote: >> On 06/ 2/10 09:41 AM, Steve Dickson wrote: >>> mount.nfs should not only fail when an invalid protocol >>> option is used (as it does), it should also print a >>> diagnostic identifying the problem. >>> >>> Signed-off-by: Steve Dickson<steved@redhat.com> >>> --- >>> utils/mount/network.c | 4 ++++ >>> 1 files changed, 4 insertions(+), 0 deletions(-) >>> >>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>> index c541257..de1014d 100644 >>> --- a/utils/mount/network.c >>> +++ b/utils/mount/network.c >>> @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options, >>> unsigned long *version) >>> nfs_error(_("%s: option parsing error\n"), >>> progname); >> >> Watch out for case fall-through. You need to add: >> >> return 0; >> >> here. > Ah I didn't see that... good catch... >> >>> case PO_BAD_VALUE: >>> + nfs_error(_("%s: invalid value for 'vers=' option"), >>> + progname); >>> return 0; >>> } >>> case 4: /* nfsvers */ >>> @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options, >>> unsigned long *version) >>> nfs_error(_("%s: option parsing error\n"), >>> progname); >> >> Case fall-through here as well. >> >>> case PO_BAD_VALUE: >>> + nfs_error(_("%s: invalid value for 'nfsvers=' option"), >>> + progname); >> >> Why wouldn't you also print a diagnostic if a numeric value was used, >> but the value was out of range? > It did not appear there was any guarantee that &tmp would have been filled > with anything useful, especially with something like nfsvers=v3 is given. > >> >> And, what about similar cases in nfs_nfs_port(), nfs_nfs_program(), >> nfs_mount_program(), nfs_mount_version(), and nfs_mount_port() ? > I was just looking at the version and protocol options and I really > didn't want to make things too verbose. Meaning we don't need to be printing > two or three messages for one error... On second thought... it turns out making this work is not too bad.. steved. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found 2010-06-02 13:41 [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command Steve Dickson 2010-06-02 13:41 ` [PATCH 1/2] mount.nfs: silently fails with bad version arguments Steve Dickson @ 2010-06-02 13:41 ` Steve Dickson 2010-06-02 21:34 ` Chuck Lever 1 sibling, 1 reply; 8+ messages in thread From: Steve Dickson @ 2010-06-02 13:41 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 | 17 +++++++++++++++-- utils/mount/stropts.c | 16 +++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/utils/mount/network.c b/utils/mount/network.c index de1014d..74f9cb2 100644 --- a/utils/mount/network.c +++ b/utils/mount/network.c @@ -1307,6 +1307,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; @@ -1393,8 +1395,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)) + !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)) @@ -1482,6 +1489,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; @@ -1539,8 +1548,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] 8+ messages in thread
* Re: [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found 2010-06-02 13:41 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson @ 2010-06-02 21:34 ` Chuck Lever 2010-06-03 12:52 ` Steve Dickson 0 siblings, 1 reply; 8+ messages in thread From: Chuck Lever @ 2010-06-02 21:34 UTC (permalink / raw) To: Steve Dickson; +Cc: Linux NFS Mailing List On 06/ 2/10 09:41 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 | 17 +++++++++++++++-- > utils/mount/stropts.c | 16 +++++++++++----- > 2 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/utils/mount/network.c b/utils/mount/network.c > index de1014d..74f9cb2 100644 > --- a/utils/mount/network.c > +++ b/utils/mount/network.c > @@ -1307,6 +1307,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; > @@ -1393,8 +1395,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)) > + !nfs_get_proto(option,&tmp_family,&protocol)) { > + > + nfs_error(_("%s: Failed to find '%s' protocol"), > + progname, option); > + errno = EPROTONOSUPPORT; > goto out_err; Code at the out_err label will clobber the errno value you just set here. > + } > } > > if (!nfs_verify_family(tmp_family)) > @@ -1482,6 +1489,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; > @@ -1539,8 +1548,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; It would be more clear if specific errnos were set in nfs_construct_new_options() or the functions it calls. > 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; It might be more clear if nfs_append_addr_option() set the right errno. Likewise for nfs_fix_mounthost_option() and nfs_verify_lock_option(). > 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; > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found 2010-06-02 21:34 ` Chuck Lever @ 2010-06-03 12:52 ` Steve Dickson 0 siblings, 0 replies; 8+ messages in thread From: Steve Dickson @ 2010-06-03 12:52 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List On 06/02/2010 05:34 PM, Chuck Lever wrote: > On 06/ 2/10 09:41 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 | 17 +++++++++++++++-- >> utils/mount/stropts.c | 16 +++++++++++----- >> 2 files changed, 26 insertions(+), 7 deletions(-) >> >> diff --git a/utils/mount/network.c b/utils/mount/network.c >> index de1014d..74f9cb2 100644 >> --- a/utils/mount/network.c >> +++ b/utils/mount/network.c >> @@ -1307,6 +1307,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; >> @@ -1393,8 +1395,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)) >> + !nfs_get_proto(option,&tmp_family,&protocol)) { >> + >> + nfs_error(_("%s: Failed to find '%s' protocol"), >> + progname, option); >> + errno = EPROTONOSUPPORT; >> goto out_err; > > Code at the out_err label will clobber the errno value you just set here. Right... > >> + } >> } >> >> if (!nfs_verify_family(tmp_family)) >> @@ -1482,6 +1489,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; >> @@ -1539,8 +1548,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; > > It would be more clear if specific errnos were set in > nfs_construct_new_options() or the functions it calls. Not sure I agree with that simple if statement not be clear... I think it pretty straightforward as to what is happening.. > >> 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; > > It might be more clear if nfs_append_addr_option() set the right errno. > Likewise for nfs_fix_mounthost_option() and nfs_verify_lock_option(). Again the concept is pretty simple.. if errno is not set, then set it.. steved. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-06-03 12:52 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-02 13:41 [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command Steve Dickson
2010-06-02 13:41 ` [PATCH 1/2] mount.nfs: silently fails with bad version arguments Steve Dickson
2010-06-02 21:34 ` Chuck Lever
2010-06-03 12:11 ` Steve Dickson
[not found] ` <4C079BE8.5010509-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org>
2010-06-03 12:32 ` Steve Dickson
2010-06-02 13:41 ` [PATCH 2/2] mount.nfs: silently fails when the network protocol is not found Steve Dickson
2010-06-02 21:34 ` Chuck Lever
2010-06-03 12:52 ` 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).