[PATCH 0/3] NFSD IPv6 patches

[PATCH 0/3] NFSD IPv6 patches

[Chuck Lever]

Here are the last few IPv6-related patches for NFSD that I have
outstanding.  There's been a enough light testing with my mountd
prototype to suggest we are close enough to enable the kernel
support.

Please consider these for 2.6.34.

---

Aime Le Rouzic (1):
      NFSD: Support AF_INET6 in svc_addsock() function

Chuck Lever (2):
      NFSD: Enable NFS server use of PF_INET6
      SUNRPC: Use rpc_pton() in ip_map_parse()


 fs/nfsd/nfsctl.c          |   36 ++++++++++++++++++++++++++++------
 fs/nfsd/nfssvc.c          |   27 +++++++++++++++++++++-----
 net/sunrpc/svcauth_unix.c |   47 +++++++++++++++++++++++++--------------------
 net/sunrpc/svcsock.c      |    2 +-
 4 files changed, 78 insertions(+), 34 deletions(-)

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

[PATCH 1/3] SUNRPC: Use rpc_pton() in ip_map_parse()

[Chuck Lever]

The existing logic in ip_map_parse() can not currently parse
shorthanded IPv6 addresses (anything with a double colon), nor can
it parse an IPv6 presentation address with a scope ID.  An
IPv6-enabled mountd can pass down both.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 net/sunrpc/svcauth_unix.c |   47 +++++++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index d8c0411..97f0e9e 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #define RPCDBG_FACILITY	RPCDBG_AUTH
 
+#include <linux/sunrpc/clnt.h>
 
 /*
  * AUTHUNIX and AUTHNULL credentials are both handled here.
@@ -187,10 +188,13 @@ static int ip_map_parse(struct cache_detail *cd,
 	 * for scratch: */
 	char *buf = mesg;
 	int len;
-	int b1, b2, b3, b4, b5, b6, b7, b8;
-	char c;
 	char class[8];
-	struct in6_addr addr;
+	union {
+		struct sockaddr		sa;
+		struct sockaddr_in	s4;
+		struct sockaddr_in6	s6;
+	} address;
+	struct sockaddr_in6 sin6;
 	int err;
 
 	struct ip_map *ipmp;
@@ -209,24 +213,24 @@ static int ip_map_parse(struct cache_detail *cd,
 	len = qword_get(&mesg, buf, mlen);
 	if (len <= 0) return -EINVAL;
 
-	if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) == 4) {
-		addr.s6_addr32[0] = 0;
-		addr.s6_addr32[1] = 0;
-		addr.s6_addr32[2] = htonl(0xffff);
-		addr.s6_addr32[3] =
-			htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
-       } else if (sscanf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x%c",
-			&b1, &b2, &b3, &b4, &b5, &b6, &b7, &b8, &c) == 8) {
-		addr.s6_addr16[0] = htons(b1);
-		addr.s6_addr16[1] = htons(b2);
-		addr.s6_addr16[2] = htons(b3);
-		addr.s6_addr16[3] = htons(b4);
-		addr.s6_addr16[4] = htons(b5);
-		addr.s6_addr16[5] = htons(b6);
-		addr.s6_addr16[6] = htons(b7);
-		addr.s6_addr16[7] = htons(b8);
-       } else
+	if (rpc_pton(buf, len, &address.sa, sizeof(address)) == 0)
 		return -EINVAL;
+	switch (address.sa.sa_family) {
+	case AF_INET:
+		/* Form a mapped IPv4 address in sin6 */
+		memset(&sin6, 0, sizeof(sin6));
+		sin6.sin6_family = AF_INET6;
+		sin6.sin6_addr.s6_addr32[2] = htonl(0xffff);
+		sin6.sin6_addr.s6_addr32[3] = address.s4.sin_addr.s_addr;
+		break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	case AF_INET6:
+		memcpy(&sin6, &address.s6, sizeof(sin6));
+		break;
+#endif
+	default:
+		return -EINVAL;
+	}
 
 	expiry = get_expiry(&mesg);
 	if (expiry ==0)
@@ -243,7 +247,8 @@ static int ip_map_parse(struct cache_detail *cd,
 	} else
 		dom = NULL;
 
-	ipmp = ip_map_lookup(class, &addr);
+	/* IPv6 scope IDs are ignored for now */
+	ipmp = ip_map_lookup(class, &sin6.sin6_addr);
 	if (ipmp) {
 		err = ip_map_update(ipmp,
 			     container_of(dom, struct unix_domain, h),

[PATCH 2/3] NFSD: Support AF_INET6 in svc_addsock() function

[Chuck Lever]

From: Aime Le Rouzic <aime.le-rouzic@bull.net>

Relax the address family check at the top of svc_addsock() to allow AF_INET6
listener sockets to be specified via /proc/fs/nfsd/portlist.

Signed-off-by: Aime Le Rouzic <aime.le-rouzic@bull.net>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

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

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 870929e..9e09391 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1357,7 +1357,7 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 
 	if (!so)
 		return err;
-	if (so->sk->sk_family != AF_INET)
+	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
 		err =  -EAFNOSUPPORT;
 	else if (so->sk->sk_protocol != IPPROTO_TCP &&
 	    so->sk->sk_protocol != IPPROTO_UDP)

[PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[Chuck Lever]

Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
kernel.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
 fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
 2 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 2604c3e..7ebb7a5 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
 	return len;
 }
 
+static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
+					 const char *transport,
+					 const int family,
+					 const unsigned short port)
+{
+	int err;
+
+	err = svc_create_xprt(serv, transport, family, port,
+						SVC_SOCK_ANONYMOUS);
+
+	if (err < 0) {
+		/* Give a reasonable perror msg for bad transport string */
+		if (err == -ENOENT)
+			err = -EPROTONOSUPPORT;
+		return err;
+	}
+
+	return 0;
+}
+
 /*
  * A transport listener is added by writing it's transport name and
  * a port number.
@@ -1000,14 +1020,16 @@ static ssize_t __write_ports_addxprt(char *buf)
 	if (err != 0)
 		return err;
 
-	err = svc_create_xprt(nfsd_serv, transport,
-				PF_INET, port, SVC_SOCK_ANONYMOUS);
-	if (err < 0) {
-		/* Give a reasonable perror msg for bad transport string */
-		if (err == -ENOENT)
-			err = -EPROTONOSUPPORT;
+	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET, port);
+	if (err < 0)
 		return err;
