[PATCH 0/6] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #2)

[PATCH 0/6] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #2)

[Jeff Layton]

This patchset is a second attempt at adding support for IPv6 to
rpc.gssd. The main changes from the first set are some minor fixes and
changes suggested by Chuck Lever. This set is also based on mainline
nfs-utils since the requisite patches for it are now there.

In order to do this, we first have to convert rpc.gssd to use TI-RPC.
The first 5 patches do this and abstract out some of the code to make
this easier. Once that support is in place, the last patch in the series
actually adds the (fairly minimal) changes to add IPv6 support.

The series should be fully bisectable, but I've only really tested the
end result for anything other than to see if it compiles. With these
patches I've been able to mount an OpenSolaris server using
NFSv3/4 + IPv6 + krb5 auth

Specific changes from the last set:
- cleaned up the usage of sockaddr and sockaddr_storage and pointers to
  them
- moved the sockaddr_storage to be part of the clnt_info instead of a
  separate allocation
- use a C99 initializer for ai_hints instead of memset()
- properly handle a return of EAI_SYSTEM from getaddrinfo()

Comments welcome.

Jeff Layton (6):
  nfs-utils: make getnameinfo() required for --enable-gss
  nfs-utils: store the address given in the upcall for later use
  nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need
    port
  nfs-utils: split out gssd rpc client creation into separate function
  nfs-utils: when TIRPC is enabled, use new API to create RPC client
  nfs-utils: add IPv6 code to gssd

 configure.ac           |    2 +
 utils/gssd/gssd.h      |    2 +-
 utils/gssd/gssd_proc.c |  398 +++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 309 insertions(+), 93 deletions(-)

[PATCH 1/6] nfs-utils: make getnameinfo() required for --enable-gss

[Jeff Layton]

Systems that are so old that they don't have getnameinfo() in glibc are
probably also running kernels that are so old that they don't support
gssapi upcalls anyway.

Make --enable-gss dependent on the presence of the getnameinfo()
function.  This allows us to reduce some conditional compilation.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 configure.ac |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index e34b7e2..00aee35 100644
--- a/configure.ac
+++ b/configure.ac
@@ -207,6 +207,8 @@ if test "$enable_nfsv4" = yes; then
   dnl but we need to make sure we get the right version
   if test "$enable_gss" = yes; then
     AC_RPCSEC_VERSION
+    AC_CHECK_FUNC([getnameinfo], ,
+		  [AC_MSG_ERROR([Function 'getnameinfo' not found.])])
   fi
 fi
 
-- 
1.6.0.6

[PATCH 2/6] nfs-utils: store the address given in the upcall for later use

[Jeff Layton]

The current upcall could be more efficient. We first convert the address
to a hostname, and then later when we set up the RPC client, we do a
hostname lookup to convert it back to an address.

Begin to change this by keeping the address in the clnt_info that we get
out of the upcall. Since a sockaddr has a port field, we can also
eliminate the port from the clnt_info.

Finally, switch to getnameinfo() instead of gethostbyaddr(). We'll need
to use that call anyway when we add support for IPv6.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd.h      |    2 +-
 utils/gssd/gssd_proc.c |   90 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 082039a..3c52f46 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -83,7 +83,7 @@ struct clnt_info {
 	int			krb5_poll_index;
 	int			spkm3_fd;
 	int			spkm3_poll_index;
-	int			port;
+	struct sockaddr_storage addr;
 };
 
 void init_client_list(void);
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 509946e..f5f3a0f 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -102,10 +102,70 @@ struct pollfd * pollarray;
 
 int pollsize;  /* the size of pollaray (in pollfd's) */
 
+/*
+ * convert an address string to a sockaddr_storage struct. Returns true
+ * on success and false on failure.
+ */
+static int
+addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const int port)
+{
+	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+
+	if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
+		s4->sin_family = AF_INET;
+		s4->sin_port = htons(port);
+	} else {
+		printerr(0, "ERROR: unable to convert %s to address\n", addr);
+		return 0;
+	}
+
+	return 1;
+}
+
+/*
+ * convert a sockaddr to a hostname
+ */
+static char *
+sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
+{
+#define MAX_HOSTNAME_LEN 127
+	char 			*hostname;
+	socklen_t		addrlen;
+	int			err;
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		addrlen = sizeof(struct sockaddr_in);
+		break;
+	default:
+		printerr(0, "ERROR: unrecognized addr family %d\n",
+			 sa->sa_family);
+		return NULL;
+	}
+
+	hostname = calloc(1, MAX_HOSTNAME_LEN + 1);
+	if (!hostname) {
+		printerr(0, "ERROR: unable to allocate hostname string\n");
+		return NULL;
+	}
+
+	err = getnameinfo(sa, addrlen, hostname, MAX_HOSTNAME_LEN, NULL,
+			  0, NI_NAMEREQD);
+	if (err) {
+		free(hostname);
+		hostname = NULL;
+		printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
+			 addr, gai_strerror(err));
+	}
+
+	return hostname;
+}
+
 /* XXX buffer problems: */
 static int
 read_service_info(char *info_file_name, char **servicename, char **servername,
-		  int *prog, int *vers, char **protocol, int *port) {
+		  int *prog, int *vers, char **protocol,
+		  struct sockaddr *addr) {
 #define INFOBUFLEN 256
 	char		buf[INFOBUFLEN + 1];
 	static char	dummy[128];
@@ -117,10 +177,9 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
 	char		protoname[16];
 	char		cb_port[128];
 	char		*p;
-	in_addr_t	inaddr;
 	int		fd = -1;
-	struct hostent	*ent = NULL;
 	int		numfields;
+	int		port = 0;
 
 	*servicename = *servername = *protocol = NULL;
 
@@ -160,21 +219,19 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
 	if((*prog != 100003) || ((*vers != 2) && (*vers != 3) && (*vers != 4)))
 		goto fail;
 
-	/* create service name */
-	inaddr = inet_addr(address);
-	if (!(ent = gethostbyaddr(&inaddr, sizeof(inaddr), AF_INET))) {
-		printerr(0, "ERROR: can't resolve server %s name\n", address);
+	if (cb_port[0] != '\0')
+		port = atoi(cb_port);
+	if (!addrstr_to_sockaddr(addr, address, port))
 		goto fail;
-	}
-	if (!(*servername = calloc(strlen(ent->h_name) + 1, 1)))
+
+	*servername = sockaddr_to_hostname(addr, address);
+	if (*servername == NULL)
 		goto fail;
-	memcpy(*servername, ent->h_name, strlen(ent->h_name));
-	snprintf(buf, INFOBUFLEN, "%s@%s", service, ent->h_name);
+
+	snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
 	if (!(*servicename = calloc(strlen(buf) + 1, 1)))
 		goto fail;
 	memcpy(*servicename, buf, strlen(buf));
-	if (cb_port[0] != '\0')
-		*port = atoi(cb_port);
 
 	if (!(*protocol = strdup(protoname)))
 		goto fail;
@@ -251,7 +308,7 @@ process_clnt_dir_files(struct clnt_info * clp)
 	if ((clp->servicename == NULL) &&
 	     read_service_info(info_file_name, &clp->servicename,
 				&clp->servername, &clp->prog, &clp->vers,
-				&clp->protocol, &clp->port))
+				&clp->protocol, (struct sockaddr *) &clp->addr))
 		return -1;
 	return 0;
 }
@@ -599,8 +656,9 @@ int create_auth_rpc_client(struct clnt_info *clp,
 			 clp->servername, uid);
 		goto out_fail;
 	}
