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

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

[Steve Dickson]

[I incorporated the comments from the code review of the 
previous patches]

The following two patches add better error diagnostics when
invalid protocol version 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   |   39 ++++++++++++++++++++++++++++++++++-----
 utils/mount/nfsumount.c |    4 +---
 utils/mount/stropts.c   |   16 +++++++++++-----
 3 files changed, 46 insertions(+), 13 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   |   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

[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 |   19 ++++++++++++++++---
 utils/mount/stropts.c |   16 +++++++++++-----
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index d9903ed..ffb18ab 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1311,6 +1311,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;
@@ -1399,8 +1401,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))
@@ -1492,6 +1499,8 @@ nfs_mount_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;
@@ -1551,8 +1560,12 @@ 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))
+		if (!nfs_get_proto(option, &tmp_family, &protocol)) {
+			nfs_error(_("%s: Failed to find '%s' protocol"), 
+				progname, option);
+			errno = EPROTONOSUPPORT;
 			goto out_err;
+		}
 		if (!nfs_verify_family(tmp_family))
 			goto out_err;
 		*family = tmp_family;
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.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 2/2] mount.nfs: silently fails when the network protocol is not found

[Chuck Lever]

On 06/ 3/10 09:02 AM, Steve Dickson wrote:
> 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 |   19 ++++++++++++++++---
>   utils/mount/stropts.c |   16 +++++++++++-----
>   2 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index d9903ed..ffb18ab 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -1311,6 +1311,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);

Set the errno _after_ calling nfs_error(), which could set it to 
something else.  nfs_error() does call C library functions which may 
alter errno, after all.

>   				return 0;
>   			}
>   			return 1;
> @@ -1399,8 +1401,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))
> @@ -1492,6 +1499,8 @@ nfs_mount_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);

Ditto.

>   			return 0;
>   		}
>   		return 1;
> @@ -1551,8 +1560,12 @@ 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))
> +		if (!nfs_get_proto(option,&tmp_family,&protocol)) {
> +			nfs_error(_("%s: Failed to find '%s' protocol"),
> +				progname, option);
> +			errno = EPROTONOSUPPORT;
>   			goto out_err;
> +		}

Does out_err also clobber errno here as well?

>   		if (!nfs_verify_family(tmp_family))
>   			goto out_err;
>   		*family = tmp_family;
> 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;

What I meant before is that the errno should really be set right where 
the error is detected, not long afterwards in some other part of the code.

Currently it looks like nfs_append_addr_option() sets the errno 
sometimes, and sometimes it doesn't and the errno is set here.  I think 
the code would make more sense overall if the errno was always 
appropriately set by the time nfs_append_addr_option() returns.

Likewise for nfs_construct_new_options() above.

>   	}
>
>   	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;
>   	}
>

Likewise for these two.

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.

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

[Steve Dickson]

On 06/03/2010 10:13 AM, Chuck Lever wrote:
> On 06/ 3/10 09:02 AM, Steve Dickson wrote:
>> 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 |   19 ++++++++++++++++---
>>   utils/mount/stropts.c |   16 +++++++++++-----
>>   2 files changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index d9903ed..ffb18ab 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -1311,6 +1311,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);
> 
> Set the errno _after_ calling nfs_error(), which could set it to
> something else.  nfs_error() does call C library functions which may
> alter errno, after all.
point.

> 
>>                   return 0;
>>               }
>>               return 1;
>> @@ -1399,8 +1401,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))
>> @@ -1492,6 +1499,8 @@ nfs_mount_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);
> 
> Ditto.
> 
>>               return 0;
>>           }
>>           return 1;
>> @@ -1551,8 +1560,12 @@ 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))
>> +        if (!nfs_get_proto(option,&tmp_family,&protocol)) {
>> +            nfs_error(_("%s: Failed to find '%s' protocol"),
>> +                progname, option);
>> +            errno = EPROTONOSUPPORT;
>>               goto out_err;
>> +        }
> 
> Does out_err also clobber errno here as well?
Ah... I thought I fixed that...

> 
>>           if (!nfs_verify_family(tmp_family))
>>               goto out_err;
>>           *family = tmp_family;
>> 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;
> 
> What I meant before is that the errno should really be set right where
> the error is detected, not long afterwards in some other part of the code.
> 
> Currently it looks like nfs_append_addr_option() sets the errno
> sometimes, and sometimes it doesn't and the errno is set here.  I think
> the code would make more sense overall if the errno was always
> appropriately set by the time nfs_append_addr_option() returns.
> 
> Likewise for nfs_construct_new_options() above.
> 
>>       }
>>
>>       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;
>>       }
>>
> 
> Likewise for these two.
Understood... Please remember the original patch was for 
when the network protocol can not be found... While I agree
with the need of the clean, that is a bit out of the scope
the original patch... 

I'm all for going back and doing this clean up... but a this particular 
moment I need to fix this bug and move on.. 

steved.

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