-	}
+
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET6, port);
+	if (err < 0 && err != -EAFNOSUPPORT)
+		return err;
+#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
+
 	return 0;
 }
 
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 171699e..7af14c1 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -275,14 +275,32 @@ int nfsd_create_serv(void)
 	return err;
 }
 
-static int nfsd_init_socks(int port)
+static int nfsd_create_xprt(const char *transport, const unsigned short port)
+{
+	int ret;
+
+	ret = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
+							SVC_SOCK_DEFAULTS);
+	if (ret < 0)
+		return ret;
+
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	ret = svc_create_xprt(nfsd_serv, transport, PF_INET6, port,
+							SVC_SOCK_DEFAULTS);
+	if (ret < 0 && ret != -EAFNOSUPPORT)
+		return ret;
+#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
+
+	return 0;
+}
+
+static int nfsd_init_socks(const unsigned short port)
 {
 	int error;
 	if (!list_empty(&nfsd_serv->sv_permsocks))
 		return 0;
 
-	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
-					SVC_SOCK_DEFAULTS);
+	error = nfsd_create_xprt("udp", port);
 	if (error < 0)
 		return error;
 
@@ -290,8 +308,7 @@ static int nfsd_init_socks(int port)
 	if (error < 0)
 		return error;
 
-	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
-					SVC_SOCK_DEFAULTS);
+	error = nfsd_create_xprt("tcp", port);
 	if (error < 0)
 		return error;
 

Re: [PATCH 1/3] SUNRPC: Use rpc_pton() in ip_map_parse()

[J. Bruce Fields]

On Wed, Jan 13, 2010 at 04:10:31PM -0500, Chuck Lever wrote:
> The existing logic in ip_map_parse() can not currently parse
> shorthanded IPv6 addresses (anything with a double colon), nor can
> it parse an IPv6 presentation address with a scope ID.  An
> IPv6-enabled mountd can pass down both.

Why does mountd need to be able to do that?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  net/sunrpc/svcauth_unix.c |   47 +++++++++++++++++++++++++--------------------
>  1 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index d8c0411..97f0e9e 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -15,6 +15,7 @@
>  #include <linux/kernel.h>
>  #define RPCDBG_FACILITY	RPCDBG_AUTH
>  
> +#include <linux/sunrpc/clnt.h>
>  
>  /*
>   * AUTHUNIX and AUTHNULL credentials are both handled here.
> @@ -187,10 +188,13 @@ static int ip_map_parse(struct cache_detail *cd,
>  	 * for scratch: */
>  	char *buf = mesg;
>  	int len;
> -	int b1, b2, b3, b4, b5, b6, b7, b8;
> -	char c;
>  	char class[8];
> -	struct in6_addr addr;
> +	union {
> +		struct sockaddr		sa;
> +		struct sockaddr_in	s4;
> +		struct sockaddr_in6	s6;
> +	} address;
> +	struct sockaddr_in6 sin6;
>  	int err;
>  
>  	struct ip_map *ipmp;
> @@ -209,24 +213,24 @@ static int ip_map_parse(struct cache_detail *cd,
>  	len = qword_get(&mesg, buf, mlen);
>  	if (len <= 0) return -EINVAL;
>  
> -	if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) == 4) {
> -		addr.s6_addr32[0] = 0;
> -		addr.s6_addr32[1] = 0;
> -		addr.s6_addr32[2] = htonl(0xffff);
> -		addr.s6_addr32[3] =
> -			htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> -       } else if (sscanf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x%c",
> -			&b1, &b2, &b3, &b4, &b5, &b6, &b7, &b8, &c) == 8) {
> -		addr.s6_addr16[0] = htons(b1);
> -		addr.s6_addr16[1] = htons(b2);
> -		addr.s6_addr16[2] = htons(b3);
> -		addr.s6_addr16[3] = htons(b4);
> -		addr.s6_addr16[4] = htons(b5);
> -		addr.s6_addr16[5] = htons(b6);
> -		addr.s6_addr16[6] = htons(b7);
> -		addr.s6_addr16[7] = htons(b8);
> -       } else
> +	if (rpc_pton(buf, len, &address.sa, sizeof(address)) == 0)
>  		return -EINVAL;
> +	switch (address.sa.sa_family) {
> +	case AF_INET:
> +		/* Form a mapped IPv4 address in sin6 */
> +		memset(&sin6, 0, sizeof(sin6));
> +		sin6.sin6_family = AF_INET6;
> +		sin6.sin6_addr.s6_addr32[2] = htonl(0xffff);
> +		sin6.sin6_addr.s6_addr32[3] = address.s4.sin_addr.s_addr;
> +		break;
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +	case AF_INET6:
> +		memcpy(&sin6, &address.s6, sizeof(sin6));
> +		break;
> +#endif
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	expiry = get_expiry(&mesg);
>  	if (expiry ==0)
> @@ -243,7 +247,8 @@ static int ip_map_parse(struct cache_detail *cd,
>  	} else
>  		dom = NULL;
>  
> -	ipmp = ip_map_lookup(class, &addr);
> +	/* IPv6 scope IDs are ignored for now */
> +	ipmp = ip_map_lookup(class, &sin6.sin6_addr);
>  	if (ipmp) {
>  		err = ip_map_update(ipmp,
>  			     container_of(dom, struct unix_domain, h),
> 

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[J. Bruce Fields]

On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
> kernel.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>  fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>  2 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 2604c3e..7ebb7a5 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>  	return len;
>  }
>  
> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
> +					 const char *transport,
> +					 const int family,
> +					 const unsigned short port)
> +{
> +	int err;
> +
> +	err = svc_create_xprt(serv, transport, family, port,
> +						SVC_SOCK_ANONYMOUS);
> +
> +	if (err < 0) {
> +		/* Give a reasonable perror msg for bad transport string */
> +		if (err == -ENOENT)
> +			err = -EPROTONOSUPPORT;

I realize you're just moving this code, not writing it, so this is a
separate question, but: it looks like it's svc_create_xprt() itself that
chooses ENOENT for this case.  Is there any reason it shouldn't just use
EPROTONOSUPPORT from the start?

--b.

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[J. Bruce Fields]

On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
> kernel.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>  fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>  2 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 2604c3e..7ebb7a5 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>  	return len;
>  }
>  
> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
> +					 const char *transport,
> +					 const int family,
> +					 const unsigned short port)
> +{
> +	int err;
> +
> +	err = svc_create_xprt(serv, transport, family, port,
> +						SVC_SOCK_ANONYMOUS);
> +
> +	if (err < 0) {
> +		/* Give a reasonable perror msg for bad transport string */
> +		if (err == -ENOENT)
> +			err = -EPROTONOSUPPORT;
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * A transport listener is added by writing it's transport name and
>   * a port number.
> @@ -1000,14 +1020,16 @@ static ssize_t __write_ports_addxprt(char *buf)
>  	if (err != 0)
>  		return err;
>  
> -	err = svc_create_xprt(nfsd_serv, transport,
> -				PF_INET, port, SVC_SOCK_ANONYMOUS);
> -	if (err < 0) {
> -		/* Give a reasonable perror msg for bad transport string */
> -		if (err == -ENOENT)
> -			err = -EPROTONOSUPPORT;
> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET, port);
> +	if (err < 0)
>  		return err;
> -	}
> +
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET6, port);
> +	if (err < 0 && err != -EAFNOSUPPORT)
> +		return err;
> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */

Does every caller of __write_ports_create_xprt() with PF_INET6 end up
being under this ifdef?  Could we instead just move the ifdef inside it
(or inside __svc_xpo_create())?  (As usual I'd rather keep any necessary
hidden away to the extent possible.)

--b.

> +
>  	return 0;
>  }
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 171699e..7af14c1 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -275,14 +275,32 @@ int nfsd_create_serv(void)
>  	return err;
>  }
>  
> -static int nfsd_init_socks(int port)
> +static int nfsd_create_xprt(const char *transport, const unsigned short port)
> +{
> +	int ret;
> +
> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
> +							SVC_SOCK_DEFAULTS);
> +	if (ret < 0)
> +		return ret;
> +
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET6, port,
> +							SVC_SOCK_DEFAULTS);
> +	if (ret < 0 && ret != -EAFNOSUPPORT)
> +		return ret;
> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
> +
> +	return 0;
> +}
> +
> +static int nfsd_init_socks(const unsigned short port)
>  {
>  	int error;
>  	if (!list_empty(&nfsd_serv->sv_permsocks))
>  		return 0;
>  
> -	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
> -					SVC_SOCK_DEFAULTS);
> +	error = nfsd_create_xprt("udp", port);
>  	if (error < 0)
>  		return error;
>  
> @@ -290,8 +308,7 @@ static int nfsd_init_socks(int port)
>  	if (error < 0)
>  		return error;
>  
> -	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
> -					SVC_SOCK_DEFAULTS);
> +	error = nfsd_create_xprt("tcp", port);
>  	if (error < 0)
>  		return error;
>  
> 