-	if (clp->port)
-		((struct sockaddr_in *)a->ai_addr)->sin_port = htons(clp->port);
+	if (((struct sockaddr_in) clp->addr).sin_port != 0)
+		((struct sockaddr_in *) a->ai_addr)->sin_port =
+				((struct sockaddr_in) clp->addr).sin_port;
 	if (a->ai_protocol == IPPROTO_TCP) {
 		if ((rpc_clnt = clnttcp_create(
 					(struct sockaddr_in *) a->ai_addr,
-- 
1.6.0.6

Re: [PATCH 2/6] nfs-utils: store the address given in the upcall for later use

[Chuck Lever]

On Apr 1, 2009, at 10:24 AM, Jeff Layton wrote:

> The current upcall could be more efficient. We first convert the  
> address
> to a hostname, and then later when we set up the RPC client, we do a
> hostname lookup to convert it back to an address.
>
> Begin to change this by keeping the address in the clnt_info that we  
> get
> out of the upcall. Since a sockaddr has a port field, we can also
> eliminate the port from the clnt_info.
>
> Finally, switch to getnameinfo() instead of gethostbyaddr(). We'll  
> need
> to use that call anyway when we add support for IPv6.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> utils/gssd/gssd.h      |    2 +-
> utils/gssd/gssd_proc.c |   90 ++++++++++++++++++++++++++++++++++++++ 
> +--------
> 2 files changed, 75 insertions(+), 17 deletions(-)
>
> diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
> index 082039a..3c52f46 100644
> --- a/utils/gssd/gssd.h
> +++ b/utils/gssd/gssd.h
> @@ -83,7 +83,7 @@ struct clnt_info {
> 	int			krb5_poll_index;
> 	int			spkm3_fd;
> 	int			spkm3_poll_index;
> -	int			port;
> +	struct sockaddr_storage addr;
> };
>
> void init_client_list(void);
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 509946e..f5f3a0f 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -102,10 +102,70 @@ struct pollfd * pollarray;
>
> int pollsize;  /* the size of pollaray (in pollfd's) */
>
> +/*
> + * convert an address string to a sockaddr_storage struct. Returns  
> true
> + * on success and false on failure.
> + */
> +static int
> +addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const  
> int port)
> +{
> +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
> +
> +	if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
> +		s4->sin_family = AF_INET;
> +		s4->sin_port = htons(port);
> +	} else {
> +		printerr(0, "ERROR: unable to convert %s to address\n", addr);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +/*
> + * convert a sockaddr to a hostname
> + */
> +static char *
> +sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
> +{
> +#define MAX_HOSTNAME_LEN 127

netdb.h provides NI_MAXHOST.  Since getnameinfo(3) is now required,  
you can use that.  If this length maximum is required by the kernel  
gss upcall API, then this should be defined in a more public place, I  
think, as part of the interface specification.

> +	char 			*hostname;
> +	socklen_t		addrlen;
> +	int			err;
> +
> +	switch (sa->sa_family) {
> +	case AF_INET:
> +		addrlen = sizeof(struct sockaddr_in);
> +		break;
> +	default:
> +		printerr(0, "ERROR: unrecognized addr family %d\n",
> +			 sa->sa_family);
> +		return NULL;
> +	}
> +
> +	hostname = calloc(1, MAX_HOSTNAME_LEN + 1);
> +	if (!hostname) {
> +		printerr(0, "ERROR: unable to allocate hostname string\n");
> +		return NULL;
> +	}

A simpler approach would be to perform the getnameinfo into a buffer  
on the stack, then strdup it.

> +
> +	err = getnameinfo(sa, addrlen, hostname, MAX_HOSTNAME_LEN, NULL,
> +			  0, NI_NAMEREQD);
> +	if (err) {
> +		free(hostname);
> +		hostname = NULL;
> +		printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
> +			 addr, gai_strerror(err));
> +	}
> +
> +	return hostname;
> +}
> +
> /* XXX buffer problems: */
> static int
> read_service_info(char *info_file_name, char **servicename, char  
> **servername,
> -		  int *prog, int *vers, char **protocol, int *port) {
> +		  int *prog, int *vers, char **protocol,
> +		  struct sockaddr *addr) {
> #define INFOBUFLEN 256
> 	char		buf[INFOBUFLEN + 1];
> 	static char	dummy[128];
> @@ -117,10 +177,9 @@ read_service_info(char *info_file_name, char  
> **servicename, char **servername,
> 	char		protoname[16];
> 	char		cb_port[128];
> 	char		*p;
> -	in_addr_t	inaddr;
> 	int		fd = -1;
> -	struct hostent	*ent = NULL;
> 	int		numfields;
> +	int		port = 0;
>
> 	*servicename = *servername = *protocol = NULL;
>
> @@ -160,21 +219,19 @@ read_service_info(char *info_file_name, char  
> **servicename, char **servername,
> 	if((*prog != 100003) || ((*vers != 2) && (*vers != 3) && (*vers !=  
> 4)))
> 		goto fail;
>
> -	/* create service name */
> -	inaddr = inet_addr(address);
> -	if (!(ent = gethostbyaddr(&inaddr, sizeof(inaddr), AF_INET))) {
> -		printerr(0, "ERROR: can't resolve server %s name\n", address);
> +	if (cb_port[0] != '\0')
> +		port = atoi(cb_port);

Before passing this to addrstr_to_sockaddr, should we check that the  
port value is in the proper range?

> +	if (!addrstr_to_sockaddr(addr, address, port))
> 		goto fail;
> -	}
> -	if (!(*servername = calloc(strlen(ent->h_name) + 1, 1)))
> +
> +	*servername = sockaddr_to_hostname(addr, address);
> +	if (*servername == NULL)
> 		goto fail;
> -	memcpy(*servername, ent->h_name, strlen(ent->h_name));
> -	snprintf(buf, INFOBUFLEN, "%s@%s", service, ent->h_name);
> +
> +	snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);

You should check the return code here.  It should be not greater than  
INFOBUFLEN.

> 	if (!(*servicename = calloc(strlen(buf) + 1, 1)))
> 		goto fail;
> 	memcpy(*servicename, buf, strlen(buf));
> -	if (cb_port[0] != '\0')
> -		*port = atoi(cb_port);
>
> 	if (!(*protocol = strdup(protoname)))
> 		goto fail;
> @@ -251,7 +308,7 @@ process_clnt_dir_files(struct clnt_info * clp)
> 	if ((clp->servicename == NULL) &&
> 	     read_service_info(info_file_name, &clp->servicename,
> 				&clp->servername, &clp->prog, &clp->vers,
> -				&clp->protocol, &clp->port))
> +				&clp->protocol, (struct sockaddr *) &clp->addr))
> 		return -1;
> 	return 0;
> }
> @@ -599,8 +656,9 @@ int create_auth_rpc_client(struct clnt_info *clp,
> 			 clp->servername, uid);
> 		goto out_fail;
> 	}
> -	if (clp->port)
> -		((struct sockaddr_in *)a->ai_addr)->sin_port = htons(clp->port);
> +	if (((struct sockaddr_in) clp->addr).sin_port != 0)
> +		((struct sockaddr_in *) a->ai_addr)->sin_port =
> +				((struct sockaddr_in) clp->addr).sin_port;

This might be an early opportunity to provide address family-agnostic  
get_port() and set_port() helpers.

>
> 	if (a->ai_protocol == IPPROTO_TCP) {
> 		if ((rpc_clnt = clnttcp_create(
> 					(struct sockaddr_in *) a->ai_addr,

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

Re: [PATCH 2/6] nfs-utils: store the address given in the upcall for later use

[Jeff Layton]

On Wed, 1 Apr 2009 12:58:09 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Apr 1, 2009, at 10:24 AM, Jeff Layton wrote:
> 
> > The current upcall could be more efficient. We first convert the  
> > address
> > to a hostname, and then later when we set up the RPC client, we do a
> > hostname lookup to convert it back to an address.
> >
> > Begin to change this by keeping the address in the clnt_info that we  
> > get
> > out of the upcall. Since a sockaddr has a port field, we can also
> > eliminate the port from the clnt_info.
> >
> > Finally, switch to getnameinfo() instead of gethostbyaddr(). We'll  
> > need
> > to use that call anyway when we add support for IPv6.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > utils/gssd/gssd.h      |    2 +-
> > utils/gssd/gssd_proc.c |   90 ++++++++++++++++++++++++++++++++++++++ 
> > +--------
> > 2 files changed, 75 insertions(+), 17 deletions(-)
> >
> > diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
> > index 082039a..3c52f46 100644
> > --- a/utils/gssd/gssd.h
> > +++ b/utils/gssd/gssd.h
> > @@ -83,7 +83,7 @@ struct clnt_info {
> > 	int			krb5_poll_index;
> > 	int			spkm3_fd;
> > 	int			spkm3_poll_index;
> > -	int			port;
> > +	struct sockaddr_storage addr;
> > };
> >
> > void init_client_list(void);
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index 509946e..f5f3a0f 100644
> > --- a/utils/gssd/gssd_proc.c
> > +++ b/utils/gssd/gssd_proc.c
> > @@ -102,10 +102,70 @@ struct pollfd * pollarray;
> >
> > int pollsize;  /* the size of pollaray (in pollfd's) */
> >
> > +/*
> > + * convert an address string to a sockaddr_storage struct. Returns  
> > true
> > + * on success and false on failure.
> > + */
> > +static int
> > +addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const  
> > int port)
> > +{
> > +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
> > +
> > +	if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
> > +		s4->sin_family = AF_INET;
> > +		s4->sin_port = htons(port);
> > +	} else {
> > +		printerr(0, "ERROR: unable to convert %s to address\n", addr);
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> > +/*
> > + * convert a sockaddr to a hostname
> > + */
> > +static char *
> > +sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
> > +{
> > +#define MAX_HOSTNAME_LEN 127
> 
> netdb.h provides NI_MAXHOST.  Since getnameinfo(3) is now required,  
> you can use that.  If this length maximum is required by the kernel  
> gss upcall API, then this should be defined in a more public place, I  
> think, as part of the interface specification.
> 

Thanks, I'll look at that.

> > +	char 			*hostname;
> > +	socklen_t		addrlen;
> > +	int			err;
> > +
> > +	switch (sa->sa_family) {
> > +	case AF_INET:
> > +		addrlen = sizeof(struct sockaddr_in);
> > +		break;
> > +	default:
> > +		printerr(0, "ERROR: unrecognized addr family %d\n",
> > +			 sa->sa_family);
> > +		return NULL;
> > +	}
> > +
> > +	hostname = calloc(1, MAX_HOSTNAME_LEN + 1);
> > +	if (!hostname) {
> > +		printerr(0, "ERROR: unable to allocate hostname string\n");
> > +		return NULL;
> > +	}
> 
> A simpler approach would be to perform the getnameinfo into a buffer  
> on the stack, then strdup it.
> 

Noted...

> > +
> > +	err = getnameinfo(sa, addrlen, hostname, MAX_HOSTNAME_LEN, NULL,
> > +			  0, NI_NAMEREQD);
> > +	if (err) {
> > +		free(hostname);
> > +		hostname = NULL;
> > +		printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
> > +			 addr, gai_strerror(err));
> > +	}
> > +
> > +	return hostname;
> > +}
> > +
> > /* XXX buffer problems: */
> > static int
> > read_service_info(char *info_file_name, char **servicename, char  
> > **servername,
> > -		  int *prog, int *vers, char **protocol, int *port) {
> > +		  int *prog, int *vers, char **protocol,
> > +		  struct sockaddr *addr) {
> > #define INFOBUFLEN 256
> > 	char		buf[INFOBUFLEN + 1];
> > 	static char	dummy[128];
> > @@ -117,10 +177,9 @@ read_service_info(char *info_file_name, char  
> > **servicename, char **servername,
> > 	char		protoname[16];
> > 	char		cb_port[128];
> > 	char		*p;
> > -	in_addr_t	inaddr;
> > 	int		fd = -1;
> > -	struct hostent	*ent = NULL;
> > 	int		numfields;
> > +	int		port = 0;
> >
> > 	*servicename = *servername = *protocol = NULL;
> >
> > @@ -160,21 +219,19 @@ read_service_info(char *info_file_name, char  
> > **servicename, char **servername,
> > 	if((*prog != 100003) || ((*vers != 2) && (*vers != 3) && (*vers !=  
> > 4)))
> > 		goto fail;
> >
> > -	/* create service name */
> > -	inaddr = inet_addr(address);
> > -	if (!(ent = gethostbyaddr(&inaddr, sizeof(inaddr), AF_INET))) {
> > -		printerr(0, "ERROR: can't resolve server %s name\n", address);
> > +	if (cb_port[0] != '\0')
> > +		port = atoi(cb_port);
> 
> Before passing this to addrstr_to_sockaddr, should we check that the  
> port value is in the proper range?
> 

Sounds reasonable.

> > +	if (!addrstr_to_sockaddr(addr, address, port))
> > 		goto fail;
> > -	}
> > -	if (!(*servername = calloc(strlen(ent->h_name) + 1, 1)))
> > +
> > +	*servername = sockaddr_to_hostname(addr, address);
> > +	if (*servername == NULL)
> > 		goto fail;
> > -	memcpy(*servername, ent->h_name, strlen(ent->h_name));
> > -	snprintf(buf, INFOBUFLEN, "%s@%s", service, ent->h_name);
> > +
> > +	snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
> 
> You should check the return code here.  It should be not greater than  
> INFOBUFLEN.
> 

Do we really need to do this? Doesn't using snprintf guarantee that
it won't be?

> > 	if (!(*servicename = calloc(strlen(buf) + 1, 1)))
> > 		goto fail;
> > 	memcpy(*servicename, buf, strlen(buf));
> > -	if (cb_port[0] != '\0')
> > -		*port = atoi(cb_port);
> >
> > 	if (!(*protocol = strdup(protoname)))
> > 		goto fail;
> > @@ -251,7 +308,7 @@ process_clnt_dir_files(struct clnt_info * clp)
> > 	if ((clp->servicename == NULL) &&
> > 	     read_service_info(info_file_name, &clp->servicename,
> > 				&clp->servername, &clp->prog, &clp->vers,
> > -				&clp->protocol, &clp->port))
> > +				&clp->protocol, (struct sockaddr *) &clp->addr))
> > 		return -1;
> > 	return 0;
> > }
> > @@ -599,8 +656,9 @@ int create_auth_rpc_client(struct clnt_info *clp,
> > 			 clp->servername, uid);
> > 		goto out_fail;
> > 	}
> > -	if (clp->port)
> > -		((struct sockaddr_in *)a->ai_addr)->sin_port = htons(clp->port);
> > +	if (((struct sockaddr_in) clp->addr).sin_port != 0)
> > +		((struct sockaddr_in *) a->ai_addr)->sin_port =
> > +				((struct sockaddr_in) clp->addr).sin_port;
> 
> This might be an early opportunity to provide address family-agnostic  
> get_port() and set_port() helpers.
> 

I do that in a later patch with the populate_port() function. The only
reason I have the above is to keep things bisectable.

> >
> > 	if (a->ai_protocol == IPPROTO_TCP) {
> > 		if ((rpc_clnt = clnttcp_create(
> > 					(struct sockaddr_in *) a->ai_addr,
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com


Thanks for the comments so far...
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/6] nfs-utils: store the address given in the upcall for later use

[Chuck Lever]

On Apr 1, 2009, at 1:06 PM, Jeff Layton wrote:
> On Wed, 1 Apr 2009 12:58:09 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>> +	if (!addrstr_to_sockaddr(addr, address, port))
>>> 		goto fail;
>>> -	}
>>> -	if (!(*servername = calloc(strlen(ent->h_name) + 1, 1)))
>>> +
>>> +	*servername = sockaddr_to_hostname(addr, address);
>>> +	if (*servername == NULL)
>>> 		goto fail;
>>> -	memcpy(*servername, ent->h_name, strlen(ent->h_name));
>>> -	snprintf(buf, INFOBUFLEN, "%s@%s", service, ent->h_name);
>>> +
>>> +	snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
>>
>> You should check the return code here.  It should be not greater than
>> INFOBUFLEN.
>>
>
> Do we really need to do this? Doesn't using snprintf guarantee that
> it won't be?

The point of the check is to see if a buffer overflow _would_ have  
occurred.  If the string was larger than the buffer, then we know  
either the service or *servername is somehow invalid (like not  
terminated with '\0').  Or, perhaps some error occurred inside snprintf.

Either way, you should check the return code here.

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

[PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Jeff Layton]

We already have the server's address from the upcall, so we don't really
need to look it up again. Move the getaddrinfo call in
create_auth_rpc_client to a new function. Skip it if we already have the
port in the sockaddr that we saved from the info in the upcall. If
we need to get the port, don't bother looking up the hostname, just do
the getaddrinfo with AI_NUMERICHOST set.

This should reduce the amount of lookups that are needed.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |  134 +++++++++++++++++++++++++++++-------------------
 1 files changed, 81 insertions(+), 53 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index f5f3a0f..4d54c40 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -538,6 +538,76 @@ out_err:
 }
 
 /*
+ * Determine the port from the servicename and set the right field in the
+ * sockaddr. This is mostly a no-op with newer kernels that send the port
+ * in the upcall. Returns true on success and false on failure.
+ */
+static int
+populate_port(struct sockaddr *sa, char *servicename, int socktype,
+	      int protocol)
+{
+	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+	char			node[INET_ADDRSTRLEN];
+	char			service[64];
+	char			*at_sign;
+	int			errcode;
+	struct addrinfo		*a = NULL;
+	struct addrinfo		ai_hints = { .ai_family	  = sa->sa_family,
+					     .ai_socktype = socktype,
+					     .ai_protocol = protocol,
+					     .ai_flags    = AI_NUMERICHOST };
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		if (s4->sin_port != 0) {
+			printerr(2, "DEBUG: port already set to %d\n",
+				 ntohs(s4->sin_port));
+			return 1;
+		}
+		if (!inet_ntop(AF_INET, &s4->sin_addr, node,
+				sizeof(struct sockaddr_in)))
+			return 0;
+		break;
+	default:
+		printerr(0, "ERROR: unsupported address family %d\n",
+			    sa->sa_family);
+		return 0;
+	}
+
+	/* extract the service name from clp->servicename */
+	if ((at_sign = strchr(servicename, '@')) == NULL) {
+		printerr(0, "WARNING: servicename (%s) not formatted as "
+			"expected with service@host\n", servicename);
+		return 0;
+	}
+	if ((at_sign - servicename) >= sizeof(service)) {
+		printerr(0, "WARNING: service portion of servicename (%s) "
+			"is too long!\n", servicename);
+		return 0;
+	}
+	strncpy(service, servicename, at_sign - servicename);
+	service[at_sign - servicename] = '\0';
+
+	errcode = getaddrinfo(node, service, &ai_hints, &a);
+	if (errcode) {
+		printerr(0, "WARNING: Error from getaddrinfo for service "
+			 "'%s': %s\n", service,
+			 errcode == EAI_SYSTEM ? strerror(errno) :
+						 gai_strerror(errcode));
+		return 0;
+	}
+
+	if (a->ai_family == AF_INET)
+		s4->sin_port = ((struct sockaddr_in *) a->ai_addr)->sin_port;
+	else
+		printerr(0, "ERROR: unrecognized address family %d returned "
+			    "by getaddrinfo.\n", a->ai_family);
+
+	freeaddrinfo(a);
+	return 1;
+}
+
+/*
  * Create an RPC connection and establish an authenticated
  * gss context with a server.
  */
@@ -552,14 +622,11 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	AUTH			*auth = NULL;
 	uid_t			save_uid = -1;
 	int			retval = -1;
-	int			errcode;
 	OM_uint32		min_stat;
 	char			rpc_errmsg[1024];
 	int			sockp = RPC_ANYSOCK;
 	int			sendsz = 32768, recvsz = 32768;
-	struct addrinfo		ai_hints, *a = NULL;
-	char			service[64];
-	char			*at_sign;
+	int			socktype, protocol;
 
 	/* Create the context as the user (not as root) */
 	save_uid = geteuid();
@@ -613,15 +680,12 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	printerr(2, "creating %s client for server %s\n", clp->protocol,
 			clp->servername);
 
-	memset(&ai_hints, '\0', sizeof(ai_hints));
-	ai_hints.ai_family = PF_INET;
-	ai_hints.ai_flags |= AI_CANONNAME;
 	if ((strcmp(clp->protocol, "tcp")) == 0) {
-		ai_hints.ai_socktype = SOCK_STREAM;
-		ai_hints.ai_protocol = IPPROTO_TCP;
+		socktype = SOCK_STREAM;
+		protocol = IPPROTO_TCP;
 	} else if ((strcmp(clp->protocol, "udp")) == 0) {
-		ai_hints.ai_socktype = SOCK_DGRAM;
-		ai_hints.ai_protocol = IPPROTO_UDP;
+		socktype = SOCK_DGRAM;
+		protocol = IPPROTO_UDP;
 	} else {
 		printerr(0, "WARNING: unrecognized protocol, '%s', requested "
 			 "for connection to server %s for user with uid %d\n",
@@ -629,39 +693,13 @@ int create_auth_rpc_client(struct clnt_info *clp,
 		goto out_fail;
 	}
 
-	/* extract the service name from clp->servicename */
-	if ((at_sign = strchr(clp->servicename, '@')) == NULL) {
-		printerr(0, "WARNING: servicename (%s) not formatted as "
-			"expected with service@host\n", clp->servicename);
-		goto out_fail;
-	}
-	if ((at_sign - clp->servicename) >= sizeof(service)) {
-		printerr(0, "WARNING: service portion of servicename (%s) "
-			"is too long!\n", clp->servicename);
-		goto out_fail;
-	}
-	strncpy(service, clp->servicename, at_sign - clp->servicename);
-	service[at_sign - clp->servicename] = '\0';
-
-	errcode = getaddrinfo(clp->servername, service, &ai_hints, &a);
-	if (errcode) {
-		printerr(0, "WARNING: Error from getaddrinfo for server "
-			 "'%s': %s\n", clp->servername, gai_strerror(errcode));
+	if (!populate_port((struct sockaddr *) &clp->addr, clp->servicename,
+			   socktype, protocol))
 		goto out_fail;
-	}
 
-	if (a == NULL) {
-		printerr(0, "WARNING: No address information found for "
-			 "connection to server %s for user with uid %d\n",
-			 clp->servername, uid);
-		goto out_fail;
-	}
-	if (((struct sockaddr_in) clp->addr).sin_port != 0)
-		((struct sockaddr_in *) a->ai_addr)->sin_port =
-				((struct sockaddr_in) clp->addr).sin_port;
-	if (a->ai_protocol == IPPROTO_TCP) {
+	if (protocol == IPPROTO_TCP) {
 		if ((rpc_clnt = clnttcp_create(
-					(struct sockaddr_in *) a->ai_addr,
+					(struct sockaddr_in *) &clp->addr,
 					clp->prog, clp->vers, &sockp,
 					sendsz, recvsz)) == NULL) {
 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
@@ -672,10 +710,10 @@ int create_auth_rpc_client(struct clnt_info *clp,
 				 clnt_spcreateerror(rpc_errmsg));
 			goto out_fail;
 		}
-	} else if (a->ai_protocol == IPPROTO_UDP) {
+	} else if (protocol == IPPROTO_UDP) {
 		const struct timeval timeout = {5, 0};
 		if ((rpc_clnt = clntudp_bufcreate(
-					(struct sockaddr_in *) a->ai_addr,
+					(struct sockaddr_in *) &clp->addr,
 					clp->prog, clp->vers, timeout,
 					&sockp, sendsz, recvsz)) == NULL) {
 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
@@ -686,16 +724,7 @@ int create_auth_rpc_client(struct clnt_info *clp,
 				 clnt_spcreateerror(rpc_errmsg));
 			goto out_fail;
 		}
-	} else {
-		/* Shouldn't happen! */
-		printerr(0, "ERROR: requested protocol '%s', but "
-			 "got addrinfo with protocol %d\n",
-			 clp->protocol, a->ai_protocol);
-		goto out_fail;
 	}
-	/* We're done with this */
-	freeaddrinfo(a);
-	a = NULL;
 
 	printerr(2, "creating context with server %s\n", clp->servicename);
 	auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
@@ -717,7 +746,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
   out:
 	if (sec.cred != GSS_C_NO_CREDENTIAL)
 		gss_release_cred(&min_stat, &sec.cred);
-  	if (a != NULL) freeaddrinfo(a);
 	/* Restore euid to original value */
 	if ((save_uid != -1) && (setfsuid(save_uid) != uid)) {
 		printerr(0, "WARNING: Failed to restore fsuid"
-- 
1.6.0.6

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Chuck Lever]

On Apr 1, 2009, at 10:24 AM, Jeff Layton wrote:

> We already have the server's address from the upcall, so we don't  
> really
> need to look it up again. Move the getaddrinfo call in
> create_auth_rpc_client to a new function. Skip it if we already have  
> the
> port in the sockaddr that we saved from the info in the upcall. If
> we need to get the port, don't bother looking up the hostname, just do
> the getaddrinfo with AI_NUMERICHOST set.
>
> This should reduce the amount of lookups that are needed.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> utils/gssd/gssd_proc.c |  134 ++++++++++++++++++++++++++++ 
> +-------------------
> 1 files changed, 81 insertions(+), 53 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index f5f3a0f..4d54c40 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -538,6 +538,76 @@ out_err:
> }
>
> /*
> + * Determine the port from the servicename and set the right field  
> in the
> + * sockaddr. This is mostly a no-op with newer kernels that send  
> the port
> + * in the upcall. Returns true on success and false on failure.
> + */
> +static int
> +populate_port(struct sockaddr *sa, char *servicename, int socktype,
> +	      int protocol)

I think you can guess the socktype from the protocol setting, so maybe  
you don't need @socktype.  Actually, for AI_NUMERICHOST I don't think  
protocol and socktype are even relevant.

> +{
> +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
> +	char			node[INET_ADDRSTRLEN];
> +	char			service[64];
> +	char			*at_sign;
> +	int			errcode;
> +	struct addrinfo		*a = NULL;
> +	struct addrinfo		ai_hints = { .ai_family	  = sa->sa_family,
> +					     .ai_socktype = socktype,
> +					     .ai_protocol = protocol,
> +					     .ai_flags    = AI_NUMERICHOST };
> +
> +	switch (sa->sa_family) {
> +	case AF_INET:
> +		if (s4->sin_port != 0) {
> +			printerr(2, "DEBUG: port already set to %d\n",
> +				 ntohs(s4->sin_port));
> +			return 1;
> +		}
> +		if (!inet_ntop(AF_INET, &s4->sin_addr, node,
> +				sizeof(struct sockaddr_in)))
> +			return 0;
> +		break;
> +	default:
> +		printerr(0, "ERROR: unsupported address family %d\n",
> +			    sa->sa_family);
> +		return 0;
> +	}
> +
> +	/* extract the service name from clp->servicename */
> +	if ((at_sign = strchr(servicename, '@')) == NULL) {
> +		printerr(0, "WARNING: servicename (%s) not formatted as "
> +			"expected with service@host\n", servicename);
> +		return 0;
> +	}
> +	if ((at_sign - servicename) >= sizeof(service)) {
> +		printerr(0, "WARNING: service portion of servicename (%s) "
> +			"is too long!\n", servicename);
> +		return 0;
> +	}
> +	strncpy(service, servicename, at_sign - servicename);
> +	service[at_sign - servicename] = '\0';
> +
> +	errcode = getaddrinfo(node, service, &ai_hints, &a);
> +	if (errcode) {
> +		printerr(0, "WARNING: Error from getaddrinfo for service "
> +			 "'%s': %s\n", service,
> +			 errcode == EAI_SYSTEM ? strerror(errno) :
> +						 gai_strerror(errcode));
> +		return 0;
> +	}

Not sure what's going on with "node" here.  You should be able to pass  
NULL for the nodename, and just get an ANYADDR sockaddr back with the  
port set.

> +
> +	if (a->ai_family == AF_INET)
> +		s4->sin_port = ((struct sockaddr_in *) a->ai_addr)->sin_port;
> +	else
> +		printerr(0, "ERROR: unrecognized address family %d returned "
> +			    "by getaddrinfo.\n", a->ai_family);
> +
> +	freeaddrinfo(a);
> +	return 1;
> +}
> +
> +/*
>  * Create an RPC connection and establish an authenticated
>  * gss context with a server.
>  */
> @@ -552,14 +622,11 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 	AUTH			*auth = NULL;
> 	uid_t			save_uid = -1;
> 	int			retval = -1;
> -	int			errcode;
> 	OM_uint32		min_stat;
> 	char			rpc_errmsg[1024];
> 	int			sockp = RPC_ANYSOCK;
> 	int			sendsz = 32768, recvsz = 32768;
> -	struct addrinfo		ai_hints, *a = NULL;
> -	char			service[64];
> -	char			*at_sign;
> +	int			socktype, protocol;
>
> 	/* Create the context as the user (not as root) */
> 	save_uid = geteuid();
> @@ -613,15 +680,12 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 	printerr(2, "creating %s client for server %s\n", clp->protocol,
> 			clp->servername);
>
> -	memset(&ai_hints, '\0', sizeof(ai_hints));
> -	ai_hints.ai_family = PF_INET;
> -	ai_hints.ai_flags |= AI_CANONNAME;
> 	if ((strcmp(clp->protocol, "tcp")) == 0) {
> -		ai_hints.ai_socktype = SOCK_STREAM;
> -		ai_hints.ai_protocol = IPPROTO_TCP;
> +		socktype = SOCK_STREAM;
> +		protocol = IPPROTO_TCP;
> 	} else if ((strcmp(clp->protocol, "udp")) == 0) {
> -		ai_hints.ai_socktype = SOCK_DGRAM;
> -		ai_hints.ai_protocol = IPPROTO_UDP;
> +		socktype = SOCK_DGRAM;
> +		protocol = IPPROTO_UDP;
> 	} else {
> 		printerr(0, "WARNING: unrecognized protocol, '%s', requested "
> 			 "for connection to server %s for user with uid %d\n",
> @@ -629,39 +693,13 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 		goto out_fail;
> 	}
>
> -	/* extract the service name from clp->servicename */
> -	if ((at_sign = strchr(clp->servicename, '@')) == NULL) {
> -		printerr(0, "WARNING: servicename (%s) not formatted as "
> -			"expected with service@host\n", clp->servicename);
> -		goto out_fail;
> -	}
> -	if ((at_sign - clp->servicename) >= sizeof(service)) {
> -		printerr(0, "WARNING: service portion of servicename (%s) "
> -			"is too long!\n", clp->servicename);
> -		goto out_fail;
> -	}
> -	strncpy(service, clp->servicename, at_sign - clp->servicename);
> -	service[at_sign - clp->servicename] = '\0';
> -
> -	errcode = getaddrinfo(clp->servername, service, &ai_hints, &a);
> -	if (errcode) {
> -		printerr(0, "WARNING: Error from getaddrinfo for server "
> -			 "'%s': %s\n", clp->servername, gai_strerror(errcode));
> +	if (!populate_port((struct sockaddr *) &clp->addr, clp->servicename,
> +			   socktype, protocol))
> 		goto out_fail;
> -	}
>
> -	if (a == NULL) {
> -		printerr(0, "WARNING: No address information found for "
> -			 "connection to server %s for user with uid %d\n",
> -			 clp->servername, uid);
> -		goto out_fail;
> -	}
> -	if (((struct sockaddr_in) clp->addr).sin_port != 0)
> -		((struct sockaddr_in *) a->ai_addr)->sin_port =
> -				((struct sockaddr_in) clp->addr).sin_port;
> -	if (a->ai_protocol == IPPROTO_TCP) {
> +	if (protocol == IPPROTO_TCP) {
> 		if ((rpc_clnt = clnttcp_create(
> -					(struct sockaddr_in *) a->ai_addr,
> +					(struct sockaddr_in *) &clp->addr,
> 					clp->prog, clp->vers, &sockp,
> 					sendsz, recvsz)) == NULL) {
> 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> @@ -672,10 +710,10 @@ int create_auth_rpc_client(struct clnt_info  
> *clp,
> 				 clnt_spcreateerror(rpc_errmsg));
> 			goto out_fail;
> 		}
> -	} else if (a->ai_protocol == IPPROTO_UDP) {
> +	} else if (protocol == IPPROTO_UDP) {
> 		const struct timeval timeout = {5, 0};
> 		if ((rpc_clnt = clntudp_bufcreate(
> -					(struct sockaddr_in *) a->ai_addr,
> +					(struct sockaddr_in *) &clp->addr,
> 					clp->prog, clp->vers, timeout,
> 					&sockp, sendsz, recvsz)) == NULL) {
> 			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> @@ -686,16 +724,7 @@ int create_auth_rpc_client(struct clnt_info *clp,
> 				 clnt_spcreateerror(rpc_errmsg));
> 			goto out_fail;
> 		}
> -	} else {
> -		/* Shouldn't happen! */
> -		printerr(0, "ERROR: requested protocol '%s', but "
> -			 "got addrinfo with protocol %d\n",
> -			 clp->protocol, a->ai_protocol);
> -		goto out_fail;
> 	}
> -	/* We're done with this */
> -	freeaddrinfo(a);
> -	a = NULL;
>
> 	printerr(2, "creating context with server %s\n", clp->servicename);
> 	auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
> @@ -717,7 +746,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
>   out:
> 	if (sec.cred != GSS_C_NO_CREDENTIAL)
> 		gss_release_cred(&min_stat, &sec.cred);
> -  	if (a != NULL) freeaddrinfo(a);
> 	/* Restore euid to original value */
> 	if ((save_uid != -1) && (setfsuid(save_uid) != uid)) {
> 		printerr(0, "WARNING: Failed to restore fsuid"

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

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Jeff Layton]

On Wed, 1 Apr 2009 13:11:04 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Apr 1, 2009, at 10:24 AM, Jeff Layton wrote:
> 
> > We already have the server's address from the upcall, so we don't  
> > really
> > need to look it up again. Move the getaddrinfo call in
> > create_auth_rpc_client to a new function. Skip it if we already have  
> > the
> > port in the sockaddr that we saved from the info in the upcall. If
> > we need to get the port, don't bother looking up the hostname, just do
> > the getaddrinfo with AI_NUMERICHOST set.
> >
> > This should reduce the amount of lookups that are needed.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > utils/gssd/gssd_proc.c |  134 ++++++++++++++++++++++++++++ 
> > +-------------------
> > 1 files changed, 81 insertions(+), 53 deletions(-)
> >
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index f5f3a0f..4d54c40 100644
> > --- a/utils/gssd/gssd_proc.c
> > +++ b/utils/gssd/gssd_proc.c
> > @@ -538,6 +538,76 @@ out_err:
> > }
> >
> > /*
> > + * Determine the port from the servicename and set the right field  
> > in the
> > + * sockaddr. This is mostly a no-op with newer kernels that send  
> > the port
> > + * in the upcall. Returns true on success and false on failure.
> > + */
> > +static int
> > +populate_port(struct sockaddr *sa, char *servicename, int socktype,
> > +	      int protocol)
> 
> I think you can guess the socktype from the protocol setting, so maybe  
> you don't need @socktype.  Actually, for AI_NUMERICHOST I don't think  
> protocol and socktype are even relevant.
> 

It seems to me that getaddrinfo() needs to have some way to know
whether you want the tcp or udp port for the service. I'm not sure
whether it uses the ai_socktype or ai_protocol to determine this
(probably the protocol), but I figure it doesn't hurt to set both.

> > +{
> > +	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
> > +	char			node[INET_ADDRSTRLEN];
> > +	char			service[64];
> > +	char			*at_sign;
> > +	int			errcode;
> > +	struct addrinfo		*a = NULL;
> > +	struct addrinfo		ai_hints = { .ai_family	  = sa->sa_family,
> > +					     .ai_socktype = socktype,
> > +					     .ai_protocol = protocol,
> > +					     .ai_flags    = AI_NUMERICHOST };
> > +
> > +	switch (sa->sa_family) {
> > +	case AF_INET:
> > +		if (s4->sin_port != 0) {
> > +			printerr(2, "DEBUG: port already set to %d\n",
> > +				 ntohs(s4->sin_port));
> > +			return 1;
> > +		}
> > +		if (!inet_ntop(AF_INET, &s4->sin_addr, node,
> > +				sizeof(struct sockaddr_in)))
> > +			return 0;
> > +		break;
> > +	default:
> > +		printerr(0, "ERROR: unsupported address family %d\n",
> > +			    sa->sa_family);
> > +		return 0;
> > +	}
> > +
> > +	/* extract the service name from clp->servicename */
> > +	if ((at_sign = strchr(servicename, '@')) == NULL) {
> > +		printerr(0, "WARNING: servicename (%s) not formatted as "
> > +			"expected with service@host\n", servicename);
> > +		return 0;
> > +	}
> > +	if ((at_sign - servicename) >= sizeof(service)) {
> > +		printerr(0, "WARNING: service portion of servicename (%s) "
> > +			"is too long!\n", servicename);
> > +		return 0;
> > +	}
> > +	strncpy(service, servicename, at_sign - servicename);
> > +	service[at_sign - servicename] = '\0';
> > +
> > +	errcode = getaddrinfo(node, service, &ai_hints, &a);
> > +	if (errcode) {
> > +		printerr(0, "WARNING: Error from getaddrinfo for service "
> > +			 "'%s': %s\n", service,
> > +			 errcode == EAI_SYSTEM ? strerror(errno) :
> > +						 gai_strerror(errcode));
> > +		return 0;
> > +	}
> 
> Not sure what's going on with "node" here.  You should be able to pass  
> NULL for the nodename, and just get an ANYADDR sockaddr back with the  
> port set.
> 

Looks like that's correct. I'll plan to fix it to do that in the next
release.

I'll also have a look at nfs_get_rpcclient(). I think I looked at it
before and decided it wasn't suitable for this, but I don't recall the
reason. Maybe I can change it so that it is.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Chuck Lever]

On Apr 1, 2009, at 1:47 PM, Jeff Layton wrote:
> On Wed, 1 Apr 2009 13:11:04 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On Apr 1, 2009, at 10:24 AM, Jeff Layton wrote:
>>> We already have the server's address from the upcall, so we don't
>>> really
>>> need to look it up again. Move the getaddrinfo call in
>>> create_auth_rpc_client to a new function. Skip it if we already have
>>> the
>>> port in the sockaddr that we saved from the info in the upcall. If
>>> we need to get the port, don't bother looking up the hostname,  
>>> just do
>>> the getaddrinfo with AI_NUMERICHOST set.
>>>
>>> This should reduce the amount of lookups that are needed.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> utils/gssd/gssd_proc.c |  134 ++++++++++++++++++++++++++++
>>> +-------------------
>>> 1 files changed, 81 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>>> index f5f3a0f..4d54c40 100644
>>> --- a/utils/gssd/gssd_proc.c
>>> +++ b/utils/gssd/gssd_proc.c
>>> @@ -538,6 +538,76 @@ out_err:
>>> }
>>>
>>> /*
>>> + * Determine the port from the servicename and set the right field
>>> in the
>>> + * sockaddr. This is mostly a no-op with newer kernels that send
>>> the port
>>> + * in the upcall. Returns true on success and false on failure.
>>> + */
>>> +static int
>>> +populate_port(struct sockaddr *sa, char *servicename, int socktype,
>>> +	      int protocol)
>>
>> I think you can guess the socktype from the protocol setting, so  
>> maybe
>> you don't need @socktype.  Actually, for AI_NUMERICHOST I don't think
>> protocol and socktype are even relevant.
>>
>
> It seems to me that getaddrinfo() needs to have some way to know
> whether you want the tcp or udp port for the service. I'm not sure
> whether it uses the ai_socktype or ai_protocol to determine this
> (probably the protocol), but I figure it doesn't hurt to set both.

As far as I understand it, the ai_socktype and ai_protocol fields are  
used to return the values needed for subsequent socket(2)/bind(2)  
system calls.  In this case you are not using these fields from the  
results...

If ai_protocol is zero, then getaddrinfo(3) assumes you want one copy  
of the address for each supported protocol type, so it returns three  
structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one with a  
zero protocol number).  The contents, except for the socktype and  
protocol fields, are the same for each.

You're using only the port number in the socket address, and ignoring  
the other contents.  So, yes, it works the same anyway, but it's  
confusing to read all the extra logic in populate_port().

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

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Jeff Layton]

On Wed, 1 Apr 2009 14:01:30 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> As far as I understand it, the ai_socktype and ai_protocol fields are  
> used to return the values needed for subsequent socket(2)/bind(2)  
> system calls.  In this case you are not using these fields from the  
> results...
> 
> If ai_protocol is zero, then getaddrinfo(3) assumes you want one copy  
> of the address for each supported protocol type, so it returns three  
> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one with a  
> zero protocol number).  The contents, except for the socktype and  
> protocol fields, are the same for each.
> 

Hypothetical situation...

Suppose there is a service in /etc/services that has a different port
number for tcp than for udp:

fooserv		50001/tcp
fooserv		50002/udp

You're saying that getaddrinfo will return the same port number in all
of the returned structures? Won't that mean that one of the port numbers
is wrong? That seems broken if so...

If that's not the case, then I think we need to at least set the
ai_protocol in the hints.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Chuck Lever]

On Apr 3, 2009, at 3:04 PM, Jeff Layton wrote:
> On Wed, 1 Apr 2009 14:01:30 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
>> As far as I understand it, the ai_socktype and ai_protocol fields are
>> used to return the values needed for subsequent socket(2)/bind(2)
>> system calls.  In this case you are not using these fields from the
>> results...
>>
>> If ai_protocol is zero, then getaddrinfo(3) assumes you want one copy
>> of the address for each supported protocol type, so it returns three
>> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one with a
>> zero protocol number).  The contents, except for the socktype and
>> protocol fields, are the same for each.
>
> Hypothetical situation...
>
> Suppose there is a service in /etc/services that has a different port
> number for tcp than for udp:
>
> fooserv		50001/tcp
> fooserv		50002/udp
>
> You're saying that getaddrinfo will return the same port number in all
> of the returned structures? Won't that mean that one of the port  
> numbers
> is wrong? That seems broken if so...

I was trying to describe observed behavior here -- it's pretty  
unlikely that there will be different port numbers in these returned  
structures.  It's difficult to say precisely how this is supposed to  
behave based on the man page or even browsing the glibc source code  
for a few minutes.

It's certainly possible to set up /etc/services as you suggest, but  
there is an IANA policy to assign the same port for both transports.   
As near as I can tell the reason we have the transport listed in /etc/ 
services at all is because some protocols support only one transport.   
So IMO it would be quite unlikely to encounter such a case where the  
port number depended on the transport.

> If that's not the case, then I think we need to at least set the
> ai_protocol in the hints.

Perhaps that's true.  What are the expected values of @service ?

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

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Jeff Layton]

On Mon, 6 Apr 2009 11:03:11 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On Apr 3, 2009, at 3:04 PM, Jeff Layton wrote:
> > On Wed, 1 Apr 2009 14:01:30 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> >> As far as I understand it, the ai_socktype and ai_protocol fields are
> >> used to return the values needed for subsequent socket(2)/bind(2)
> >> system calls.  In this case you are not using these fields from the
> >> results...
> >>
> >> If ai_protocol is zero, then getaddrinfo(3) assumes you want one copy
> >> of the address for each supported protocol type, so it returns three
> >> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one with a
> >> zero protocol number).  The contents, except for the socktype and
> >> protocol fields, are the same for each.
> >
> > Hypothetical situation...
> >
> > Suppose there is a service in /etc/services that has a different port
> > number for tcp than for udp:
> >
> > fooserv		50001/tcp
> > fooserv		50002/udp
> >
> > You're saying that getaddrinfo will return the same port number in all
> > of the returned structures? Won't that mean that one of the port  
> > numbers
> > is wrong? That seems broken if so...
> 
> I was trying to describe observed behavior here -- it's pretty  
> unlikely that there will be different port numbers in these returned  
> structures.  It's difficult to say precisely how this is supposed to  
> behave based on the man page or even browsing the glibc source code  
> for a few minutes.
> 
> It's certainly possible to set up /etc/services as you suggest, but  
> there is an IANA policy to assign the same port for both transports.   
> As near as I can tell the reason we have the transport listed in /etc/ 
> services at all is because some protocols support only one transport.   
> So IMO it would be quite unlikely to encounter such a case where the  
> port number depended on the transport.
> 
> > If that's not the case, then I think we need to at least set the
> > ai_protocol in the hints.
> 
> Perhaps that's true.  What are the expected values of @service ?
> 

