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

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

[Steve Dickson]

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.nfs: silently fails with bad version arguments
  mount.nfs: silently fails when the network protocol is not found

 utils/mount/network.c |   21 +++++++++++++++++++--
 utils/mount/stropts.c |   16 +++++++++++-----
 2 files changed, 30 insertions(+), 7 deletions(-)

[PATCH 1/2] mount.nfs: silently fails with bad version arguments

[Steve Dickson]

mount.nfs should not only fail when an invalid protocol
option is used (as it does), it should also print a
diagnostic identifying the problem.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/mount/network.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index c541257..de1014d 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version)
 			nfs_error(_("%s: option parsing error\n"),
 					progname);
 		case PO_BAD_VALUE:
+			nfs_error(_("%s: invalid value for 'vers=' option"),
+					progname);
 			return 0;
 		}
 	case 4: /* nfsvers */
@@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version)
 			nfs_error(_("%s: option parsing error\n"),
 					progname);
 		case PO_BAD_VALUE:
+			nfs_error(_("%s: invalid value for 'nfsvers=' option"),
+					progname);
 			return 0;
 		}
 	}
-- 
1.6.6.1

Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments

[Chuck Lever]

On 06/ 2/10 09:41 AM, Steve Dickson wrote:
> mount.nfs should not only fail when an invalid protocol
> option is used (as it does), it should also print a
> diagnostic identifying the problem.
>
> Signed-off-by: Steve Dickson<steved@redhat.com>
> ---
>   utils/mount/network.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index c541257..de1014d 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version)
>   			nfs_error(_("%s: option parsing error\n"),
>   					progname);

Watch out for case fall-through.  You need to add:

			return 0;

here.

>   		case PO_BAD_VALUE:
> +			nfs_error(_("%s: invalid value for 'vers=' option"),
> +					progname);
>   			return 0;
>   		}
>   	case 4: /* nfsvers */
> @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options, unsigned long *version)
>   			nfs_error(_("%s: option parsing error\n"),
>   					progname);

Case fall-through here as well.

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

Why wouldn't you also print a diagnostic if a numeric value was used, 
but the value was out of range?

And, what about similar cases in nfs_nfs_port(), nfs_nfs_program(), 
nfs_mount_program(), nfs_mount_version(), and nfs_mount_port() ?

>   			return 0;
>   		}
>   	}

Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments

[Steve Dickson]

On 06/02/2010 05:34 PM, Chuck Lever wrote:
> On 06/ 2/10 09:41 AM, Steve Dickson wrote:
>> mount.nfs should not only fail when an invalid protocol
>> option is used (as it does), it should also print a
>> diagnostic identifying the problem.
>>
>> Signed-off-by: Steve Dickson<steved@redhat.com>
>> ---
>>   utils/mount/network.c |    4 ++++
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index c541257..de1014d 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options,
>> unsigned long *version)
>>               nfs_error(_("%s: option parsing error\n"),
>>                       progname);
> 
> Watch out for case fall-through.  You need to add:
> 
>             return 0;
> 
> here.
Ah I didn't see that... good catch... 
> 
>>           case PO_BAD_VALUE:
>> +            nfs_error(_("%s: invalid value for 'vers=' option"),
>> +                    progname);
>>               return 0;
>>           }
>>       case 4: /* nfsvers */
>> @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options,
>> unsigned long *version)
>>               nfs_error(_("%s: option parsing error\n"),
>>                       progname);
> 
> Case fall-through here as well.
> 
>>           case PO_BAD_VALUE:
>> +            nfs_error(_("%s: invalid value for 'nfsvers=' option"),
>> +                    progname);
> 
> Why wouldn't you also print a diagnostic if a numeric value was used,
> but the value was out of range?
It did not appear there was any guarantee that &tmp would have been filled
with anything useful, especially with something like nfsvers=v3 is given.

> 
> And, what about similar cases in nfs_nfs_port(), nfs_nfs_program(),
> nfs_mount_program(), nfs_mount_version(), and nfs_mount_port() ?
I was just looking at the version and protocol options and I really
didn't want to make things too verbose. Meaning we don't need to be printing
two or three messages for one error... 

But I do see the need of going back and taking a good hard look
at all the diagnostics that do (and do not) come out of the mount
command in error conditions... since it appears the diagnostics 
messages are a bit light at times..

steved.

Re: [PATCH 1/2] mount.nfs: silently fails with bad version arguments

[Steve Dickson]