Re: [PATCH 1/3] SUNRPC: Use rpc_pton() in ip_map_parse()

[Chuck Lever]

On Jan 15, 2010, at 5:25 PM, J. Bruce Fields wrote:
> On Wed, Jan 13, 2010 at 04:10:31PM -0500, Chuck Lever wrote:
>> The existing logic in ip_map_parse() can not currently parse
>> shorthanded IPv6 addresses (anything with a double colon), nor can
>> it parse an IPv6 presentation address with a scope ID.  An
>> IPv6-enabled mountd can pass down both.
>
> Why does mountd need to be able to do that?

AFAICT, the glibc APIs (like inet_ntop(3) and getnameinfo(3)) provide  
no way to disable shorthanding.

Since we have kernel support for it now, it makes sense to me to allow  
the kernel to accept a broader range of valid presentation address  
formats.

>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> net/sunrpc/svcauth_unix.c |   47 ++++++++++++++++++++++++ 
>> +--------------------
>> 1 files changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
>> index d8c0411..97f0e9e 100644
>> --- a/net/sunrpc/svcauth_unix.c
>> +++ b/net/sunrpc/svcauth_unix.c
>> @@ -15,6 +15,7 @@
>> #include <linux/kernel.h>
>> #define RPCDBG_FACILITY	RPCDBG_AUTH
>>
>> +#include <linux/sunrpc/clnt.h>
>>
>> /*
>>  * AUTHUNIX and AUTHNULL credentials are both handled here.
>> @@ -187,10 +188,13 @@ static int ip_map_parse(struct cache_detail  
>> *cd,
>> 	 * for scratch: */
>> 	char *buf = mesg;
>> 	int len;
>> -	int b1, b2, b3, b4, b5, b6, b7, b8;
>> -	char c;
>> 	char class[8];
>> -	struct in6_addr addr;
>> +	union {
>> +		struct sockaddr		sa;
>> +		struct sockaddr_in	s4;
>> +		struct sockaddr_in6	s6;
>> +	} address;
>> +	struct sockaddr_in6 sin6;
>> 	int err;
>>
>> 	struct ip_map *ipmp;
>> @@ -209,24 +213,24 @@ static int ip_map_parse(struct cache_detail  
>> *cd,
>> 	len = qword_get(&mesg, buf, mlen);
>> 	if (len <= 0) return -EINVAL;
>>
>> -	if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) == 4) {
>> -		addr.s6_addr32[0] = 0;
>> -		addr.s6_addr32[1] = 0;
>> -		addr.s6_addr32[2] = htonl(0xffff);
>> -		addr.s6_addr32[3] =
>> -			htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
>> -       } else if (sscanf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x: 
>> %04x%c",
>> -			&b1, &b2, &b3, &b4, &b5, &b6, &b7, &b8, &c) == 8) {
>> -		addr.s6_addr16[0] = htons(b1);
>> -		addr.s6_addr16[1] = htons(b2);
>> -		addr.s6_addr16[2] = htons(b3);
>> -		addr.s6_addr16[3] = htons(b4);
>> -		addr.s6_addr16[4] = htons(b5);
>> -		addr.s6_addr16[5] = htons(b6);
>> -		addr.s6_addr16[6] = htons(b7);
>> -		addr.s6_addr16[7] = htons(b8);
>> -       } else
>> +	if (rpc_pton(buf, len, &address.sa, sizeof(address)) == 0)
>> 		return -EINVAL;
>> +	switch (address.sa.sa_family) {
>> +	case AF_INET:
>> +		/* Form a mapped IPv4 address in sin6 */
>> +		memset(&sin6, 0, sizeof(sin6));
>> +		sin6.sin6_family = AF_INET6;
>> +		sin6.sin6_addr.s6_addr32[2] = htonl(0xffff);
>> +		sin6.sin6_addr.s6_addr32[3] = address.s4.sin_addr.s_addr;
>> +		break;
>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>> +	case AF_INET6:
>> +		memcpy(&sin6, &address.s6, sizeof(sin6));
>> +		break;
>> +#endif
>> +	default:
>> +		return -EINVAL;
>> +	}
>>
>> 	expiry = get_expiry(&mesg);
>> 	if (expiry ==0)
>> @@ -243,7 +247,8 @@ static int ip_map_parse(struct cache_detail *cd,
>> 	} else
>> 		dom = NULL;
>>
>> -	ipmp = ip_map_lookup(class, &addr);
>> +	/* IPv6 scope IDs are ignored for now */
>> +	ipmp = ip_map_lookup(class, &sin6.sin6_addr);
>> 	if (ipmp) {
>> 		err = ip_map_update(ipmp,
>> 			     container_of(dom, struct unix_domain, h),
>>
> --
> 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

Re: [PATCH 1/3] SUNRPC: Use rpc_pton() in ip_map_parse()

[J. Bruce Fields]

On Fri, Jan 15, 2010 at 05:55:09PM -0500, Chuck Lever wrote:
> On Jan 15, 2010, at 5:25 PM, J. Bruce Fields wrote:
>> On Wed, Jan 13, 2010 at 04:10:31PM -0500, Chuck Lever wrote:
>>> The existing logic in ip_map_parse() can not currently parse
>>> shorthanded IPv6 addresses (anything with a double colon), nor can
>>> it parse an IPv6 presentation address with a scope ID.  An
>>> IPv6-enabled mountd can pass down both.
>>
>> Why does mountd need to be able to do that?
>
> AFAICT, the glibc APIs (like inet_ntop(3) and getnameinfo(3)) provide no 
> way to disable shorthanding.
>
> Since we have kernel support for it now, it makes sense to me to allow  
> the kernel to accept a broader range of valid presentation address  
> formats.

OK.  I can live with that.--b.

>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> net/sunrpc/svcauth_unix.c |   47 ++++++++++++++++++++++++ 
>>> +--------------------
>>> 1 files changed, 26 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
>>> index d8c0411..97f0e9e 100644
>>> --- a/net/sunrpc/svcauth_unix.c
>>> +++ b/net/sunrpc/svcauth_unix.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/kernel.h>
>>> #define RPCDBG_FACILITY	RPCDBG_AUTH
>>>
>>> +#include <linux/sunrpc/clnt.h>
>>>
>>> /*
>>>  * AUTHUNIX and AUTHNULL credentials are both handled here.
>>> @@ -187,10 +188,13 @@ static int ip_map_parse(struct cache_detail  
>>> *cd,
>>> 	 * for scratch: */
>>> 	char *buf = mesg;
>>> 	int len;
>>> -	int b1, b2, b3, b4, b5, b6, b7, b8;
>>> -	char c;
>>> 	char class[8];
>>> -	struct in6_addr addr;
>>> +	union {
>>> +		struct sockaddr		sa;
>>> +		struct sockaddr_in	s4;
>>> +		struct sockaddr_in6	s6;
>>> +	} address;
>>> +	struct sockaddr_in6 sin6;
>>> 	int err;
>>>
>>> 	struct ip_map *ipmp;
>>> @@ -209,24 +213,24 @@ static int ip_map_parse(struct cache_detail  
>>> *cd,
>>> 	len = qword_get(&mesg, buf, mlen);
>>> 	if (len <= 0) return -EINVAL;
>>>
>>> -	if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) == 4) {
>>> -		addr.s6_addr32[0] = 0;
>>> -		addr.s6_addr32[1] = 0;
>>> -		addr.s6_addr32[2] = htonl(0xffff);
>>> -		addr.s6_addr32[3] =
>>> -			htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
>>> -       } else if (sscanf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x: 
>>> %04x%c",
>>> -			&b1, &b2, &b3, &b4, &b5, &b6, &b7, &b8, &c) == 8) {
>>> -		addr.s6_addr16[0] = htons(b1);
>>> -		addr.s6_addr16[1] = htons(b2);
>>> -		addr.s6_addr16[2] = htons(b3);
>>> -		addr.s6_addr16[3] = htons(b4);
>>> -		addr.s6_addr16[4] = htons(b5);
>>> -		addr.s6_addr16[5] = htons(b6);
>>> -		addr.s6_addr16[6] = htons(b7);
>>> -		addr.s6_addr16[7] = htons(b8);
>>> -       } else
>>> +	if (rpc_pton(buf, len, &address.sa, sizeof(address)) == 0)
>>> 		return -EINVAL;
>>> +	switch (address.sa.sa_family) {
>>> +	case AF_INET:
>>> +		/* Form a mapped IPv4 address in sin6 */
>>> +		memset(&sin6, 0, sizeof(sin6));
>>> +		sin6.sin6_family = AF_INET6;
>>> +		sin6.sin6_addr.s6_addr32[2] = htonl(0xffff);
>>> +		sin6.sin6_addr.s6_addr32[3] = address.s4.sin_addr.s_addr;
>>> +		break;
>>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>> +	case AF_INET6:
>>> +		memcpy(&sin6, &address.s6, sizeof(sin6));
>>> +		break;
>>> +#endif
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>>
>>> 	expiry = get_expiry(&mesg);
>>> 	if (expiry ==0)
>>> @@ -243,7 +247,8 @@ static int ip_map_parse(struct cache_detail *cd,
>>> 	} else
>>> 		dom = NULL;
>>>
>>> -	ipmp = ip_map_lookup(class, &addr);
>>> +	/* IPv6 scope IDs are ignored for now */
>>> +	ipmp = ip_map_lookup(class, &sin6.sin6_addr);
>>> 	if (ipmp) {
>>> 		err = ip_map_update(ipmp,
>>> 			     container_of(dom, struct unix_domain, h),
>>>
>> --
>> 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
>
>
>

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[Chuck Lever]

On Jan 15, 2010, at 5:35 PM, J. Bruce Fields wrote:

> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>> kernel.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 2604c3e..7ebb7a5 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>> 	return len;
>> }
>>
>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>> +					 const char *transport,
>> +					 const int family,
>> +					 const unsigned short port)
>> +{
>> +	int err;
>> +
>> +	err = svc_create_xprt(serv, transport, family, port,
>> +						SVC_SOCK_ANONYMOUS);
>> +
>> +	if (err < 0) {
>> +		/* Give a reasonable perror msg for bad transport string */
>> +		if (err == -ENOENT)
>> +			err = -EPROTONOSUPPORT;
>
> I realize you're just moving this code, not writing it, so this is a
> separate question, but: it looks like it's svc_create_xprt() itself  
> that
> chooses ENOENT for this case.  Is there any reason it shouldn't just  
> use
> EPROTONOSUPPORT from the start?

I guess this little transformation is for the benefit of rpc.nfsd?  I  
don't know of a reason offhand why you couldn't change  
svc_create_xprt().  I don't immediately see any of svc_create_xprt's  
callers that might depend on getting ENOENT, but I didn't look very  
hard.

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

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[Chuck Lever]

On Jan 15, 2010, at 5:44 PM, J. Bruce Fields wrote:

> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>> kernel.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 2604c3e..7ebb7a5 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>> 	return len;
>> }
>>
>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>> +					 const char *transport,
>> +					 const int family,
>> +					 const unsigned short port)
>> +{
>> +	int err;
>> +
>> +	err = svc_create_xprt(serv, transport, family, port,
>> +						SVC_SOCK_ANONYMOUS);
>> +
>> +	if (err < 0) {
>> +		/* Give a reasonable perror msg for bad transport string */
>> +		if (err == -ENOENT)
>> +			err = -EPROTONOSUPPORT;
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> /*
>>  * A transport listener is added by writing it's transport name and
>>  * a port number.
>> @@ -1000,14 +1020,16 @@ static ssize_t __write_ports_addxprt(char  
>> *buf)
>> 	if (err != 0)
>> 		return err;
>>
>> -	err = svc_create_xprt(nfsd_serv, transport,
>> -				PF_INET, port, SVC_SOCK_ANONYMOUS);
>> -	if (err < 0) {
>> -		/* Give a reasonable perror msg for bad transport string */
>> -		if (err == -ENOENT)
>> -			err = -EPROTONOSUPPORT;
>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET,  
>> port);
>> +	if (err < 0)
>> 		return err;
>> -	}
>> +
>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET6,  
>> port);
>> +	if (err < 0 && err != -EAFNOSUPPORT)
>> +		return err;
>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>
> Does every caller of __write_ports_create_xprt() with PF_INET6 end up
> being under this ifdef?  Could we instead just move the ifdef inside  
> it
> (or inside __svc_xpo_create())?  (As usual I'd rather keep any  
> necessary
> hidden away to the extent possible.)

