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

[Steve Dickson <SteveD@redhat.com>]

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!!