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...
steved.
--
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