I've only ever seen "nfs" here, but I guess you could use this with
others. Maybe "nfsacl" too?

IANA might not set different port like this, but there's nothing
that stops someone from doing this at their site.

I agree that it's an unlikely (and somewhat pathological) situation,
but it's easy to deal with -- just set the protocol in the hints. I've
confirmed that it makes getaddrinfo do the right thing.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Chuck Lever]

On Apr 6, 2009, at 11:21 AM, Jeff Layton wrote:
> On Mon, 6 Apr 2009 11:03:11 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
>> On Apr 3, 2009, at 3:04 PM, Jeff Layton wrote:
>>> On Wed, 1 Apr 2009 14:01:30 -0400
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>>
>>>> As far as I understand it, the ai_socktype and ai_protocol fields  
>>>> are
>>>> used to return the values needed for subsequent socket(2)/bind(2)
>>>> system calls.  In this case you are not using these fields from the
>>>> results...
>>>>
>>>> If ai_protocol is zero, then getaddrinfo(3) assumes you want one  
>>>> copy
>>>> of the address for each supported protocol type, so it returns  
>>>> three
>>>> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one  
>>>> with a
>>>> zero protocol number).  The contents, except for the socktype and
>>>> protocol fields, are the same for each.
>>>
>>> Hypothetical situation...
>>>
>>> Suppose there is a service in /etc/services that has a different  
>>> port
>>> number for tcp than for udp:
>>>
>>> fooserv		50001/tcp
>>> fooserv		50002/udp
>>>
>>> You're saying that getaddrinfo will return the same port number in  
>>> all
>>> of the returned structures? Won't that mean that one of the port
>>> numbers
>>> is wrong? That seems broken if so...
>>
>> I was trying to describe observed behavior here -- it's pretty
>> unlikely that there will be different port numbers in these returned
>> structures.  It's difficult to say precisely how this is supposed to
>> behave based on the man page or even browsing the glibc source code
>> for a few minutes.
>>
>> It's certainly possible to set up /etc/services as you suggest, but
>> there is an IANA policy to assign the same port for both transports.
>> As near as I can tell the reason we have the transport listed in / 
>> etc/
>> services at all is because some protocols support only one transport.
>> So IMO it would be quite unlikely to encounter such a case where the
>> port number depended on the transport.
>>
>>> If that's not the case, then I think we need to at least set the
>>> ai_protocol in the hints.
>>
>> Perhaps that's true.  What are the expected values of @service ?
>
> I've only ever seen "nfs" here, but I guess you could use this with
> others. Maybe "nfsacl" too?