On 06/03/2010 08:11 AM, Steve Dickson wrote:
> 
> 
> On 06/02/2010 05:34 PM, Chuck Lever wrote:
>> On 06/ 2/10 09:41 AM, Steve Dickson wrote:
>>> mount.nfs should not only fail when an invalid protocol
>>> option is used (as it does), it should also print a
>>> diagnostic identifying the problem.
>>>
>>> Signed-off-by: Steve Dickson<steved@redhat.com>
>>> ---
>>>   utils/mount/network.c |    4 ++++
>>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>> index c541257..de1014d 100644
>>> --- a/utils/mount/network.c
>>> +++ b/utils/mount/network.c
>>> @@ -1254,6 +1254,8 @@ nfs_nfs_version(struct mount_options *options,
>>> unsigned long *version)
>>>               nfs_error(_("%s: option parsing error\n"),
>>>                       progname);
>>
>> Watch out for case fall-through.  You need to add:
>>
>>             return 0;
>>
>> here.
> Ah I didn't see that... good catch... 
>>
>>>           case PO_BAD_VALUE:
>>> +            nfs_error(_("%s: invalid value for 'vers=' option"),
>>> +                    progname);
>>>               return 0;
>>>           }
>>>       case 4: /* nfsvers */
>>> @@ -1268,6 +1270,8 @@ nfs_nfs_version(struct mount_options *options,
>>> unsigned long *version)
>>>               nfs_error(_("%s: option parsing error\n"),
>>>                       progname);
>>
>> Case fall-through here as well.
>>
>>>           case PO_BAD_VALUE:
>>> +            nfs_error(_("%s: invalid value for 'nfsvers=' option"),
>>> +                    progname);
>>
>> Why wouldn't you also print a diagnostic if a numeric value was used,
>> but the value was out of range?
> It did not appear there was any guarantee that &tmp would have been filled
> with anything useful, especially with something like nfsvers=v3 is given.
> 
>>
>> And, what about similar cases in nfs_nfs_port(), nfs_nfs_program(),
>> nfs_mount_program(), nfs_mount_version(), and nfs_mount_port() ?
> I was just looking at the version and protocol options and I really
> didn't want to make things too verbose. Meaning we don't need to be printing
> two or three messages for one error... 
On second thought... it turns out making this work is not too bad..

steved.

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

diff --git a/utils/mount/network.c b/utils/mount/network.c
index de1014d..74f9cb2 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1307,6 +1307,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;
@@ -1393,8 +1395,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))
+		    !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))
@@ -1482,6 +1489,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;
@@ -1539,8 +1548,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 2/2] mount.nfs: silently fails when the network protocol is not found

[Chuck Lever]

On 06/ 2/10 09:41 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 |   17 +++++++++++++++--
>   utils/mount/stropts.c |   16 +++++++++++-----
>   2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index de1014d..74f9cb2 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -1307,6 +1307,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;
> @@ -1393,8 +1395,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))
> +		    !nfs_get_proto(option,&tmp_family,&protocol)) {
> +
> +			nfs_error(_("%s: Failed to find '%s' protocol"),
> +				progname, option);
> +			errno = EPROTONOSUPPORT;
>   			goto out_err;

Code at the out_err label will clobber the errno value you just set here.

> +		}
>   	}
>
>   	if (!nfs_verify_family(tmp_family))
> @@ -1482,6 +1489,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;
> @@ -1539,8 +1548,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;

It would be more clear if specific errnos were set in 
nfs_construct_new_options() or the functions it calls.

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

It might be more clear if nfs_append_addr_option() set the right errno. 
  Likewise for nfs_fix_mounthost_option() and nfs_verify_lock_option().

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

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

[Steve Dickson]

On 06/02/2010 05:34 PM, Chuck Lever wrote:
> On 06/ 2/10 09:41 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 |   17 +++++++++++++++--
>>   utils/mount/stropts.c |   16 +++++++++++-----
>>   2 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index de1014d..74f9cb2 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -1307,6 +1307,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;
>> @@ -1393,8 +1395,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))
>> +            !nfs_get_proto(option,&tmp_family,&protocol)) {
>> +
>> +            nfs_error(_("%s: Failed to find '%s' protocol"),
>> +                progname, option);
>> +            errno = EPROTONOSUPPORT;
>>               goto out_err;
> 
> Code at the out_err label will clobber the errno value you just set here.
Right... 

> 
>> +        }
>>       }
>>
>>       if (!nfs_verify_family(tmp_family))
>> @@ -1482,6 +1489,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;
>> @@ -1539,8 +1548,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;
> 
> It would be more clear if specific errnos were set in
> nfs_construct_new_options() or the functions it calls.
Not sure I agree with that simple if statement not be clear...
I think it pretty straightforward as to what is happening.. 
 
> 
>>           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;
> 
> It might be more clear if nfs_append_addr_option() set the right errno.
>  Likewise for nfs_fix_mounthost_option() and nfs_verify_lock_option().
Again the concept is pretty simple.. if errno is not set, then set it.. 

steved.

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

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