Re: [PATCH 1/2] mount: silently fails when bad option values are given

[Chuck Lever <chuck.lever@oracle.com>]

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.