From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Dickson Subject: Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments Date: Thu, 03 Jun 2010 08:11:20 -0400 Message-ID: <4C079BE8.5010509@RedHat.com> References: <1275486084-23899-1-git-send-email-steved@redhat.com> <1275486084-23899-2-git-send-email-steved@redhat.com> <4C06CE71.1020404@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Cc: Linux NFS Mailing List To: Chuck Lever Return-path: Received: from mx1.redhat.com ([209.132.183.28]:29148 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751616Ab0FCMLX (ORCPT ); Thu, 3 Jun 2010 08:11:23 -0400 In-Reply-To: <4C06CE71.1020404@oracle.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 >> --- >> 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.