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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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!!
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