What's even stranger is that one is supposed to use rpcbind to figure  
out what NFS port to use, not /etc/services.

> IANA might not set different port like this, but there's nothing
> that stops someone from doing this at their site.

True, but one might expect that setting the NFS service port via  
rpc.nfsd on the server would make gssd use that port too.  I guess  
that's why the port number is now passed up from the kernel.

> I agree that it's an unlikely (and somewhat pathological) situation,
> but it's easy to deal with -- just set the protocol in the hints. I've
> confirmed that it makes getaddrinfo do the right thing.

That's good.  I think we need to understand exactly what this is  
supposed to do, though.  Should we use an rpcbind call here instead?   
This is _rpc_ gssd, after all.  If it's OK as it stands, then I think  
some comments about why this works this way are in order :-)

Kevin, can you enlighten us?

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

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Jeff Layton]

On Mon, 6 Apr 2009 11:46:33 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On Apr 6, 2009, at 11:21 AM, Jeff Layton wrote:
> > On Mon, 6 Apr 2009 11:03:11 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> >> On Apr 3, 2009, at 3:04 PM, Jeff Layton wrote:
> >>> On Wed, 1 Apr 2009 14:01:30 -0400
> >>> Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>
> >>>> As far as I understand it, the ai_socktype and ai_protocol fields  
> >>>> are
> >>>> used to return the values needed for subsequent socket(2)/bind(2)
> >>>> system calls.  In this case you are not using these fields from the
> >>>> results...
> >>>>
> >>>> If ai_protocol is zero, then getaddrinfo(3) assumes you want one  
> >>>> copy
> >>>> of the address for each supported protocol type, so it returns  
> >>>> three
> >>>> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one  
> >>>> with a
> >>>> zero protocol number).  The contents, except for the socktype and
> >>>> protocol fields, are the same for each.
> >>>
> >>> Hypothetical situation...
> >>>
> >>> Suppose there is a service in /etc/services that has a different  
> >>> port
> >>> number for tcp than for udp:
> >>>
> >>> fooserv		50001/tcp
> >>> fooserv		50002/udp
> >>>
> >>> You're saying that getaddrinfo will return the same port number in  
> >>> all
> >>> of the returned structures? Won't that mean that one of the port
> >>> numbers
> >>> is wrong? That seems broken if so...
> >>
> >> I was trying to describe observed behavior here -- it's pretty
> >> unlikely that there will be different port numbers in these returned
> >> structures.  It's difficult to say precisely how this is supposed to
> >> behave based on the man page or even browsing the glibc source code
> >> for a few minutes.
> >>
> >> It's certainly possible to set up /etc/services as you suggest, but
> >> there is an IANA policy to assign the same port for both transports.
> >> As near as I can tell the reason we have the transport listed in / 
> >> etc/
> >> services at all is because some protocols support only one transport.
> >> So IMO it would be quite unlikely to encounter such a case where the
> >> port number depended on the transport.
> >>
> >>> If that's not the case, then I think we need to at least set the
> >>> ai_protocol in the hints.
> >>
> >> Perhaps that's true.  What are the expected values of @service ?
> >
> > I've only ever seen "nfs" here, but I guess you could use this with
> > others. Maybe "nfsacl" too?
> 
> What's even stranger is that one is supposed to use rpcbind to figure  
> out what NFS port to use, not /etc/services.
> 
> > IANA might not set different port like this, but there's nothing
> > that stops someone from doing this at their site.
> 
> True, but one might expect that setting the NFS service port via  
> rpc.nfsd on the server would make gssd use that port too.  I guess  
> that's why the port number is now passed up from the kernel.
> 
> > I agree that it's an unlikely (and somewhat pathological) situation,
> > but it's easy to deal with -- just set the protocol in the hints. I've
> > confirmed that it makes getaddrinfo do the right thing.
> 
> That's good.  I think we need to understand exactly what this is  
> supposed to do, though.  Should we use an rpcbind call here instead?   
> This is _rpc_ gssd, after all.  If it's OK as it stands, then I think  
> some comments about why this works this way are in order :-)
> 
> Kevin, can you enlighten us?
> 