The only two call sites are right here:  one with PF_INET and one with  
PF_INET6.

When IPv6 support is not enabled, we want only the first call (with  
PF_INET).  When IPv6 support is enabled, we want two calls; one with  
PF_INET and then one with PF_INET6.

This particular interface is for creating a named transport.  We don't  
name our IPv6 transports separately from IPv4 transports, so this is a  
simple way to get IPv6 enabled listeners.  It's actually been a while  
since I sat down and thought through this.

> --b.
>
>> +
>> 	return 0;
>> }
>>
>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>> index 171699e..7af14c1 100644
>> --- a/fs/nfsd/nfssvc.c
>> +++ b/fs/nfsd/nfssvc.c
>> @@ -275,14 +275,32 @@ int nfsd_create_serv(void)
>> 	return err;
>> }
>>
>> -static int nfsd_init_socks(int port)
>> +static int nfsd_create_xprt(const char *transport, const unsigned  
>> short port)
>> +{
>> +	int ret;
>> +
>> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
>> +							SVC_SOCK_DEFAULTS);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET6, port,
>> +							SVC_SOCK_DEFAULTS);
>> +	if (ret < 0 && ret != -EAFNOSUPPORT)
>> +		return ret;
>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>> +
>> +	return 0;
>> +}
>> +
>> +static int nfsd_init_socks(const unsigned short port)
>> {
>> 	int error;
>> 	if (!list_empty(&nfsd_serv->sv_permsocks))
>> 		return 0;
>>
>> -	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
>> -					SVC_SOCK_DEFAULTS);
>> +	error = nfsd_create_xprt("udp", port);
>> 	if (error < 0)
>> 		return error;
>>
>> @@ -290,8 +308,7 @@ static int nfsd_init_socks(int port)
>> 	if (error < 0)
>> 		return error;
>>
>> -	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
>> -					SVC_SOCK_DEFAULTS);
>> +	error = nfsd_create_xprt("tcp", port);
>> 	if (error < 0)
>> 		return error;
>>
>>

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

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[J. Bruce Fields]

