2010-10-05 16:48:59

by Pavel Emelyanov

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

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 <[email protected]>
---
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

2010-10-06 09:46:08

by Pavel Emelyanov

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

> 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 <[email protected]>
---

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

2010-10-12 00:03:43

by J. Bruce Fields

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

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 <[email protected]>
> ---
>
> 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
>

2010-10-06 02:55:52

by Chuck Lever III

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

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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

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