That's a very good point. It does seem like this should be something
we resolve with rpcbind instead of locally.

This sure would be simpler if we could just depend on having the port
in the upcall, but I guess that would break backward compatibility.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Kevin Coffman]

On Mon, Apr 6, 2009 at 11:46 AM, Chuck Lever <chuck.lever@oracle.com> w=
rote:
> On Apr 6, 2009, at 11:21 AM, Jeff Layton wrote:
>>
>> On Mon, 6 Apr 2009 11:03:11 -0400
>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>> On Apr 3, 2009, at 3:04 PM, Jeff Layton wrote:
>>>>
>>>> On Wed, 1 Apr 2009 14:01:30 -0400
>>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>>> As far as I understand it, the ai_socktype and ai_protocol fields=
 are
>>>>> used to return the values needed for subsequent socket(2)/bind(2)
>>>>> system calls. =A0In this case you are not using these fields from=
 the
>>>>> results...
>>>>>
>>>>> If ai_protocol is zero, then getaddrinfo(3) assumes you want one =
copy
>>>>> of the address for each supported protocol type, so it returns th=
ree
>>>>> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one wit=
h a
>>>>> zero protocol number). =A0The contents, except for the socktype a=
nd
>>>>> protocol fields, are the same for each.
>>>>
>>>> Hypothetical situation...
>>>>
>>>> Suppose there is a service in /etc/services that has a different p=
ort
>>>> number for tcp than for udp:
>>>>
>>>> fooserv =A0 =A0 =A0 =A0 50001/tcp
>>>> fooserv =A0 =A0 =A0 =A0 50002/udp
>>>>
>>>> You're saying that getaddrinfo will return the same port number in=
 all