On Fri, Jan 15, 2010 at 06:17:38PM -0500, Chuck Lever wrote:
>
> On Jan 15, 2010, at 5:44 PM, J. Bruce Fields wrote:
>
>> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>>> kernel.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>> index 2604c3e..7ebb7a5 100644
>>> --- a/fs/nfsd/nfsctl.c
>>> +++ b/fs/nfsd/nfsctl.c
>>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>>> 	return len;
>>> }
>>>
>>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>>> +					 const char *transport,
>>> +					 const int family,
>>> +					 const unsigned short port)
>>> +{
>>> +	int err;
>>> +
>>> +	err = svc_create_xprt(serv, transport, family, port,
>>> +						SVC_SOCK_ANONYMOUS);
>>> +
>>> +	if (err < 0) {
>>> +		/* Give a reasonable perror msg for bad transport string */
>>> +		if (err == -ENOENT)
>>> +			err = -EPROTONOSUPPORT;
>>> +		return err;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> /*
>>>  * A transport listener is added by writing it's transport name and
>>>  * a port number.
>>> @@ -1000,14 +1020,16 @@ static ssize_t __write_ports_addxprt(char  
>>> *buf)
>>> 	if (err != 0)
>>> 		return err;
>>>
>>> -	err = svc_create_xprt(nfsd_serv, transport,
>>> -				PF_INET, port, SVC_SOCK_ANONYMOUS);
>>> -	if (err < 0) {
>>> -		/* Give a reasonable perror msg for bad transport string */
>>> -		if (err == -ENOENT)
>>> -			err = -EPROTONOSUPPORT;
>>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET,  
>>> port);
>>> +	if (err < 0)
>>> 		return err;
>>> -	}
>>> +
>>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET6,  
>>> port);
>>> +	if (err < 0 && err != -EAFNOSUPPORT)
>>> +		return err;
>>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>>
>> Does every caller of __write_ports_create_xprt() with PF_INET6 end up
>> being under this ifdef?  Could we instead just move the ifdef inside  
>> it
>> (or inside __svc_xpo_create())?  (As usual I'd rather keep any  
>> necessary
>> hidden away to the extent possible.)
>
> The only two call sites are right here:  one with PF_INET and one with  
> PF_INET6.

