From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chuck Lever Subject: Re: [PATCH 1/2] mount: silently fails when bad option values are given Date: Thu, 03 Jun 2010 11:55:16 -0400 Message-ID: <4C07D064.7070707@oracle.com> References: <1275570134-24864-1-git-send-email-steved@redhat.com> <1275570134-24864-2-git-send-email-steved@redhat.com> <4C07B669.2040300@oracle.com> <4C07BE09.3060602@RedHat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Cc: Linux NFS Mailing List To: Steve Dickson Return-path: Received: from rcsinet10.oracle.com ([148.87.113.121]:16873 "EHLO rcsinet10.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751818Ab0FCP4o (ORCPT ); Thu, 3 Jun 2010 11:56:44 -0400 In-Reply-To: <4C07BE09.3060602-AfCzQyP5zfLQT0dZR+AlfA@public.gmane.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 >>> --- >>> 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.