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

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