OK, but svc_create_xprt has a few more.

>
> When IPv6 support is not enabled, we want only the first call (with  
> PF_INET).  When IPv6 support is enabled, we want two calls; one with  
> PF_INET and then one with PF_INET6.

Right, I understand, but we could equally well do that by calling
svc_create_xprt twice, and depending on it (or __svc_xpo_create(), or
someone else there) to return -EAFNOSUPPORT in the !defined(CONFIG_IPV6)
&& !defined(CONFIG_IPV6_MODULE case.  Right?

No change in behavior, I just want the #ifdef's buried a little further
if it's possible.

--b.

>
> This particular interface is for creating a named transport.  We don't  
> name our IPv6 transports separately from IPv4 transports, so this is a  
> simple way to get IPv6 enabled listeners.  It's actually been a while  
> since I sat down and thought through this.
>
>> --b.
>>
>>> +
>>> 	return 0;
>>> }
>>>
>>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>>> index 171699e..7af14c1 100644
>>> --- a/fs/nfsd/nfssvc.c
>>> +++ b/fs/nfsd/nfssvc.c
>>> @@ -275,14 +275,32 @@ int nfsd_create_serv(void)
>>> 	return err;
>>> }
>>>
>>> -static int nfsd_init_socks(int port)
>>> +static int nfsd_create_xprt(const char *transport, const unsigned  
>>> short port)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
>>> +							SVC_SOCK_DEFAULTS);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET6, port,
>>> +							SVC_SOCK_DEFAULTS);
>>> +	if (ret < 0 && ret != -EAFNOSUPPORT)
>>> +		return ret;
>>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int nfsd_init_socks(const unsigned short port)
>>> {
>>> 	int error;
>>> 	if (!list_empty(&nfsd_serv->sv_permsocks))
>>> 		return 0;
>>>
>>> -	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
>>> -					SVC_SOCK_DEFAULTS);
>>> +	error = nfsd_create_xprt("udp", port);
>>> 	if (error < 0)
>>> 		return error;
>>>
>>> @@ -290,8 +308,7 @@ static int nfsd_init_socks(int port)
>>> 	if (error < 0)
>>> 		return error;
>>>
>>> -	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
>>> -					SVC_SOCK_DEFAULTS);
>>> +	error = nfsd_create_xprt("tcp", port);
>>> 	if (error < 0)
>>> 		return error;
>>>
>>>
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[J. Bruce Fields]

