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

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

[Steve Dickson]

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;
 		}
 	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;
 		}
 	case 4: /* nfsvers */
@@ -1265,9 +1270,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 'nfsvers=' option\n"),
 					progname);
+			return 0;
 		case PO_BAD_VALUE:
+			nfs_error(_("%s: invalid value for 'nfsvers=' option"),
+					progname);
 			return 0;
 		}
 	}
@@ -1336,6 +1344,8 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
 			return 1;
 		}
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'port=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1423,6 +1433,8 @@ nfs_mount_program(struct mount_options *options, unsigned long *program)
 			return 1;
 		}
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'mountprog=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1452,6 +1464,8 @@ nfs_mount_version(struct mount_options *options, unsigned long *version)
 			return 1;
 		}
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'mountvers=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1510,6 +1524,8 @@ nfs_mount_port(struct mount_options *options, unsigned long *port)
 			return 1;
 		}
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'mountport=' option"),
+				progname);
 		return 0;
 	}
 
diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index 9d798a2..1514340 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct mount_options *options,
 	struct pmap nfs_pmap, mnt_pmap;
 	sa_family_t family;
 
-	if (!nfs_options2pmap(options, &nfs_pmap, &mnt_pmap)) {
-		nfs_error(_("%s: bad mount options"), progname);
+	if (!nfs_options2pmap(options, &nfs_pmap, &mnt_pmap))
 		return EX_FAIL;
-	}
 
 	/* Skip UMNT call for vers=4 mounts */
 	if (nfs_pmap.pm_vers == 4)
-- 
1.6.6.1

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

[Chuck Lever]

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.

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

>   	case 4: /* nfsvers */
> @@ -1265,9 +1270,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 'nfsvers=' option\n"),
>   					progname);
> +			return 0;
>   		case PO_BAD_VALUE:
> +			nfs_error(_("%s: invalid value for 'nfsvers=' option"),
> +					progname);
>   			return 0;
>   		}
>   	}
> @@ -1336,6 +1344,8 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
>   			return 1;
>   		}

Another missed fall-through.

>   	case PO_BAD_VALUE:
> +		nfs_error(_("%s: invalid value for 'port=' option"),
> +				progname);
>   		return 0;
>   	}

And here, an error diagnostic is displayed for "port=crikey" but not for 
"port=-17".

> @@ -1423,6 +1433,8 @@ nfs_mount_program(struct mount_options *options, unsigned long *program)
>   			return 1;
>   		}

Another missed fall-through.

>   	case PO_BAD_VALUE:
> +		nfs_error(_("%s: invalid value for 'mountprog=' option"),
> +				progname);
>   		return 0;
>   	}
>
> @@ -1452,6 +1464,8 @@ nfs_mount_version(struct mount_options *options, unsigned long *version)
>   			return 1;
>   		}

Ditto.

>   	case PO_BAD_VALUE:
> +		nfs_error(_("%s: invalid value for 'mountvers=' option"),
> +				progname);
>   		return 0;
>   	}
>
> @@ -1510,6 +1524,8 @@ nfs_mount_port(struct mount_options *options, unsigned long *port)
>   			return 1;
>   		}

Ditto.

>   	case PO_BAD_VALUE:
> +		nfs_error(_("%s: invalid value for 'mountport=' option"),
> +				progname);
>   		return 0;
>   	}
>
> diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
> index 9d798a2..1514340 100644
> --- a/utils/mount/nfsumount.c
> +++ b/utils/mount/nfsumount.c
> @@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct mount_options *options,
>   	struct pmap nfs_pmap, mnt_pmap;
>   	sa_family_t family;
>
> -	if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap)) {
> -		nfs_error(_("%s: bad mount options"), progname);
> +	if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap))
>   		return EX_FAIL;
> -	}
>
>   	/* Skip UMNT call for vers=4 mounts */
>   	if (nfs_pmap.pm_vers == 4)

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

[Steve Dickson]

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

> 
>>       case 4: /* nfsvers */
>> @@ -1265,9 +1270,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 'nfsvers=' option\n"),
>>                       progname);
>> +            return 0;
>>           case PO_BAD_VALUE:
>> +            nfs_error(_("%s: invalid value for 'nfsvers=' option"),
>> +                    progname);
>>               return 0;
>>           }
>>       }
>> @@ -1336,6 +1344,8 @@ nfs_nfs_port(struct mount_options *options,
>> unsigned long *port)
>>               return 1;
>>           }
> 
> Another missed fall-through.
again known... 

> 
>>       case PO_BAD_VALUE:
>> +        nfs_error(_("%s: invalid value for 'port=' option"),
>> +                progname);
>>           return 0;
>>       }
> 
> And here, an error diagnostic is displayed for "port=crikey" but not for
> "port=-17".
> 
>> @@ -1423,6 +1433,8 @@ nfs_mount_program(struct mount_options *options,
>> unsigned long *program)
>>               return 1;
>>           }
> 
> Another missed fall-through.
Same as above... 