>>>> of the returned structures? Won't that mean that one of the port
>>>> numbers
>>>> is wrong? That seems broken if so...
>>>
>>> I was trying to describe observed behavior here -- it's pretty
>>> unlikely that there will be different port numbers in these returne=
d
>>> structures. =A0It's difficult to say precisely how this is supposed=
 to
>>> behave based on the man page or even browsing the glibc source code
>>> for a few minutes.
>>>
>>> It's certainly possible to set up /etc/services as you suggest, but
>>> there is an IANA policy to assign the same port for both transports=
=2E
>>> As near as I can tell the reason we have the transport listed in /e=
tc/
>>> services at all is because some protocols support only one transpor=
t.
>>> So IMO it would be quite unlikely to encounter such a case where th=
e
>>> port number depended on the transport.
>>>
>>>> If that's not the case, then I think we need to at least set the
>>>> ai_protocol in the hints.
>>>
>>> Perhaps that's true. =A0What are the expected values of @service ?
>>
>> I've only ever seen "nfs" here, but I guess you could use this with
>> others. Maybe "nfsacl" too?
>
> What's even stranger is that one is supposed to use rpcbind to figure=
 out
> what NFS port to use, not /etc/services.
>
>> IANA might not set different port like this, but there's nothing
>> that stops someone from doing this at their site.
>
> True, but one might expect that setting the NFS service port via rpc.=
nfsd on
> the server would make gssd use that port too. =A0I guess that's why t=
he port
> number is now passed up from the kernel.
>
>> I agree that it's an unlikely (and somewhat pathological) situation,
>> but it's easy to deal with -- just set the protocol in the hints. I'=
ve
>> confirmed that it makes getaddrinfo do the right thing.
>
> That's good. =A0I think we need to understand exactly what this is su=
pposed to
> do, though. =A0Should we use an rpcbind call here instead? =A0This is=
 _rpc_
> gssd, after all. =A0If it's OK as it stands, then I think some commen=
ts about
> why this works this way are in order :-)
>
> Kevin, can you enlighten us?

Here is what I recall.  For v4 we are not supposed to need a
portmap/rpcbind call anymore (or at least an "extra" one from gssd).
So for cases where we don't get a port number in the upcall, we use
the normal port for "nfs".  If we get a port number in the upcall, we
use it.  (That may be the normal port, or a different one.  I'm not
sure where that value comes from.)

I recall there were complaints about the portmap call.  That resulted
in:  http://osdir.com/ml/linux.nfsv4/2005-12/msg00025.html

HTH a little?

K.C.

Re: [PATCH 3/6] nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need port

[Chuck Lever]