On Fri, Jan 15, 2010 at 06:09:17PM -0500, Chuck Lever wrote:
>
> On Jan 15, 2010, at 5:35 PM, J. Bruce Fields wrote:
>
>> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>>> kernel.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>> index 2604c3e..7ebb7a5 100644
>>> --- a/fs/nfsd/nfsctl.c
>>> +++ b/fs/nfsd/nfsctl.c
>>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>>> 	return len;
>>> }
>>>
>>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>>> +					 const char *transport,
>>> +					 const int family,
>>> +					 const unsigned short port)
>>> +{
>>> +	int err;
>>> +
>>> +	err = svc_create_xprt(serv, transport, family, port,
>>> +						SVC_SOCK_ANONYMOUS);
>>> +
>>> +	if (err < 0) {
>>> +		/* Give a reasonable perror msg for bad transport string */
>>> +		if (err == -ENOENT)
>>> +			err = -EPROTONOSUPPORT;
>>
>> I realize you're just moving this code, not writing it, so this is a
>> separate question, but: it looks like it's svc_create_xprt() itself  
>> that
>> chooses ENOENT for this case.  Is there any reason it shouldn't just  
>> use
>> EPROTONOSUPPORT from the start?
>
> I guess this little transformation is for the benefit of rpc.nfsd?  I  
> don't know of a reason offhand why you couldn't change  
> svc_create_xprt().  I don't immediately see any of svc_create_xprt's  
> callers that might depend on getting ENOENT, but I didn't look very  
> hard.

Could I talk you into writing a patch that does that, as long as you're
touching this code?

--b.

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[Chuck Lever]

On Jan 21, 2010, at 5:07 PM, J. Bruce Fields wrote:

> On Fri, Jan 15, 2010 at 06:17:38PM -0500, Chuck Lever wrote:
>>
>> On Jan 15, 2010, at 5:44 PM, J. Bruce Fields wrote:
>>
>>> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>>>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>>>> kernel.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>>>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>>>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>> index 2604c3e..7ebb7a5 100644
>>>> --- a/fs/nfsd/nfsctl.c
>>>> +++ b/fs/nfsd/nfsctl.c
>>>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>>>> 	return len;
>>>> }
>>>>
>>>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>>>> +					 const char *transport,
>>>> +					 const int family,
>>>> +					 const unsigned short port)
>>>> +{
>>>> +	int err;
>>>> +
>>>> +	err = svc_create_xprt(serv, transport, family, port,
>>>> +						SVC_SOCK_ANONYMOUS);
>>>> +
>>>> +	if (err < 0) {
>>>> +		/* Give a reasonable perror msg for bad transport string */
>>>> +		if (err == -ENOENT)
>>>> +			err = -EPROTONOSUPPORT;
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> /*
>>>> * A transport listener is added by writing it's transport name and
>>>> * a port number.
>>>> @@ -1000,14 +1020,16 @@ static ssize_t __write_ports_addxprt(char
>>>> *buf)
>>>> 	if (err != 0)
>>>> 		return err;
>>>>
>>>> -	err = svc_create_xprt(nfsd_serv, transport,
>>>> -				PF_INET, port, SVC_SOCK_ANONYMOUS);
>>>> -	if (err < 0) {
>>>> -		/* Give a reasonable perror msg for bad transport string */
>>>> -		if (err == -ENOENT)
>>>> -			err = -EPROTONOSUPPORT;
>>>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET,
>>>> port);
>>>> +	if (err < 0)
>>>> 		return err;
>>>> -	}
>>>> +
>>>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET6,
>>>> port);
>>>> +	if (err < 0 && err != -EAFNOSUPPORT)
>>>> +		return err;
>>>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>>>
>>> Does every caller of __write_ports_create_xprt() with PF_INET6 end  
>>> up
>>> being under this ifdef?  Could we instead just move the ifdef inside
>>> it
>>> (or inside __svc_xpo_create())?  (As usual I'd rather keep any
>>> necessary
>>> hidden away to the extent possible.)
>>
>> The only two call sites are right here:  one with PF_INET and one  
>> with
>> PF_INET6.
>
> OK, but svc_create_xprt has a few more.
>
>>
>> When IPv6 support is not enabled, we want only the first call (with
>> PF_INET).  When IPv6 support is enabled, we want two calls; one with
>> PF_INET and then one with PF_INET6.
>
> Right, I understand, but we could equally well do that by calling
> svc_create_xprt twice, and depending on it (or __svc_xpo_create(), or
> someone else there) to return -EAFNOSUPPORT in the ! 
> defined(CONFIG_IPV6)
> && !defined(CONFIG_IPV6_MODULE case.  Right?
>
> No change in behavior, I just want the #ifdef's buried a little  
> further
> if it's possible.

Yeah, OK.  That kind of change would effect other call sites, so I  
would need to break this into a couple of smaller patches.

>
> --b.
>
>>
>> This particular interface is for creating a named transport.  We  
>> don't
>> name our IPv6 transports separately from IPv4 transports, so this  
>> is a
>> simple way to get IPv6 enabled listeners.  It's actually been a while
>> since I sat down and thought through this.
>>
>>> --b.
>>>
>>>> +
>>>> 	return 0;
>>>> }
>>>>
>>>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>>>> index 171699e..7af14c1 100644
>>>> --- a/fs/nfsd/nfssvc.c
>>>> +++ b/fs/nfsd/nfssvc.c
>>>> @@ -275,14 +275,32 @@ int nfsd_create_serv(void)
>>>> 	return err;
>>>> }
>>>>
>>>> -static int nfsd_init_socks(int port)
>>>> +static int nfsd_create_xprt(const char *transport, const unsigned
>>>> short port)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
>>>> +							SVC_SOCK_DEFAULTS);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>>> +	ret = svc_create_xprt(nfsd_serv, transport, PF_INET6, port,
>>>> +							SVC_SOCK_DEFAULTS);
>>>> +	if (ret < 0 && ret != -EAFNOSUPPORT)
>>>> +		return ret;
>>>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int nfsd_init_socks(const unsigned short port)
>>>> {
>>>> 	int error;
>>>> 	if (!list_empty(&nfsd_serv->sv_permsocks))
>>>> 		return 0;
>>>>
>>>> -	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
>>>> -					SVC_SOCK_DEFAULTS);
>>>> +	error = nfsd_create_xprt("udp", port);
>>>> 	if (error < 0)
>>>> 		return error;
>>>>
>>>> @@ -290,8 +308,7 @@ static int nfsd_init_socks(int port)
>>>> 	if (error < 0)
>>>> 		return error;
>>>>
>>>> -	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
>>>> -					SVC_SOCK_DEFAULTS);
>>>> +	error = nfsd_create_xprt("tcp", port);
>>>> 	if (error < 0)
>>>> 		return error;
>>>>
>>>>
>>
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>>
>>
>>

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

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[Chuck Lever]

On Jan 21, 2010, at 5:52 PM, J. Bruce Fields wrote:

> On Fri, Jan 15, 2010 at 06:09:17PM -0500, Chuck Lever wrote:
>>
>> On Jan 15, 2010, at 5:35 PM, J. Bruce Fields wrote:
>>
>>> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>>>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>>>> kernel.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>>>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>>>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>> index 2604c3e..7ebb7a5 100644
>>>> --- a/fs/nfsd/nfsctl.c
>>>> +++ b/fs/nfsd/nfsctl.c
>>>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>>>> 	return len;
>>>> }
>>>>
>>>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>>>> +					 const char *transport,
>>>> +					 const int family,
>>>> +					 const unsigned short port)
>>>> +{
>>>> +	int err;
>>>> +
>>>> +	err = svc_create_xprt(serv, transport, family, port,
>>>> +						SVC_SOCK_ANONYMOUS);
>>>> +
>>>> +	if (err < 0) {
>>>> +		/* Give a reasonable perror msg for bad transport string */
>>>> +		if (err == -ENOENT)
>>>> +			err = -EPROTONOSUPPORT;
>>>
>>> I realize you're just moving this code, not writing it, so this is a
>>> separate question, but: it looks like it's svc_create_xprt() itself
>>> that
>>> chooses ENOENT for this case.  Is there any reason it shouldn't just
>>> use
>>> EPROTONOSUPPORT from the start?
>>
>> I guess this little transformation is for the benefit of rpc.nfsd?  I
>> don't know of a reason offhand why you couldn't change
>> svc_create_xprt().  I don't immediately see any of svc_create_xprt's
>> callers that might depend on getting ENOENT, but I didn't look very
>> hard.
>
> Could I talk you into writing a patch that does that, as long as  
> you're
> touching this code?

Since we have a little time before 2.6.34, I'll look into it.

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

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[J. Bruce Fields]

On Fri, Jan 22, 2010 at 12:07:42PM -0500, Chuck Lever wrote:
>
> On Jan 21, 2010, at 5:07 PM, J. Bruce Fields wrote:
>
>> On Fri, Jan 15, 2010 at 06:17:38PM -0500, Chuck Lever wrote:
>>>
>>> On Jan 15, 2010, at 5:44 PM, J. Bruce Fields wrote:
>>>
>>>> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>>>>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>>>>> kernel.
>>>>>
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>
>>>>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>>>>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>>>>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>>> index 2604c3e..7ebb7a5 100644
>>>>> --- a/fs/nfsd/nfsctl.c
>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>>>>> 	return len;
>>>>> }
>>>>>
>>>>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>>>>> +					 const char *transport,
>>>>> +					 const int family,
>>>>> +					 const unsigned short port)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	err = svc_create_xprt(serv, transport, family, port,
>>>>> +						SVC_SOCK_ANONYMOUS);
>>>>> +
>>>>> +	if (err < 0) {
>>>>> +		/* Give a reasonable perror msg for bad transport string */
>>>>> +		if (err == -ENOENT)
>>>>> +			err = -EPROTONOSUPPORT;
>>>>> +		return err;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * A transport listener is added by writing it's transport name and
>>>>> * a port number.
>>>>> @@ -1000,14 +1020,16 @@ static ssize_t __write_ports_addxprt(char
>>>>> *buf)
>>>>> 	if (err != 0)
>>>>> 		return err;
>>>>>
>>>>> -	err = svc_create_xprt(nfsd_serv, transport,
>>>>> -				PF_INET, port, SVC_SOCK_ANONYMOUS);
>>>>> -	if (err < 0) {
>>>>> -		/* Give a reasonable perror msg for bad transport string */
>>>>> -		if (err == -ENOENT)
>>>>> -			err = -EPROTONOSUPPORT;
>>>>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET,
>>>>> port);
>>>>> +	if (err < 0)
>>>>> 		return err;
>>>>> -	}
>>>>> +
>>>>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>>>> +	err = __write_ports_create_xprt(nfsd_serv, transport, PF_INET6,
>>>>> port);
>>>>> +	if (err < 0 && err != -EAFNOSUPPORT)
>>>>> +		return err;
>>>>> +#endif	/* CONFIG_IPV6 || CONFIG_IPV6_MODULE */
>>>>
>>>> Does every caller of __write_ports_create_xprt() with PF_INET6 end  
>>>> up
>>>> being under this ifdef?  Could we instead just move the ifdef inside
>>>> it
>>>> (or inside __svc_xpo_create())?  (As usual I'd rather keep any
>>>> necessary
>>>> hidden away to the extent possible.)
>>>
>>> The only two call sites are right here:  one with PF_INET and one  
>>> with
>>> PF_INET6.
>>
>> OK, but svc_create_xprt has a few more.
>>
>>>
>>> When IPv6 support is not enabled, we want only the first call (with
>>> PF_INET).  When IPv6 support is enabled, we want two calls; one with
>>> PF_INET and then one with PF_INET6.
>>
>> Right, I understand, but we could equally well do that by calling
>> svc_create_xprt twice, and depending on it (or __svc_xpo_create(), or
>> someone else there) to return -EAFNOSUPPORT in the ! 
>> defined(CONFIG_IPV6)
>> && !defined(CONFIG_IPV6_MODULE case.  Right?
>>
>> No change in behavior, I just want the #ifdef's buried a little  
>> further
>> if it's possible.
>
> Yeah, OK.  That kind of change would effect other call sites, so I would 
> need to break this into a couple of smaller patches.

Yes, that sounds right.

--b.

Re: [PATCH 3/3] NFSD: Enable NFS server use of PF_INET6

[J. Bruce Fields]

On Fri, Jan 22, 2010 at 12:10:59PM -0500, Chuck Lever wrote:
>
> On Jan 21, 2010, at 5:52 PM, J. Bruce Fields wrote:
>
>> On Fri, Jan 15, 2010 at 06:09:17PM -0500, Chuck Lever wrote:
>>>
>>> On Jan 15, 2010, at 5:35 PM, J. Bruce Fields wrote:
>>>
>>>> On Wed, Jan 13, 2010 at 04:10:48PM -0500, Chuck Lever wrote:
>>>>> Try to use an PF_INET6 listener for NFSD if IPv6 is enabled in the
>>>>> kernel.
>>>>>
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>
>>>>> fs/nfsd/nfsctl.c |   36 +++++++++++++++++++++++++++++-------
>>>>> fs/nfsd/nfssvc.c |   27 ++++++++++++++++++++++-----
>>>>> 2 files changed, 51 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>>> index 2604c3e..7ebb7a5 100644
>>>>> --- a/fs/nfsd/nfsctl.c
>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>> @@ -981,6 +981,26 @@ static ssize_t __write_ports_delfd(char *buf)
>>>>> 	return len;
>>>>> }
>>>>>
>>>>> +static ssize_t __write_ports_create_xprt(struct svc_serv *serv,
>>>>> +					 const char *transport,
>>>>> +					 const int family,
>>>>> +					 const unsigned short port)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	err = svc_create_xprt(serv, transport, family, port,
>>>>> +						SVC_SOCK_ANONYMOUS);
>>>>> +
>>>>> +	if (err < 0) {
>>>>> +		/* Give a reasonable perror msg for bad transport string */
>>>>> +		if (err == -ENOENT)
>>>>> +			err = -EPROTONOSUPPORT;
>>>>
>>>> I realize you're just moving this code, not writing it, so this is a
>>>> separate question, but: it looks like it's svc_create_xprt() itself
>>>> that
>>>> chooses ENOENT for this case.  Is there any reason it shouldn't just
>>>> use
>>>> EPROTONOSUPPORT from the start?
>>>
>>> I guess this little transformation is for the benefit of rpc.nfsd?  I
>>> don't know of a reason offhand why you couldn't change
>>> svc_create_xprt().  I don't immediately see any of svc_create_xprt's
>>> callers that might depend on getting ENOENT, but I didn't look very
>>> hard.
>>
>> Could I talk you into writing a patch that does that, as long as  
>> you're
>> touching this code?
>
> Since we have a little time before 2.6.34, I'll look into it.

Thanks, Chuck.

--b.