> 
>>       case PO_BAD_VALUE:
>> +        nfs_error(_("%s: invalid value for 'mountprog=' option"),
>> +                progname);
>>           return 0;
>>       }
>>
>> @@ -1452,6 +1464,8 @@ nfs_mount_version(struct mount_options *options,
>> unsigned long *version)
>>               return 1;
>>           }
> 
> Ditto.
Ditto... :-)

> 
>>       case PO_BAD_VALUE:
>> +        nfs_error(_("%s: invalid value for 'mountvers=' option"),
>> +                progname);
>>           return 0;
>>       }
>>
>> @@ -1510,6 +1524,8 @@ nfs_mount_port(struct mount_options *options,
>> unsigned long *port)
>>               return 1;
>>           }
> 
> Ditto.
Double Ditto.. :-)

steved.


> 
>>       case PO_BAD_VALUE:
>> +        nfs_error(_("%s: invalid value for 'mountport=' option"),
>> +                progname);
>>           return 0;
>>       }
>>
>> diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
>> index 9d798a2..1514340 100644
>> --- a/utils/mount/nfsumount.c
>> +++ b/utils/mount/nfsumount.c
>> @@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct
>> mount_options *options,
>>       struct pmap nfs_pmap, mnt_pmap;
>>       sa_family_t family;
>>
>> -    if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap)) {
>> -        nfs_error(_("%s: bad mount options"), progname);
>> +    if (!nfs_options2pmap(options,&nfs_pmap,&mnt_pmap))
>>           return EX_FAIL;
>> -    }
>>
>>       /* Skip UMNT call for vers=4 mounts */
>>       if (nfs_pmap.pm_vers == 4)
> 

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

[Chuck Lever]

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.

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

[Steve Dickson]

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

steved.

[PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command (take 3)

[Steve Dickson]

[I've incorporated the last few code review comments]

The following two patches add better error diagnostics when
invalid option values are given and when the network protocol
can not be determined. 

Steve Dickson (2):
  mount: silently fails when bad option values are given
  mount.nfs: silently fails when the network protocol is not found

 utils/mount/network.c   |   59 ++++++++++++++++++++++++++++++++++++----------
 utils/mount/nfsumount.c |    4 +--
 utils/mount/stropts.c   |   16 ++++++++----
 3 files changed, 58 insertions(+), 21 deletions(-)

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

[Steve Dickson]

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   |   29 +++++++++++++++++++++++++----
 utils/mount/nfsumount.c |    4 +---
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index c541257..b08ce80 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1211,7 +1211,10 @@ nfs_nfs_program(struct mount_options *options, unsigned long *program)
 			*program = tmp;
 			return 1;
 		}
+	/* FALLTHRU */
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'nfsprog=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1249,11 +1252,14 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version)
 				*version = tmp;
 				return 1;
 			}
+		/* FALLTHRU */
+		case PO_BAD_VALUE:
+			nfs_error(_("%s: invalid value for 'vers=' option"),
+					progname);
 			return 0;
 		case PO_NOT_FOUND:
-			nfs_error(_("%s: option parsing error\n"),
+			nfs_error(_("%s: parsing error on 'vers=' option\n"),
 					progname);
-		case PO_BAD_VALUE:
 			return 0;
 		}
 	case 4: /* nfsvers */
@@ -1263,11 +1269,14 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version)
 				*version = tmp;
 				return 1;
 			}
+		/* FALLTHRU */
+		case PO_BAD_VALUE:
+			nfs_error(_("%s: invalid value for 'nfsvers=' option"),
+					progname);
 			return 0;
 		case PO_NOT_FOUND:
-			nfs_error(_("%s: option parsing error\n"),
+			nfs_error(_("%s: parsing error on 'nfsvers=' option\n"),
 					progname);
-		case PO_BAD_VALUE:
 			return 0;
 		}
 	}
@@ -1335,7 +1344,10 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
 			*port = tmp;
 			return 1;
 		}
+	/* FALLTHRU */
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'port=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1422,7 +1434,10 @@ nfs_mount_program(struct mount_options *options, unsigned long *program)
 			*program = tmp;
 			return 1;
 		}
+	/* FALLTHRU */
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'mountprog=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1451,7 +1466,10 @@ nfs_mount_version(struct mount_options *options, unsigned long *version)
 			*version = tmp;
 			return 1;
 		}
+	/* FALLTHRU */
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'mountvers=' option"),
+				progname);
 		return 0;
 	}
 
@@ -1509,7 +1527,10 @@ nfs_mount_port(struct mount_options *options, unsigned long *port)
 			*port = tmp;
 			return 1;
 		}
+	/* FALLTHRU */
 	case PO_BAD_VALUE:
+		nfs_error(_("%s: invalid value for 'mountport=' option"),
+				progname);
 		return 0;
 	}
 
diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index 9d798a2..1514340 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -179,10 +179,8 @@ static int nfs_umount_do_umnt(struct mount_options *options,
 	struct pmap nfs_pmap, mnt_pmap;
 	sa_family_t family;
 
-	if (!nfs_options2pmap(options, &nfs_pmap, &mnt_pmap)) {
-		nfs_error(_("%s: bad mount options"), progname);
+	if (!nfs_options2pmap(options, &nfs_pmap, &mnt_pmap))
 		return EX_FAIL;
-	}
 
 	/* Skip UMNT call for vers=4 mounts */
 	if (nfs_pmap.pm_vers == 4)
-- 
1.6.5.2

[PATCH 2/2] mount.nfs: silently fails when the network protocol is not found

[Steve Dickson]

mount.nfs should display some type of error diagnostics when
the network protocol can not be determined.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/mount/network.c |   30 +++++++++++++++++++++---------
 utils/mount/stropts.c |   16 +++++++++++-----
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index b08ce80..7c28869 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1312,6 +1312,8 @@ nfs_nfs_protocol(struct mount_options *options, unsigned long *protocol)
 		if (option != NULL) {
 			if (!nfs_get_proto(option, &family, protocol)) {
 				errno = EPROTONOSUPPORT;
+				nfs_error(_("%s: Failed to find '%s' protocol"), 
+					progname, option);
 				return 0;
 			}
 			return 1;
@@ -1401,8 +1403,13 @@ int nfs_nfs_proto_family(struct mount_options *options,
 	case 2: /* proto */
 		option = po_get(options, "proto");
 		if (option != NULL &&
-		    !nfs_get_proto(option, &tmp_family, &protocol))
-			goto out_err;
+		    !nfs_get_proto(option, &tmp_family, &protocol)) {
+
+			nfs_error(_("%s: Failed to find '%s' protocol"), 
+				progname, option);
+			errno = EPROTONOSUPPORT;
+			return 0;
+		}
 	}
 
 	if (!nfs_verify_family(tmp_family))
@@ -1495,6 +1502,8 @@ nfs_mount_protocol(struct mount_options *options, unsigned long *protocol)
 	option = po_get(options, "mountproto");
 	if (option != NULL) {
 		if (!nfs_get_proto(option, &family, protocol)) {
+			nfs_error(_("%s: Failed to find '%s' protocol"), 
+				progname, option);
 			errno = EPROTONOSUPPORT;
 			return 0;
 		}
@@ -1556,10 +1565,16 @@ int nfs_mount_proto_family(struct mount_options *options,
 
 	option = po_get(options, "mountproto");
 	if (option != NULL) {
-		if (!nfs_get_proto(option, &tmp_family, &protocol))
-			goto out_err;
-		if (!nfs_verify_family(tmp_family))
-			goto out_err;
+		if (!nfs_get_proto(option, &tmp_family, &protocol)) {
+			nfs_error(_("%s: Failed to find '%s' protocol"), 
+				progname, option);
+			errno = EPROTONOSUPPORT;
+			return 0;
+		}
+		if (!nfs_verify_family(tmp_family)) {
+			errno = EAFNOSUPPORT;
+			return 0;
+		}
 		*family = tmp_family;
 		return 1;
 	}
@@ -1571,9 +1586,6 @@ int nfs_mount_proto_family(struct mount_options *options,
 	 * NFS.
 	 */
 	return nfs_nfs_proto_family(options, family);
-out_err:
-	errno = EAFNOSUPPORT;
-	return 0;
 }
 
 /**
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 98557d2..0241400 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -538,7 +538,10 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options)
 
 	if (!nfs_construct_new_options(options, nfs_saddr, &nfs_pmap,
 					mnt_saddr, &mnt_pmap)) {
-		errno = EINVAL;
+		if (rpc_createerr.cf_stat == RPC_UNKNOWNPROTO)
+			errno = EPROTONOSUPPORT;
+		else
+			errno = EINVAL;
 		return 0;
 	}
 
@@ -586,18 +589,21 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi,
 		errno = ENOMEM;
 		return result;
 	}
-
+	errno = 0;
 	if (!nfs_append_addr_option(sap, salen, options)) {
-		errno = EINVAL;
+		if (errno == 0)
+			errno = EINVAL;
 		goto out_fail;
 	}
 
 	if (!nfs_fix_mounthost_option(options, mi->hostname)) {
-		errno = EINVAL;
+		if (errno == 0)
+			errno = EINVAL;
 		goto out_fail;
 	}
 	if (!mi->fake && !nfs_verify_lock_option(options)) {
-		errno = EINVAL;
+		if (errno == 0)
+			errno = EINVAL;
 		goto out_fail;
 	}
 
-- 
1.6.5.2

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

[Chuck Lever]

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?

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

[Steve Dickson]

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

Re: [PATCH 0/2] mountd.nfs: Better error diagnostics for the mount command (take 3)

[Steve Dickson]

On 06/03/2010 12:51 PM, Steve Dickson wrote:
> [I've incorporated the last few code review comments]
> 
> The following two patches add better error diagnostics when
> invalid option values are given and when the network protocol
> can not be determined. 
> 
> Steve Dickson (2):
>   mount: silently fails when bad option values are given
>   mount.nfs: silently fails when the network protocol is not found
> 
>  utils/mount/network.c   |   59 ++++++++++++++++++++++++++++++++++++----------
>  utils/mount/nfsumount.c |    4 +--
>  utils/mount/stropts.c   |   16 ++++++++----
>  3 files changed, 58 insertions(+), 21 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Committed... 

steved.