On Apr 6, 2009, at 6:33 PM, Kevin Coffman wrote:
> On Mon, Apr 6, 2009 at 11:46 AM, Chuck Lever  
> <chuck.lever@oracle.com> wrote:
>> On Apr 6, 2009, at 11:21 AM, Jeff Layton wrote:
>>>
>>> On Mon, 6 Apr 2009 11:03:11 -0400
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>>
>>>> On Apr 3, 2009, at 3:04 PM, Jeff Layton wrote:
>>>>>
>>>>> On Wed, 1 Apr 2009 14:01:30 -0400
>>>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>
>>>>>> As far as I understand it, the ai_socktype and ai_protocol  
>>>>>> fields are
>>>>>> used to return the values needed for subsequent socket(2)/bind(2)
>>>>>> system calls.  In this case you are not using these fields from  
>>>>>> the
>>>>>> results...
>>>>>>
>>>>>> If ai_protocol is zero, then getaddrinfo(3) assumes you want  
>>>>>> one copy
>>>>>> of the address for each supported protocol type, so it returns  
>>>>>> three
>>>>>> structures (one for IPPROTO_UDP, one for IPPROTO_TCP, and one  
>>>>>> with a
>>>>>> zero protocol number).  The contents, except for the socktype and
>>>>>> protocol fields, are the same for each.
>>>>>
>>>>> Hypothetical situation...
>>>>>
>>>>> Suppose there is a service in /etc/services that has a different  
>>>>> port
>>>>> number for tcp than for udp:
>>>>>
>>>>> fooserv         50001/tcp
>>>>> fooserv         50002/udp
>>>>>
>>>>> You're saying that getaddrinfo will return the same port number  
>>>>> in all
>>>>> of the returned structures? Won't that mean that one of the port
>>>>> numbers
>>>>> is wrong? That seems broken if so...
>>>>
>>>> I was trying to describe observed behavior here -- it's pretty
>>>> unlikely that there will be different port numbers in these  
>>>> returned
>>>> structures.  It's difficult to say precisely how this is supposed  
>>>> to
>>>> behave based on the man page or even browsing the glibc source code
>>>> for a few minutes.
>>>>
>>>> It's certainly possible to set up /etc/services as you suggest, but
>>>> there is an IANA policy to assign the same port for both  
>>>> transports.
>>>> As near as I can tell the reason we have the transport listed in / 
>>>> etc/
>>>> services at all is because some protocols support only one  
>>>> transport.
>>>> So IMO it would be quite unlikely to encounter such a case where  
>>>> the
>>>> port number depended on the transport.
>>>>
>>>>> If that's not the case, then I think we need to at least set the
>>>>> ai_protocol in the hints.
>>>>
>>>> Perhaps that's true.  What are the expected values of @service ?
>>>
>>> I've only ever seen "nfs" here, but I guess you could use this with
>>> others. Maybe "nfsacl" too?
>>
>> What's even stranger is that one is supposed to use rpcbind to  
>> figure out
>> what NFS port to use, not /etc/services.
>>
>>> IANA might not set different port like this, but there's nothing
>>> that stops someone from doing this at their site.
>>
>> True, but one might expect that setting the NFS service port via  
>> rpc.nfsd on
>> the server would make gssd use that port too.  I guess that's why  
>> the port
>> number is now passed up from the kernel.
>>
>>> I agree that it's an unlikely (and somewhat pathological) situation,
>>> but it's easy to deal with -- just set the protocol in the hints.  
>>> I've
>>> confirmed that it makes getaddrinfo do the right thing.
>>
>> That's good.  I think we need to understand exactly what this is  
>> supposed to
>> do, though.  Should we use an rpcbind call here instead?  This is  
>> _rpc_
>> gssd, after all.  If it's OK as it stands, then I think some  
>> comments about
>> why this works this way are in order :-)
>>
>> Kevin, can you enlighten us?
>
> Here is what I recall.  For v4 we are not supposed to need a
> portmap/rpcbind call anymore (or at least an "extra" one from gssd).
> So for cases where we don't get a port number in the upcall, we use
> the normal port for "nfs".  If we get a port number in the upcall, we
> use it.  (That may be the normal port, or a different one.  I'm not
> sure where that value comes from.)

I don't have any experience with rpcgss, but I would think using /etc/ 
services here is incorrect.  If the kernel doesn't provide a port  
number, then gssd should always do an rpcbind against the server  
instead of trusting what's in the client's /etc/services.  We are  
creating a separate RPC client and transport here, after all.

I also think the kernel should pass up the value of the port= mount  
option.  If that mount option wasn't specified, the value should be  
zero for NFSv2/v3 mounts (to force an rpcbind) and 2049 for NFSv4  
mounts (to squelch the rpcbind).

The two questions we have now are:

1.  where does the kernel get the port value passed in the upcall?  is  
it set only if the mount command line had "port=?"  Does the passed-in  
port value change over the life of the mount?

2.  if a zero port is passed up to gssd, shouldn't it always do an  
rpcbind?

> I recall there were complaints about the portmap call.  That resulted
> in:  http://osdir.com/ml/linux.nfsv4/2005-12/msg00025.html
>
> HTH a little?
>
> K.C.

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

[PATCH 4/6] nfs-utils: split out gssd rpc client creation into separate function

[Jeff Layton]

Abstract out the actual RPC client creation functions. This will allow
us to create a new TI-RPC enabled client creation function in a later
patch.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |   80 +++++++++++++++++++++++++++++------------------
 1 files changed, 49 insertions(+), 31 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 4d54c40..b88b0c6 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -537,6 +537,52 @@ out_err:
 	return -1;
 }
 
+static CLIENT *
+create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
+{
+	CLIENT			*rpc_clnt = NULL;
+	char			rpc_errmsg[1024];
+	int			sockp = RPC_ANYSOCK;
+	int			sendsz = 32768, recvsz = 32768;
+	const struct timeval 	timeout = {5, 0};
+
+	switch(protocol) {
+	case IPPROTO_TCP:
+		if ((rpc_clnt = clnttcp_create(
+					(struct sockaddr_in *) &clp->addr,
+					clp->prog, clp->vers, &sockp,
+					sendsz, recvsz)) == NULL) {
+			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
+				 "WARNING: can't create tcp rpc_clnt "
+				 "for server %s for user with uid %d",
+				 clp->servername, uid);
+			printerr(0, "%s\n",
+				 clnt_spcreateerror(rpc_errmsg));
+		}
+		break;
+	case IPPROTO_UDP:
+		if ((rpc_clnt = clntudp_bufcreate(
+					(struct sockaddr_in *) &clp->addr,
+					clp->prog, clp->vers, timeout,
+					&sockp, sendsz, recvsz)) == NULL) {
+			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
+				 "WARNING: can't create udp rpc_clnt "
+				 "for server %s for user with uid %d",
+				 clp->servername, uid);
+			printerr(0, "%s\n",
+				 clnt_spcreateerror(rpc_errmsg));
+		}
+		break;
+	default:
+		/* Shouldn't happen! */
+		printerr(0, "ERROR: requested protocol '%s', but "
+			 "got addrinfo with protocol %d\n",
+			 clp->protocol, protocol);
+	}
+
+	return rpc_clnt;
+}
+
 /*
  * Determine the port from the servicename and set the right field in the
  * sockaddr. This is mostly a no-op with newer kernels that send the port
@@ -623,9 +669,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
 	uid_t			save_uid = -1;
 	int			retval = -1;
 	OM_uint32		min_stat;
-	char			rpc_errmsg[1024];
-	int			sockp = RPC_ANYSOCK;
-	int			sendsz = 32768, recvsz = 32768;
 	int			socktype, protocol;
 
 	/* Create the context as the user (not as root) */
@@ -697,34 +740,9 @@ int create_auth_rpc_client(struct clnt_info *clp,
 			   socktype, protocol))
 		goto out_fail;
 
-	if (protocol == IPPROTO_TCP) {
-		if ((rpc_clnt = clnttcp_create(
-					(struct sockaddr_in *) &clp->addr,
-					clp->prog, clp->vers, &sockp,
-					sendsz, recvsz)) == NULL) {
-			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
-				 "WARNING: can't create tcp rpc_clnt "
-				 "for server %s for user with uid %d",
-				 clp->servername, uid);
-			printerr(0, "%s\n",
-				 clnt_spcreateerror(rpc_errmsg));
-			goto out_fail;
-		}
-	} else if (protocol == IPPROTO_UDP) {
-		const struct timeval timeout = {5, 0};
-		if ((rpc_clnt = clntudp_bufcreate(
-					(struct sockaddr_in *) &clp->addr,
-					clp->prog, clp->vers, timeout,
-					&sockp, sendsz, recvsz)) == NULL) {
-			snprintf(rpc_errmsg, sizeof(rpc_errmsg),
-				 "WARNING: can't create udp rpc_clnt "
-				 "for server %s for user with uid %d",
-				 clp->servername, uid);
-			printerr(0, "%s\n",
-				 clnt_spcreateerror(rpc_errmsg));
-			goto out_fail;
-		}
-	}
+	rpc_clnt = create_rpc_client(clp, protocol, uid);
+	if (!rpc_clnt)
+		goto out_fail;
 
 	printerr(2, "creating context with server %s\n", clp->servicename);
 	auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
-- 
1.6.0.6

[PATCH 5/6] nfs-utils: when TIRPC is enabled, use new API to create RPC client

[Jeff Layton]

When we add IPv6 code, we'll need to use the newer API to create
the rpc client. Add a new rpc client creation function that's
conditionally compiled in whenever TIRPC is enabled.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index b88b0c6..2d7b58d 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -537,6 +537,81 @@ out_err:
 	return -1;
 }
 
+#ifdef HAVE_LIBTIRPC
+static CLIENT *
+create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
+{
+	CLIENT			*rpc_clnt = NULL;
+	char			rpc_errmsg[1024];
+	int			sendsz = 32768, recvsz = 32768;
+	struct netconfig	*nc = NULL;
+	struct netbuf		nb;
+	const struct timeval	timeout = {5, 0};
+	void 			*handle = NULL;
+	char			*nc_protofmly;
+	struct protoent		*proto;
+	sa_family_t		af = ((struct sockaddr *) &clp->addr)->sa_family;
+
+	proto = getprotobynumber(protocol);
+	if (proto == NULL) {
+		printerr(0, "ERROR: can't resolve %d to protocol\n",
+			 protocol);
+		goto out;
+	}
+
+	switch(af) {
+	case AF_INET:
+		nc_protofmly = NC_INET;
+		nb.len = nb.maxlen = sizeof(struct sockaddr_in);
+		break;
+	default:
+		printerr(0, "ERROR: unsupported address family %hd\n", af);
+		goto out;
+	}
+
+	handle = setnetconfig();
+	while ((nc = getnetconfig(handle)) != NULL) {
+
+		if (nc->nc_protofmly != NULL &&
+		    strcmp(nc->nc_protofmly, nc_protofmly) != 0)
+			continue;
+		if (nc->nc_proto != NULL &&
+		    strcmp(nc->nc_proto, proto->p_name) != 0)
+			continue;
+
+		break;
+	}
+
+	if (!nc) {
+		printerr(0, "ERROR: unable to find netconfig entry for "
+			    "addr family %hd and proto %d\n",
+			    af, protocol);
+		goto out;
+	}
+
+	nb.buf = &clp->addr;
+
+	rpc_clnt = clnt_tli_create(RPC_ANYFD, nc, &nb, clp->prog, clp->vers,
+				   sendsz, recvsz);
+	if (!rpc_clnt) {
+		snprintf(rpc_errmsg, sizeof(rpc_errmsg),
+			 "ERROR: unable to create %s RPC client for %s with "
+			 "uid %d", clp->protocol, clp->servername, uid);
+		printerr(0, "%s\n", clnt_spcreateerror(rpc_errmsg));
+		goto out;
+	}
+
+	/* set retry timeout for connectionless transports */
+	if (nc->nc_semantics == NC_TPI_CLTS)
+		clnt_control(rpc_clnt, CLSET_RETRY_TIMEOUT, (void *) &timeout);
+out:
+	if (handle)
+		endnetconfig(handle);
+	return rpc_clnt;
+}
+
+#else  /* HAVE_LIBTIRPC */
+
 static CLIENT *
 create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
 {
@@ -582,6 +657,7 @@ create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
 
 	return rpc_clnt;
 }
