[PATCH] sunrpc: Don't return NULL from rpcb_create

[PATCH] sunrpc: Don't return NULL from rpcb_create

[Pavel Emelyanov]

Its callers check for ERR_PTR.

However grep shows that nobody calls this with sa_familiy other than
AF_INET/AF_INET6 so this case can be simply dropped.

Furthermore - the remaining code can be replaced with the existing helper.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
 net/sunrpc/rpcb_clnt.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 83af38d..620dee7 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -241,17 +241,7 @@ static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr,
 					RPC_CLNT_CREATE_NONPRIVPORT),
 	};
 
-	switch (srvaddr->sa_family) {
-	case AF_INET:
-		((struct sockaddr_in *)srvaddr)->sin_port = htons(RPCBIND_PORT);
-		break;
-	case AF_INET6:
-		((struct sockaddr_in6 *)srvaddr)->sin6_port = htons(RPCBIND_PORT);
-		break;
-	default:
-		return NULL;
-	}
-
+	rpc_set_port(srvaddr, RPCBIND_PORT);
 	return rpc_create(&args);
 }
 
-- 
1.5.5.6

Re: [PATCH] sunrpc: Don't return NULL from rpcb_create

[Chuck Lever]

On Oct 5, 2010, at 12:48 PM, Pavel Emelyanov wrote:

> Its callers check for ERR_PTR.

That's a good observation.

> However grep shows that nobody calls this with sa_familiy other than
> AF_INET/AF_INET6 so this case can be simply dropped.

The reason for this is in the future, we may want to support additional address family types.  We should, therefore, ensure that every piece of code that is sensitive to address families fail in some orderly manner to let developers know where a change is needed.

> Furthermore - the remaining code can be replaced with the existing helper.

A better choice is for rpcb_create() to return ERR_PTR(-EAFNOSUPPORT) or ERR_PTR(-EPFNOSUPPORT) in this case, rather than failing silently.

> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> ---
> net/sunrpc/rpcb_clnt.c |   12 +-----------
> 1 files changed, 1 insertions(+), 11 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index 83af38d..620dee7 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -241,17 +241,7 @@ static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr,
> 					RPC_CLNT_CREATE_NONPRIVPORT),
> 	};
> 
> -	switch (srvaddr->sa_family) {
> -	case AF_INET:
> -		((struct sockaddr_in *)srvaddr)->sin_port = htons(RPCBIND_PORT);
> -		break;
> -	case AF_INET6:
> -		((struct sockaddr_in6 *)srvaddr)->sin6_port = htons(RPCBIND_PORT);
> -		break;
> -	default:
> -		return NULL;
> -	}
> -
> +	rpc_set_port(srvaddr, RPCBIND_PORT);
> 	return rpc_create(&args);
> }
> 
> -- 
> 1.5.5.6
> 
> --
> 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

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

[PATCH v2] sunrpc: Don't return NULL from rpcb_create

[Pavel Emelyanov]

> The reason for this is in the future, we may want to support additional 
> address family types.  We should, therefore, ensure that every piece of 
> code that is sensitive to address families fail in some orderly manner
> to let developers know where a change is needed.

Makes sense. I was under impression, that AF-s other than INET are not
cared about at all :(

Here's a fixed version of the patch.

Log:

Its callers check for ERR_PTR.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---

 net/sunrpc/rpcb_clnt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 83af38d..1ef2d41 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -249,7 +249,7 @@ static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr,
 		((struct sockaddr_in6 *)srvaddr)->sin6_port = htons(RPCBIND_PORT);
 		break;
 	default:
-		return NULL;
+		return ERR_PTR(-EAFNOSUPPORT);
 	}
 
 	return rpc_create(&args);
-- 
1.5.5.6

Re: [PATCH v2] sunrpc: Don't return NULL from rpcb_create

[J. Bruce Fields]

On Wed, Oct 06, 2010 at 01:45:56PM +0400, Pavel Emelyanov wrote:
> > The reason for this is in the future, we may want to support additional 
> > address family types.  We should, therefore, ensure that every piece of 
> > code that is sensitive to address families fail in some orderly manner
> > to let developers know where a change is needed.
> 
> Makes sense. I was under impression, that AF-s other than INET are not
> cared about at all :(

Also leaving the rest of these to Trond; poke me if you need me to merge
anything more....

--b.

> 
> Here's a fixed version of the patch.
> 
> Log:
> 
> Its callers check for ERR_PTR.
> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> ---
> 
>  net/sunrpc/rpcb_clnt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index 83af38d..1ef2d41 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -249,7 +249,7 @@ static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr,
>  		((struct sockaddr_in6 *)srvaddr)->sin6_port = htons(RPCBIND_PORT);
>  		break;
>  	default:
> -		return NULL;
> +		return ERR_PTR(-EAFNOSUPPORT);
>  	}
>  
>  	return rpc_create(&args);
> -- 
> 1.5.5.6
>