+#endif /* HAVE_LIBTIRPC */
 
 /*
  * Determine the port from the servicename and set the right field in the
-- 
1.6.0.6

Re: [PATCH 5/6] nfs-utils: when TIRPC is enabled, use new API to create RPC client

[Chuck Lever]

On Apr 1, 2009, at 10:24 AM, Jeff Layton wrote:

> When we add IPv6 code, we'll need to use the newer API to create
> the rpc client. Add a new rpc client creation function that's
> conditionally compiled in whenever TIRPC is enabled.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> utils/gssd/gssd_proc.c |   76 +++++++++++++++++++++++++++++++++++++++ 
> +++++++++
> 1 files changed, 76 insertions(+), 0 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index b88b0c6..2d7b58d 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -537,6 +537,81 @@ out_err:
> 	return -1;
> }
>
> +#ifdef HAVE_LIBTIRPC
> +static CLIENT *
> +create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
> +{
> +	CLIENT			*rpc_clnt = NULL;
> +	char			rpc_errmsg[1024];
> +	int			sendsz = 32768, recvsz = 32768;
> +	struct netconfig	*nc = NULL;
> +	struct netbuf		nb;
> +	const struct timeval	timeout = {5, 0};
> +	void 			*handle = NULL;
> +	char			*nc_protofmly;
> +	struct protoent		*proto;
> +	sa_family_t		af = ((struct sockaddr *) &clp->addr)->sa_family;
> +
> +	proto = getprotobynumber(protocol);
> +	if (proto == NULL) {
> +		printerr(0, "ERROR: can't resolve %d to protocol\n",
> +			 protocol);
> +		goto out;
> +	}
> +
> +	switch(af) {
> +	case AF_INET:
> +		nc_protofmly = NC_INET;
> +		nb.len = nb.maxlen = sizeof(struct sockaddr_in);
> +		break;
> +	default:
> +		printerr(0, "ERROR: unsupported address family %hd\n", af);
> +		goto out;
> +	}
> +
> +	handle = setnetconfig();
> +	while ((nc = getnetconfig(handle)) != NULL) {
> +
> +		if (nc->nc_protofmly != NULL &&
> +		    strcmp(nc->nc_protofmly, nc_protofmly) != 0)
> +			continue;
> +		if (nc->nc_proto != NULL &&
> +		    strcmp(nc->nc_proto, proto->p_name) != 0)
> +			continue;
> +
> +		break;
> +	}
> +
> +	if (!nc) {
> +		printerr(0, "ERROR: unable to find netconfig entry for "
> +			    "addr family %hd and proto %d\n",
> +			    af, protocol);
> +		goto out;
> +	}
> +
> +	nb.buf = &clp->addr;
> +
> +	rpc_clnt = clnt_tli_create(RPC_ANYFD, nc, &nb, clp->prog, clp->vers,
> +				   sendsz, recvsz);

Can you use (or modify slightly) nfs_get_rpcclient()?  That would  
probably work for both TI-RPC and legacy, and you wouldn't have to  
open-code all the nconf junk.

> +	if (!rpc_clnt) {
> +		snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> +			 "ERROR: unable to create %s RPC client for %s with "
> +			 "uid %d", clp->protocol, clp->servername, uid);
> +		printerr(0, "%s\n", clnt_spcreateerror(rpc_errmsg));
> +		goto out;
> +	}
> +
> +	/* set retry timeout for connectionless transports */
> +	if (nc->nc_semantics == NC_TPI_CLTS)
> +		clnt_control(rpc_clnt, CLSET_RETRY_TIMEOUT, (void *) &timeout);
> +out:
> +	if (handle)
> +		endnetconfig(handle);
> +	return rpc_clnt;
> +}
> +
> +#else  /* HAVE_LIBTIRPC */
> +
> static CLIENT *
> create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
> {
> @@ -582,6 +657,7 @@ create_rpc_client(struct clnt_info *clp, int  
> protocol, uid_t uid)
>
> 	return rpc_clnt;
> }
> +#endif /* HAVE_LIBTIRPC */
>
> /*
>  * Determine the port from the servicename and set the right field  
> in the

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

[PATCH 6/6] nfs-utils: add IPv6 code to gssd

[Jeff Layton]

All of the pieces to handle IPv6 are now in place. Add IPv6-specific
code wrapped in the proper #ifdef's so that IPv6 support works when
it's enabled at build-time.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 2d7b58d..93908a3 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -110,10 +110,16 @@ static int
 addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const int port)
 {
 	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+	struct sockaddr_in6	*s6 = (struct sockaddr_in6 *) sa;
 
 	if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
 		s4->sin_family = AF_INET;
 		s4->sin_port = htons(port);
+#ifdef IPV6_SUPPORTED
+	} else if (inet_pton(AF_INET6, addr, &s6->sin6_addr)) {
+		s6->sin6_family = AF_INET6;
+		s6->sin6_port = htons(port);
+#endif /* IPV6_SUPPORTED */
 	} else {
 		printerr(0, "ERROR: unable to convert %s to address\n", addr);
 		return 0;
@@ -137,6 +143,11 @@ sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
 	case AF_INET:
 		addrlen = sizeof(struct sockaddr_in);
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		addrlen = sizeof(struct sockaddr_in6);
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		printerr(0, "ERROR: unrecognized addr family %d\n",
 			 sa->sa_family);
@@ -564,6 +575,12 @@ create_rpc_client(struct clnt_info *clp, int protocol, uid_t uid)
 		nc_protofmly = NC_INET;
 		nb.len = nb.maxlen = sizeof(struct sockaddr_in);
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		nc_protofmly = NC_INET6;
+		nb.len = nb.maxlen = sizeof(struct sockaddr_in6);
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		printerr(0, "ERROR: unsupported address family %hd\n", af);
 		goto out;
@@ -669,6 +686,7 @@ populate_port(struct sockaddr *sa, char *servicename, int socktype,
 	      int protocol)
 {
 	struct sockaddr_in	*s4 = (struct sockaddr_in *) sa;
+	struct sockaddr_in6	*s6 = (struct sockaddr_in6 *) sa;
 	char			node[INET_ADDRSTRLEN];
 	char			service[64];
 	char			*at_sign;
@@ -690,6 +708,18 @@ populate_port(struct sockaddr *sa, char *servicename, int socktype,
 				sizeof(struct sockaddr_in)))
 			return 0;
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		if (s6->sin6_port != 0) {
+			printerr(2, "DEBUG: port already set to %d\n",
+				 ntohs(s6->sin6_port));
+			return 1;
+		}
+		if (!inet_ntop(AF_INET6, &s6->sin6_addr, node,
+				sizeof(struct sockaddr_in6)))
+			return 0;
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		printerr(0, "ERROR: unsupported address family %d\n",
 			    sa->sa_family);
@@ -721,6 +751,10 @@ populate_port(struct sockaddr *sa, char *servicename, int socktype,
 
 	if (a->ai_family == AF_INET)
 		s4->sin_port = ((struct sockaddr_in *) a->ai_addr)->sin_port;
+#ifdef IPV6_SUPPORTED
+	else if (a->ai_family == AF_INET6)
+		s6->sin6_port = ((struct sockaddr_in6 *) a->ai_addr)->sin6_port;
+#endif /* IPV6_SUPPORTED */
 	else
 		printerr(0, "ERROR: unrecognized address family %d returned "
 			    "by getaddrinfo.\n", a->ai_family);
-- 
1.6.0.6

Re: [PATCH 0/6] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #2)

[Steve Dickson]

I believe Patch 1/6 is missing from this patch set,
at least I can't seem to find it.... 

Also, is this the final version? Or will there be another
one addressing Chuck's issues.

steved.

Jeff Layton wrote:
> This patchset is a second attempt at adding support for IPv6 to
> rpc.gssd. The main changes from the first set are some minor fixes and
> changes suggested by Chuck Lever. This set is also based on mainline
> nfs-utils since the requisite patches for it are now there.
> 
> In order to do this, we first have to convert rpc.gssd to use TI-RPC.
> The first 5 patches do this and abstract out some of the code to make
> this easier. Once that support is in place, the last patch in the series
> actually adds the (fairly minimal) changes to add IPv6 support.
> 
> The series should be fully bisectable, but I've only really tested the
> end result for anything other than to see if it compiles. With these
> patches I've been able to mount an OpenSolaris server using
> NFSv3/4 + IPv6 + krb5 auth
> 
> Specific changes from the last set:
> - cleaned up the usage of sockaddr and sockaddr_storage and pointers to
>   them
> - moved the sockaddr_storage to be part of the clnt_info instead of a
>   separate allocation
> - use a C99 initializer for ai_hints instead of memset()
> - properly handle a return of EAI_SYSTEM from getaddrinfo()
> 
> Comments welcome.
> 
> Jeff Layton (6):
>   nfs-utils: make getnameinfo() required for --enable-gss
>   nfs-utils: store the address given in the upcall for later use
>   nfs-utils: skip getaddrinfo in create_auth_rpc_client unless we need
>     port
>   nfs-utils: split out gssd rpc client creation into separate function
>   nfs-utils: when TIRPC is enabled, use new API to create RPC client
>   nfs-utils: add IPv6 code to gssd
> 
>  configure.ac           |    2 +
>  utils/gssd/gssd.h      |    2 +-
>  utils/gssd/gssd_proc.c |  398 +++++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 309 insertions(+), 93 deletions(-)
> 
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH 0/6] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #2)

[Jeff Layton]

On Fri, 03 Apr 2009 12:07:18 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> I believe Patch 1/6 is missing from this patch set,
> at least I can't seem to find it.... 
> 
> Also, is this the final version? Or will there be another
> one addressing Chuck's issues.
> 
> steved.
> 

Yeah, Time-warner cable's mail servers ate about half the series (grrrr).

There'll be another set coming to address Chuck's comments. It'll be a
little while before I have a chance to work through them all though.

-- 
Jeff Layton <jlayton@redhat.com>