[PATCH 0/6] nfs-utils: add IPv6 support for rpc.nfsd (try #4)

[PATCH 0/6] nfs-utils: add IPv6 support for rpc.nfsd (try #4)

[Jeff Layton]

This is the fourth attempt at a patchset to add IPv6 support to the
rpc.nfsd program. This set is quite a bit different from the earlier
ones. The main differences are:

1) it first walks /etc/netconfig to determine the protocols and address
families to enable. Then disables any specified by command line options.

2) it uses getaddrinfo to generate sockaddrs for sockets that get passed
off to the kernel. This simplifies the code quite a bit and makes for
more robust handling of the -H option.

3) converts nfsd to use xlog logging facility. It also adds a --debug
option that causes it to log to stderr rather than syslog, and makes it
print out all debug messages.

4) in the event that an ipv6-enabled rpc.nfsd is run on a kernel that
doesn't have an ipv6-enabled knfsd, the program doesn't log an error
message if it's able to start nfsd on an IPv4 socket. This should cut
down spurious logging until IPv6 support is more widely available.

There is also a lot of general cleanup and reorganization of the startup
logic. Although this patchset is a bit larger than the prior ones, I
think the result is a much cleaner set of code.

This set should be bisectable, but I've only really tested the final
result. I have tested it on various combinations of build options and
with ipv6.ko blacklisted and it seems to work appropriately in all
cases.

Jeff Layton (6):
  nfs-utils: don't link libexport.a and libmisc.a to nfsd
  nfs-utils: clean up option parsing in nfsd.c
  nfs-utils: break up nfssvc.c into more individually callable
    functions
  nfs-utils: set IPV6_V6ONLY on nfssvc IPv6 sockets
  nfs-utils: add IPv6 support to nfsd
  nfs-utils: update the nfsd manpage

 support/include/nfs/nfs.h |   15 ++-
 support/include/nfslib.h  |    7 +-
 support/nfs/nfssvc.c      |  231 ++++++++++++++++++++++++++++---------------
 utils/nfsd/Makefile.am    |    4 +-
 utils/nfsd/nfsd.c         |  243 +++++++++++++++++++++++++++++++++------------
 utils/nfsd/nfsd.man       |   19 +++-
 6 files changed, 365 insertions(+), 154 deletions(-)

[PATCH 1/6] nfs-utils: don't link libexport.a and libmisc.a to nfsd

[Jeff Layton]

...they aren't needed.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsd/Makefile.am |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/utils/nfsd/Makefile.am b/utils/nfsd/Makefile.am
index 445e3fd..ea85cd5 100644
--- a/utils/nfsd/Makefile.am
+++ b/utils/nfsd/Makefile.am
@@ -8,9 +8,7 @@ KPREFIX		= @kprefix@
 sbin_PROGRAMS	= nfsd
 
 nfsd_SOURCES = nfsd.c
-nfsd_LDADD = ../../support/export/libexport.a \
-	     ../../support/nfs/libnfs.a \
-	     ../../support/misc/libmisc.a
+nfsd_LDADD = ../../support/nfs/libnfs.a
 
 MAINTAINERCLEANFILES = Makefile.in
 
-- 
1.6.0.6

[PATCH 2/6] nfs-utils: clean up option parsing in nfsd.c

[Jeff Layton]

Minor formatting nits.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsd/nfsd.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index e3c0094..c7bd003 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -97,11 +97,11 @@ main(int argc, char **argv)
 			}
 			break;
 		case 'T':
-				NFSCTL_TCPUNSET(protobits);
-				break;
+			NFSCTL_TCPUNSET(protobits);
+			break;
 		case 'U':
-				NFSCTL_UDPUNSET(protobits);
-				break;
+			NFSCTL_UDPUNSET(protobits);
+			break;
 		default:
 			fprintf(stderr, "Invalid argument: '%c'\n", c);
 		case 'h':
-- 
1.6.0.6

[PATCH 3/6] nfs-utils: break up nfssvc.c into more individually callable functions

[Jeff Layton]

nfssvc.c contains functions for starting up knfsd. Currently, the only
non-static function in that file is nfssvc(). In order to add IPv6
support, we'll need to be able to call some of these functions in a more
granular fashion.

Reorganize these functions and add prototypes to the header so that they
can be called individually, and change the nfsd program to call those
routines individually.

Change nfssvc_setfds to take a different set of args and change it to
use getaddrinfo to look up addresses. This simplifies the code in the
core nfsd program significantly and should make adding IPv6 support
easier.

Finally, change nfsd to use xlog routines for logging and add a --debug
switch to enable sending output to stderr rather than syslog.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 support/include/nfslib.h |    7 ++-
 support/nfs/nfssvc.c     |  226 ++++++++++++++++++++++++++++++----------------
 utils/nfsd/nfsd.c        |  109 ++++++++++++++---------
 3 files changed, 219 insertions(+), 123 deletions(-)

diff --git a/support/include/nfslib.h b/support/include/nfslib.h
index ae98650..fe24fe3 100644
--- a/support/include/nfslib.h
+++ b/support/include/nfslib.h
@@ -20,6 +20,7 @@
 #include <paths.h>
 #include <rpcsvc/nfs_prot.h>
 #include <nfs/nfs.h>
+#include <netdb.h>
 #include "xlog.h"
 
 #ifndef _PATH_EXPORTS
@@ -130,7 +131,11 @@ int			wildmat(char *text, char *pattern);
  * nfsd library functions.
  */
 int			nfsctl(int, struct nfsctl_arg *, union nfsctl_res *);
-int			nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4, unsigned int portbits, char *haddr);
+int			nfssvc_inuse(void);
+int			nfssvc_setfds(const struct addrinfo *hints, const char *node,
+					const char *port);
+void			nfssvc_setvers(unsigned int ctlbits, int minorvers4);
+int			nfssvc_threads(unsigned short port, int nrservs);
 int			nfsaddclient(struct nfsctl_client *clp);
 int			nfsdelclient(struct nfsctl_client *clp);
 int			nfsexport(struct nfsctl_export *exp);
diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 33c15a7..e7f3262 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -10,113 +10,179 @@
 #include <config.h>
 #endif
 
+#include <sys/types.h>
 #include <sys/socket.h>
+#include <netdb.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <errno.h>
-#include <syslog.h>
-
 
 #include "nfslib.h"
+#include "xlog.h"
 
 #define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
 #define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
 #define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
 
-static void
-nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
+/*
+ * Are there already sockets configured? If not, then it is safe to try to
+ * open some and pass them through.
+ *
+ * Note: If the user explicitly asked for 'udp', then we should probably check
+ * if that is open, and should open it if not. However we don't yet. All
+ * sockets have to be opened when the first daemon is started.
+ */
+int
+nfssvc_inuse(void)
 {
-	int fd, n, on=1;
+	int fd, n;
 	char buf[BUFSIZ];
-	int udpfd = -1, tcpfd = -1;
-	struct sockaddr_in sin;
 
 	fd = open(NFSD_PORTS_FILE, O_RDONLY);
+
+	/* problem opening file, assume that nothing is configured */
 	if (fd < 0)
-		return;
+		return 0;
+
 	n = read(fd, buf, BUFSIZ);
 	close(fd);
+
 	if (n != 0)
-		return;
-	/* there are no ports currently open, so it is safe to
-	 * try to open some and pass them through.
-	 * Note: If the user explicitly asked for 'udp', then
-	 * we should probably check if that is open, and should
-	 * open it if not.  However we don't yet.  All sockets
-	 * have to be opened when the first daemon is started.
+		return 1;
+
+	return 0;
+}
+
+int
+nfssvc_setfds(const struct addrinfo *hints, const char *node, const char *port)
+{
+	int fd, on = 1, fac = L_ERROR;
+	int sockfd = -1, rc = 0;
+	char buf[BUFSIZ];
+	struct addrinfo *addrhead = NULL, *addr;
+	char *proto, *family;
+
+	/*
+	 * if file can't be opened, then assume that it's not available and
+	 * that the caller should just fall back to the old nfsctl interface
 	 */
 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
 	if (fd < 0)
-		return;
-	sin.sin_family = AF_INET;
-	sin.sin_port   = htons(port);
-	sin.sin_addr.s_addr =  inet_addr(haddr);
-
-	if (NFSCTL_UDPISSET(ctlbits)) {
-		udpfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
-		if (udpfd < 0) {
-			syslog(LOG_ERR, "nfssvc: unable to create UPD socket: "
-				"errno %d (%s)\n", errno, strerror(errno));
-			exit(1);
-		}
-		if (bind(udpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
-			syslog(LOG_ERR, "nfssvc: unable to bind UPD socket: "
-				"errno %d (%s)\n", errno, strerror(errno));
-			exit(1);
-		}
+		return 0;
+
+	switch(hints->ai_family) {
+	case AF_INET:
+		family = "inet";
+		break;
+	case AF_INET6:
+		family = "inet6";
+		break;
+	default:
+		/* FIXME: should never happen -- return error here? */
+		family = "unknown family";
+	}
+
+	rc = getaddrinfo(node, port, hints, &addrhead);
+	if (rc == EAI_NONAME && !strcmp(port, "nfs"))
+		rc = getaddrinfo(node, "2049", hints, &addrhead);
+
+	if (rc != 0) {
+		xlog(L_ERROR, "unable to resolve %s:%s to %s address: "
+				"%s", node ? node : "ANYADDR", port, family,
+				rc == EAI_SYSTEM ? strerror(errno) :
+					gai_strerror(rc));
+		goto error;
 	}
 
-	if (NFSCTL_TCPISSET(ctlbits)) {
-		tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
-		if (tcpfd < 0) {
-			syslog(LOG_ERR, "nfssvc: unable to createt tcp socket: "
-				"errno %d (%s)\n", errno, strerror(errno));
-			exit(1);
+	addr = addrhead;
+	while(addr) {
+		/* skip non-TCP / non-UDP sockets */
+		switch(addr->ai_protocol) {
+		case IPPROTO_UDP:
+			proto = "UDP";
+			break;
+		case IPPROTO_TCP:
+			proto = "TCP";
+			break;
+		default:
+			addr = addr->ai_next;
+			continue;
 		}
-		if (setsockopt(tcpfd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) < 0) {
-			syslog(LOG_ERR, "nfssvc: unable to set SO_REUSEADDR: "
-				"errno %d (%s)\n", errno, strerror(errno));
-			exit(1);
+
+		xlog(D_GENERAL, "Creating %s %s socket.", family, proto);
+
+		/* open socket and prepare to hand it off to kernel */
+		sockfd = socket(addr->ai_family, addr->ai_socktype,
+				addr->ai_protocol);
+		if (sockfd < 0) {
+			xlog(L_ERROR, "unable to create %s %s socket: "
+				"errno %d (%s)", family, proto, errno,
+				strerror(errno));
+			rc = errno;
+			goto error;
 		}
-		if (bind(tcpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
-			syslog(LOG_ERR, "nfssvc: unable to bind TCP socket: "
-				"errno %d (%s)\n", errno, strerror(errno));
-			exit(1);
+		if (addr->ai_protocol == IPPROTO_TCP &&
+		    setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on))) {
+			xlog(L_ERROR, "unable to set SO_REUSEADDR on %s "
+				"socket: errno %d (%s)", family, errno,
+				strerror(errno));
+			rc = errno;
+			goto error;
 		}
-		if (listen(tcpfd, 64) < 0){
-			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
-				"errno %d (%s)\n", errno, strerror(errno));
-			exit(1);
+		if (bind(sockfd, addr->ai_addr, addr->ai_addrlen)) {
+			xlog(L_ERROR, "unable to bind %s %s socket: "
+				"errno %d (%s)", family, proto, errno,
+				strerror(errno));
+			rc = errno;
+			goto error;
 		}
-	}
-	if (udpfd >= 0) {
-		snprintf(buf, BUFSIZ,"%d\n", udpfd); 
-		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
-			syslog(LOG_ERR, 
-			       "nfssvc: writing fds to kernel failed: errno %d (%s)", 
-			       errno, strerror(errno));
+		if (addr->ai_protocol == IPPROTO_TCP && listen(sockfd, 64)) {
+			xlog(L_ERROR, "unable to create listening socket: "
+				"errno %d (%s)", errno, strerror(errno));
+			rc = errno;
+			goto error;
 		}
-		close(fd);
-		fd = -1;
-	}
-	if (tcpfd >= 0) {
+
 		if (fd < 0)
 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
-		snprintf(buf, BUFSIZ,"%d\n", tcpfd); 
+
+		if (fd < 0) {
+			xlog(L_ERROR, "couldn't open ports file: errno "
+				      "%d (%s)", errno, strerror(errno));
+			goto error;
+		}
+
+		snprintf(buf, BUFSIZ, "%d\n", sockfd); 
 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
-			syslog(LOG_ERR, 
-			       "nfssvc: writing fds to kernel failed: errno %d (%s)", 
-			       errno, strerror(errno));
+			/*
+			 * this error may be common on older kernels that don't
+			 * support IPv6, so turn into a debug message.
+			 */
+			if (errno == EAFNOSUPPORT)
+				fac = D_ALL;
+			xlog(fac, "writing fd to kernel failed: errno %d (%s)",
+				  errno, strerror(errno));
+			rc = errno;
+			goto error;
 		}
+		close(fd);
+		fd = -1;
+		addr = addr->ai_next;
 	}
-	close(fd);
 
-	return;
+error:
+	if (fd >= 0)
+		close(fd);
+	if (addrhead)
+		freeaddrinfo(addrhead);
+
+	return rc;
 }
-static void
-nfssvc_versbits(unsigned int ctlbits, int minorvers4)
+
+void
+nfssvc_setvers(unsigned int ctlbits, int minorvers4)
 {
 	int fd, n, off;
 	char buf[BUFSIZ], *ptr;
@@ -140,27 +206,21 @@ nfssvc_versbits(unsigned int ctlbits, int minorvers4)
 				    n);
 	snprintf(ptr+off, BUFSIZ - off, "\n");
 	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
-		syslog(LOG_ERR, "nfssvc: Setting version failed: errno %d (%s)", 
+		xlog(L_ERROR, "Setting version failed: errno %d (%s)", 
 			errno, strerror(errno));
 	}
 	close(fd);
 
 	return;
 }
+
 int
-nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
-	unsigned protobits, char *haddr)
+nfssvc_threads(unsigned short port, const int nrservs)
 {
 	struct nfsctl_arg	arg;
+	struct servent *ent;
 	int fd;
 
-	/* Note: must set versions before fds so that
-	 * the ports get registered with portmap against correct
-	 * versions
-	 */
-	nfssvc_versbits(versbits, minorvers4);
-	nfssvc_setfds(port, protobits, haddr);
-
 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
 	if (fd < 0)
 		fd = open("/proc/fs/nfs/threads", O_WRONLY);
@@ -180,6 +240,14 @@ nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
 			return 0;
 	}
 
+	if (!port) {
+		ent = getservbyname ("nfs", "udp");
+		if (ent != NULL)
+			port = ntohs (ent->s_port);
+		else
+			port = 2049;
+	}
+
 	arg.ca_version = NFSCTL_VERSION;
 	arg.ca_svc.svc_nthreads = nrservs;
 	arg.ca_svc.svc_port = port;
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index c7bd003..bdc71db 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -18,13 +18,13 @@
 #include <string.h>
 #include <errno.h>
 #include <getopt.h>
-#include <syslog.h>
 #include <netdb.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
 
 #include "nfslib.h"
+#include "xlog.h"
 
 static void	usage(const char *);
 
@@ -37,48 +37,48 @@ static struct option longopts[] =
 	{ "no-udp", 0, 0, 'U' },
 	{ "port", 1, 0, 'P' },
 	{ "port", 1, 0, 'p' },
+	{ "debug", 0, 0, 'd' },
 	{ NULL, 0, 0, 0 }
 };
 unsigned int protobits = NFSCTL_ALLBITS;
 unsigned int versbits = NFSCTL_ALLBITS;
 int minorvers4 = NFSD_MAXMINORVERS4;		/* nfsv4 minor version */
-char *haddr = NULL;
 
 int
 main(int argc, char **argv)
 {
-	int	count = 1, c, error, port, fd, found_one;
-	struct servent *ent;
-	struct hostent *hp;
-	char *p;
-
-	ent = getservbyname ("nfs", "udp");
-	if (ent != NULL)
-		port = ntohs (ent->s_port);
-	else
-		port = 2049;
-
-	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts, NULL)) != EOF) {
+	int	count = 1, c, error, portnum = 0, fd, found_one;
+	char *p, *port = "nfs";
+	char *haddr = NULL;
+	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
+
+	xlog_syslog(1);
+	xlog_stderr(0);
+
+	while ((c = getopt_long(argc, argv, "dH:hN:p:P:TU", longopts, NULL)) != EOF) {
 		switch(c) {
+		case 'd':
+			xlog_config(D_ALL, 1);
+			xlog_syslog(0);
+			xlog_stderr(1);
+			break;
 		case 'H':
-			if (inet_addr(optarg) != INADDR_NONE) {
+			/*
+			 * for now, this only handles one -H option. Use the
+			 * first one specified.
+			 */
+			if (!haddr)
 				haddr = strdup(optarg);
-			} else if ((hp = gethostbyname(optarg)) != NULL) {
-				haddr = inet_ntoa((*(struct in_addr*)(hp->h_addr_list[0])));
-			} else {
-				fprintf(stderr, "%s: Unknown hostname: %s\n",
-					argv[0], optarg);
-				usage(argv [0]);
-			}
 			break;
 		case 'P':	/* XXX for nfs-server compatibility */
 		case 'p':
-			port = atoi(optarg);
-			if (port <= 0 || port > 65535) {
+			portnum = atoi(optarg);
+			if (portnum <= 0 || portnum > 65535) {
 				fprintf(stderr, "%s: bad port number: %s\n",
 					argv[0], optarg);
-				usage(argv [0]);
+				usage(argv[0]);
 			}
+			port = strdup(optarg);
 			break;
 		case 'N':
 			switch((c = strtol(optarg, &p, 0))) {
@@ -108,25 +108,21 @@ main(int argc, char **argv)
 			usage(argv[0]);
 		}
 	}
-	/*
-	 * Do some sanity checking, if the ctlbits are set
-	 */
-	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
-		fprintf(stderr, "invalid protocol specified\n");
-		exit(1);
-	}
+
+	xlog_open("nfsd");
+
 	found_one = 0;
 	for (c = NFSD_MINVERS; c <= NFSD_MAXVERS; c++) {
 		if (NFSCTL_VERISSET(versbits, c))
 			found_one = 1;
 	}
 	if (!found_one) {
-		fprintf(stderr, "no version specified\n");
+		xlog(L_ERROR, "no version specified");
 		exit(1);
 	}			
 
 	if (NFSCTL_VERISSET(versbits, 4) && !NFSCTL_TCPISSET(protobits)) {
-		fprintf(stderr, "version 4 requires the TCP protocol\n");
+		xlog(L_ERROR, "version 4 requires the TCP protocol");
 		exit(1);
 	}
 	if (haddr == NULL) {
@@ -135,24 +131,51 @@ main(int argc, char **argv)
 	}
 
 	if (chdir(NFS_STATEDIR)) {
-		fprintf(stderr, "%s: chdir(%s) failed: %s\n",
-			argv [0], NFS_STATEDIR, strerror(errno));
+		xlog(L_ERROR, "chdir(%s) failed: %s",
+			NFS_STATEDIR, strerror(errno));
 		exit(1);
 	}
 
 	if (optind < argc) {
 		if ((count = atoi(argv[optind])) < 0) {
 			/* insane # of servers */
-			fprintf(stderr,
-				"%s: invalid server count (%d), using 1\n",
+			xlog(L_ERROR,
+				"invalid server count (%d), using 1",
 				argv[0], count);
 			count = 1;
 		}
 	}
+
+	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
+		xlog(L_ERROR, "invalid protocol specified");
+		exit(1);
+	}
+
+	if (!NFSCTL_UDPISSET(protobits))
+		hints.ai_protocol = IPPROTO_TCP;
+	else if (!NFSCTL_TCPISSET(protobits))
+		hints.ai_protocol = IPPROTO_UDP;
+
+	hints.ai_family = AF_INET;
+
+	/*
+	 * must set versions before the fd's so that the right versions get
+	 * registered with rpcbind. Note that on older kernels w/o the right
+	 * interfaces, these are a no-op.
+	 */
+	if (!nfssvc_inuse()) {
+		nfssvc_setvers(versbits, minorvers4);
+		error = nfssvc_setfds(&hints, haddr, port);
+		if (error)
+			goto out;
+	}
+
 	/* KLUDGE ALERT:
 	   Some kernels let nfsd kernel threads inherit open files
 	   from the program that spawns them (i.e. us).  So close
 	   everything before spawning kernel threads.  --Chip */
+	xlog_syslog(1);
+	xlog_stderr(0);
 	fd = open("/dev/null", O_RDWR);
 	if (fd == -1)
 		perror("/dev/null");
@@ -163,13 +186,13 @@ main(int argc, char **argv)
 	}
 	closeall(3);
 
-	openlog("nfsd", LOG_PID, LOG_DAEMON);
-	if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0) {
+	if ((error = nfssvc_threads(portnum, count)) < 0) {
 		int e = errno;
-		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
-		closelog();
+		xlog(L_ERROR, "error starting threads: %s", strerror(e));
 	}
 
+out:
+	free(haddr);
 	return (error != 0);
 }
 
@@ -177,7 +200,7 @@ static void
 usage(const char *prog)
 {
 	fprintf(stderr, "Usage:\n"
-		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-T|--no-tcp] [-U|--no-udp] nrservs\n", 
+		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-T|--no-tcp] [-U|--no-udp] [-d|--debug] nrservs\n", 
 		prog);
 	exit(2);
 }
-- 
1.6.0.6

Re: [PATCH 3/6] nfs-utils: break up nfssvc.c into more individually callable functions

[Chuck Lever]

Some cursory comments.  I think Steve should take the first two in  
this series now, as they are simple and reasonable clean-ups.

On Jun 2, 2009, at 7:43 AM, Jeff Layton wrote:

> nfssvc.c contains functions for starting up knfsd. Currently, the only
> non-static function in that file is nfssvc(). In order to add IPv6
> support, we'll need to be able to call some of these functions in a  
> more
> granular fashion.
>
> Reorganize these functions and add prototypes to the header so that  
> they
> can be called individually, and change the nfsd program to call those
> routines individually.
>
> Change nfssvc_setfds to take a different set of args and change it to
> use getaddrinfo to look up addresses. This simplifies the code in the
> core nfsd program significantly and should make adding IPv6 support
> easier.
>
> Finally, change nfsd to use xlog routines for logging and add a -- 
> debug
> switch to enable sending output to stderr rather than syslog.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/include/nfslib.h |    7 ++-
> support/nfs/nfssvc.c     |  226 +++++++++++++++++++++++++++++ 
> +----------------
> utils/nfsd/nfsd.c        |  109 ++++++++++++++---------
> 3 files changed, 219 insertions(+), 123 deletions(-)
>
> diff --git a/support/include/nfslib.h b/support/include/nfslib.h
> index ae98650..fe24fe3 100644
> --- a/support/include/nfslib.h
> +++ b/support/include/nfslib.h
> @@ -20,6 +20,7 @@
> #include <paths.h>
> #include <rpcsvc/nfs_prot.h>
> #include <nfs/nfs.h>
> +#include <netdb.h>
> #include "xlog.h"
>
> #ifndef _PATH_EXPORTS
> @@ -130,7 +131,11 @@ int			wildmat(char *text, char *pattern);
>  * nfsd library functions.
>  */
> int			nfsctl(int, struct nfsctl_arg *, union nfsctl_res *);
> -int			nfssvc(int port, int nrservs, unsigned int versbits, int  
> minorvers4, unsigned int portbits, char *haddr);
> +int			nfssvc_inuse(void);
> +int			nfssvc_setfds(const struct addrinfo *hints, const char *node,
> +					const char *port);
> +void			nfssvc_setvers(unsigned int ctlbits, int minorvers4);
> +int			nfssvc_threads(unsigned short port, int nrservs);
> int			nfsaddclient(struct nfsctl_client *clp);
> int			nfsdelclient(struct nfsctl_client *clp);
> int			nfsexport(struct nfsctl_export *exp);
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 33c15a7..e7f3262 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -10,113 +10,179 @@
> #include <config.h>
> #endif
>
> +#include <sys/types.h>
> #include <sys/socket.h>
> +#include <netdb.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> -#include <syslog.h>
> -
>
> #include "nfslib.h"
> +#include "xlog.h"
>
> #define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> #define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> #define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
>
> -static void
> -nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> +/*
> + * Are there already sockets configured? If not, then it is safe to  
> try to
> + * open some and pass them through.
> + *
> + * Note: If the user explicitly asked for 'udp', then we should  
> probably check
> + * if that is open, and should open it if not. However we don't  
> yet. All
> + * sockets have to be opened when the first daemon is started.
> + */
> +int
> +nfssvc_inuse(void)
> {
> -	int fd, n, on=1;
> +	int fd, n;
> 	char buf[BUFSIZ];
> -	int udpfd = -1, tcpfd = -1;
> -	struct sockaddr_in sin;
>
> 	fd = open(NFSD_PORTS_FILE, O_RDONLY);
> +
> +	/* problem opening file, assume that nothing is configured */
> 	if (fd < 0)
> -		return;
> +		return 0;
> +
> 	n = read(fd, buf, BUFSIZ);
> 	close(fd);
> +
> 	if (n != 0)
> -		return;
> -	/* there are no ports currently open, so it is safe to
> -	 * try to open some and pass them through.
> -	 * Note: If the user explicitly asked for 'udp', then
> -	 * we should probably check if that is open, and should
> -	 * open it if not.  However we don't yet.  All sockets
> -	 * have to be opened when the first daemon is started.
> +		return 1;
> +
> +	return 0;
> +}
> +
> +int
> +nfssvc_setfds(const struct addrinfo *hints, const char *node, const  
> char *port)
> +{
> +	int fd, on = 1, fac = L_ERROR;
> +	int sockfd = -1, rc = 0;
> +	char buf[BUFSIZ];
> +	struct addrinfo *addrhead = NULL, *addr;
> +	char *proto, *family;
> +
> +	/*
> +	 * if file can't be opened, then assume that it's not available and
> +	 * that the caller should just fall back to the old nfsctl interface
> 	 */
> 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
> 	if (fd < 0)
> -		return;
> -	sin.sin_family = AF_INET;
> -	sin.sin_port   = htons(port);
> -	sin.sin_addr.s_addr =  inet_addr(haddr);
> -
> -	if (NFSCTL_UDPISSET(ctlbits)) {
> -		udpfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> -		if (udpfd < 0) {
> -			syslog(LOG_ERR, "nfssvc: unable to create UPD socket: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> -			exit(1);
> -		}
> -		if (bind(udpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> -			syslog(LOG_ERR, "nfssvc: unable to bind UPD socket: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> -			exit(1);
> -		}
> +		return 0;
> +
> +	switch(hints->ai_family) {
> +	case AF_INET:
> +		family = "inet";
> +		break;
> +	case AF_INET6:
> +		family = "inet6";
> +		break;
> +	default:
> +		/* FIXME: should never happen -- return error here? */
> +		family = "unknown family";
> +	}
> +
> +	rc = getaddrinfo(node, port, hints, &addrhead);
> +	if (rc == EAI_NONAME && !strcmp(port, "nfs"))
> +		rc = getaddrinfo(node, "2049", hints, &addrhead);

Here (and in the getservbyname(3) call below) perhaps you could use  
NFS_PORT or something similar.

I recommend breaking this patch up.  It's a lot to change at once, and  
we might benefit from being able to bisect over these changes.

> +
> +	if (rc != 0) {
> +		xlog(L_ERROR, "unable to resolve %s:%s to %s address: "
> +				"%s", node ? node : "ANYADDR", port, family,
> +				rc == EAI_SYSTEM ? strerror(errno) :
> +					gai_strerror(rc));
> +		goto error;
> 	}
>
> -	if (NFSCTL_TCPISSET(ctlbits)) {
> -		tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> -		if (tcpfd < 0) {
> -			syslog(LOG_ERR, "nfssvc: unable to createt tcp socket: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> -			exit(1);
> +	addr = addrhead;
> +	while(addr) {
> +		/* skip non-TCP / non-UDP sockets */
> +		switch(addr->ai_protocol) {
> +		case IPPROTO_UDP:
> +			proto = "UDP";
> +			break;
> +		case IPPROTO_TCP:
> +			proto = "TCP";
> +			break;
> +		default:
> +			addr = addr->ai_next;
> +			continue;
> 		}
> -		if (setsockopt(tcpfd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on))  
> < 0) {
> -			syslog(LOG_ERR, "nfssvc: unable to set SO_REUSEADDR: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> -			exit(1);
> +
> +		xlog(D_GENERAL, "Creating %s %s socket.", family, proto);
> +
> +		/* open socket and prepare to hand it off to kernel */
> +		sockfd = socket(addr->ai_family, addr->ai_socktype,
> +				addr->ai_protocol);
> +		if (sockfd < 0) {
> +			xlog(L_ERROR, "unable to create %s %s socket: "
> +				"errno %d (%s)", family, proto, errno,
> +				strerror(errno));
> +			rc = errno;
> +			goto error;
> 		}
> -		if (bind(tcpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> -			syslog(LOG_ERR, "nfssvc: unable to bind TCP socket: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> -			exit(1);
> +		if (addr->ai_protocol == IPPROTO_TCP &&
> +		    setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &on,  
> sizeof(on))) {
> +			xlog(L_ERROR, "unable to set SO_REUSEADDR on %s "
> +				"socket: errno %d (%s)", family, errno,
> +				strerror(errno));
> +			rc = errno;
> +			goto error;
> 		}
> -		if (listen(tcpfd, 64) < 0){
> -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> -			exit(1);
> +		if (bind(sockfd, addr->ai_addr, addr->ai_addrlen)) {
> +			xlog(L_ERROR, "unable to bind %s %s socket: "
> +				"errno %d (%s)", family, proto, errno,
> +				strerror(errno));
> +			rc = errno;
> +			goto error;
> 		}
> -	}
> -	if (udpfd >= 0) {
> -		snprintf(buf, BUFSIZ,"%d\n", udpfd);
> -		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> -			syslog(LOG_ERR,
> -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> -			       errno, strerror(errno));
> +		if (addr->ai_protocol == IPPROTO_TCP && listen(sockfd, 64)) {
> +			xlog(L_ERROR, "unable to create listening socket: "
> +				"errno %d (%s)", errno, strerror(errno));
> +			rc = errno;
> +			goto error;
> 		}
> -		close(fd);
> -		fd = -1;
> -	}
> -	if (tcpfd >= 0) {
> +
> 		if (fd < 0)
> 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
> -		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
> +
> +		if (fd < 0) {
> +			xlog(L_ERROR, "couldn't open ports file: errno "
> +				      "%d (%s)", errno, strerror(errno));
> +			goto error;
> +		}
> +
> +		snprintf(buf, BUFSIZ, "%d\n", sockfd);
> 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> -			syslog(LOG_ERR,
> -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> -			       errno, strerror(errno));
> +			/*
> +			 * this error may be common on older kernels that don't
> +			 * support IPv6, so turn into a debug message.
> +			 */
> +			if (errno == EAFNOSUPPORT)
> +				fac = D_ALL;
> +			xlog(fac, "writing fd to kernel failed: errno %d (%s)",
> +				  errno, strerror(errno));
> +			rc = errno;
> +			goto error;
> 		}
> +		close(fd);
> +		fd = -1;
> +		addr = addr->ai_next;
> 	}
> -	close(fd);
>
> -	return;
> +error:
> +	if (fd >= 0)
> +		close(fd);
> +	if (addrhead)
> +		freeaddrinfo(addrhead);
> +
> +	return rc;
> }
> -static void
> -nfssvc_versbits(unsigned int ctlbits, int minorvers4)
> +
> +void
> +nfssvc_setvers(unsigned int ctlbits, int minorvers4)
> {
> 	int fd, n, off;
> 	char buf[BUFSIZ], *ptr;
> @@ -140,27 +206,21 @@ nfssvc_versbits(unsigned int ctlbits, int  
> minorvers4)
> 				    n);
> 	snprintf(ptr+off, BUFSIZ - off, "\n");
> 	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> -		syslog(LOG_ERR, "nfssvc: Setting version failed: errno %d (%s)",
> +		xlog(L_ERROR, "Setting version failed: errno %d (%s)",
> 			errno, strerror(errno));
> 	}
> 	close(fd);
>
> 	return;
> }
> +
> int
> -nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
> -	unsigned protobits, char *haddr)
> +nfssvc_threads(unsigned short port, const int nrservs)
> {
> 	struct nfsctl_arg	arg;
> +	struct servent *ent;
> 	int fd;
>
> -	/* Note: must set versions before fds so that
> -	 * the ports get registered with portmap against correct
> -	 * versions
> -	 */
> -	nfssvc_versbits(versbits, minorvers4);
> -	nfssvc_setfds(port, protobits, haddr);
> -
> 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
> 	if (fd < 0)
> 		fd = open("/proc/fs/nfs/threads", O_WRONLY);
> @@ -180,6 +240,14 @@ nfssvc(int port, int nrservs, unsigned int  
> versbits, int minorvers4,
> 			return 0;
> 	}
>
> +	if (!port) {
> +		ent = getservbyname ("nfs", "udp");
> +		if (ent != NULL)
> +			port = ntohs (ent->s_port);
> +		else
> +			port = 2049;
> +	}
> +

In addition to using NFS_PORT (or something similar), you should  
remove the blank before the "(" to match the same style used elsewhere  
in this file.

>
> 	arg.ca_version = NFSCTL_VERSION;
> 	arg.ca_svc.svc_nthreads = nrservs;
> 	arg.ca_svc.svc_port = port;
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index c7bd003..bdc71db 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -18,13 +18,13 @@
> #include <string.h>
> #include <errno.h>
> #include <getopt.h>
> -#include <syslog.h>
> #include <netdb.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
>
> #include "nfslib.h"
> +#include "xlog.h"
>
> static void	usage(const char *);
>
> @@ -37,48 +37,48 @@ static struct option longopts[] =
> 	{ "no-udp", 0, 0, 'U' },
> 	{ "port", 1, 0, 'P' },
> 	{ "port", 1, 0, 'p' },
> +	{ "debug", 0, 0, 'd' },
> 	{ NULL, 0, 0, 0 }
> };
> unsigned int protobits = NFSCTL_ALLBITS;
> unsigned int versbits = NFSCTL_ALLBITS;
> int minorvers4 = NFSD_MAXMINORVERS4;		/* nfsv4 minor version */
> -char *haddr = NULL;
>
> int
> main(int argc, char **argv)
> {
> -	int	count = 1, c, error, port, fd, found_one;
> -	struct servent *ent;
> -	struct hostent *hp;
> -	char *p;
> -
> -	ent = getservbyname ("nfs", "udp");
> -	if (ent != NULL)
> -		port = ntohs (ent->s_port);
> -	else
> -		port = 2049;
> -
> -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,  
> NULL)) != EOF) {
> +	int	count = 1, c, error, portnum = 0, fd, found_one;
> +	char *p, *port = "nfs";
> +	char *haddr = NULL;
> +	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
> +
> +	xlog_syslog(1);
> +	xlog_stderr(0);
> +
> +	while ((c = getopt_long(argc, argv, "dH:hN:p:P:TU", longopts,  
> NULL)) != EOF) {
> 		switch(c) {
> +		case 'd':
> +			xlog_config(D_ALL, 1);
> +			xlog_syslog(0);
> +			xlog_stderr(1);
> +			break;
> 		case 'H':
> -			if (inet_addr(optarg) != INADDR_NONE) {
> +			/*
> +			 * for now, this only handles one -H option. Use the
> +			 * first one specified.
> +			 */

Interesting comment.  Did the old version allow you to specify more  
than one?  Can you run rpc.nfsd more than once, specifying a different  
"-H" on each?

>
> +			if (!haddr)
> 				haddr = strdup(optarg);
> -			} else if ((hp = gethostbyname(optarg)) != NULL) {
> -				haddr = inet_ntoa((*(struct in_addr*)(hp->h_addr_list[0])));
> -			} else {
> -				fprintf(stderr, "%s: Unknown hostname: %s\n",
> -					argv[0], optarg);
> -				usage(argv [0]);
> -			}
> 			break;
> 		case 'P':	/* XXX for nfs-server compatibility */
> 		case 'p':
> -			port = atoi(optarg);
> -			if (port <= 0 || port > 65535) {
> +			portnum = atoi(optarg);
> +			if (portnum <= 0 || portnum > 65535) {
> 				fprintf(stderr, "%s: bad port number: %s\n",
> 					argv[0], optarg);
> -				usage(argv [0]);
> +				usage(argv[0]);
> 			}
> +			port = strdup(optarg);
> 			break;
> 		case 'N':
> 			switch((c = strtol(optarg, &p, 0))) {
> @@ -108,25 +108,21 @@ main(int argc, char **argv)
> 			usage(argv[0]);
> 		}
> 	}
> -	/*
> -	 * Do some sanity checking, if the ctlbits are set
> -	 */
> -	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> -		fprintf(stderr, "invalid protocol specified\n");
> -		exit(1);
> -	}
> +
> +	xlog_open("nfsd");

How about "rpc.nfsd" or even argv[0] ?

>
> +
> 	found_one = 0;
> 	for (c = NFSD_MINVERS; c <= NFSD_MAXVERS; c++) {
> 		if (NFSCTL_VERISSET(versbits, c))
> 			found_one = 1;
> 	}
> 	if (!found_one) {
> -		fprintf(stderr, "no version specified\n");
> +		xlog(L_ERROR, "no version specified");
> 		exit(1);
> 	}			
>
> 	if (NFSCTL_VERISSET(versbits, 4) && !NFSCTL_TCPISSET(protobits)) {
> -		fprintf(stderr, "version 4 requires the TCP protocol\n");
> +		xlog(L_ERROR, "version 4 requires the TCP protocol");
> 		exit(1);
> 	}
> 	if (haddr == NULL) {
> @@ -135,24 +131,51 @@ main(int argc, char **argv)
> 	}
>
> 	if (chdir(NFS_STATEDIR)) {
> -		fprintf(stderr, "%s: chdir(%s) failed: %s\n",
> -			argv [0], NFS_STATEDIR, strerror(errno));
> +		xlog(L_ERROR, "chdir(%s) failed: %s",
> +			NFS_STATEDIR, strerror(errno));
> 		exit(1);
> 	}
>
> 	if (optind < argc) {
> 		if ((count = atoi(argv[optind])) < 0) {
> 			/* insane # of servers */
> -			fprintf(stderr,
> -				"%s: invalid server count (%d), using 1\n",
> +			xlog(L_ERROR,
> +				"invalid server count (%d), using 1",
> 				argv[0], count);
> 			count = 1;
> 		}
> 	}
> +
> +	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> +		xlog(L_ERROR, "invalid protocol specified");
> +		exit(1);
> +	}
> +
> +	if (!NFSCTL_UDPISSET(protobits))
> +		hints.ai_protocol = IPPROTO_TCP;
> +	else if (!NFSCTL_TCPISSET(protobits))
> +		hints.ai_protocol = IPPROTO_UDP;
> +
> +	hints.ai_family = AF_INET;
> +
> +	/*
> +	 * must set versions before the fd's so that the right versions get
> +	 * registered with rpcbind. Note that on older kernels w/o the right
> +	 * interfaces, these are a no-op.
> +	 */
> +	if (!nfssvc_inuse()) {
> +		nfssvc_setvers(versbits, minorvers4);
> +		error = nfssvc_setfds(&hints, haddr, port);
> +		if (error)
> +			goto out;
> +	}
> +
> 	/* KLUDGE ALERT:
> 	   Some kernels let nfsd kernel threads inherit open files
> 	   from the program that spawns them (i.e. us).  So close
> 	   everything before spawning kernel threads.  --Chip */
> +	xlog_syslog(1);
> +	xlog_stderr(0);
> 	fd = open("/dev/null", O_RDWR);
> 	if (fd == -1)
> 		perror("/dev/null");
> @@ -163,13 +186,13 @@ main(int argc, char **argv)
> 	}
> 	closeall(3);
>
> -	openlog("nfsd", LOG_PID, LOG_DAEMON);
> -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> haddr)) < 0) {
> +	if ((error = nfssvc_threads(portnum, count)) < 0) {
> 		int e = errno;
> -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
> -		closelog();
> +		xlog(L_ERROR, "error starting threads: %s", strerror(e));
> 	}
>
> +out:
> +	free(haddr);
> 	return (error != 0);
> }
>
> @@ -177,7 +200,7 @@ static void
> usage(const char *prog)
> {
> 	fprintf(stderr, "Usage:\n"
> -		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version  
> version ] [-T|--no-tcp] [-U|--no-udp] nrservs\n",
> +		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version  
> version ] [-T|--no-tcp] [-U|--no-udp] [-d|--debug] nrservs\n",
> 		prog);
> 	exit(2);
> }
> -- 
> 1.6.0.6
>

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

Re: [PATCH 3/6] nfs-utils: break up nfssvc.c into more individually callable functions

[Jeff Layton]

On Tue, 2 Jun 2009 11:32:01 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Some cursory comments.  I think Steve should take the first two in  
> this series now, as they are simple and reasonable clean-ups.
> 
> On Jun 2, 2009, at 7:43 AM, Jeff Layton wrote:
> 
> > nfssvc.c contains functions for starting up knfsd. Currently, the only
> > non-static function in that file is nfssvc(). In order to add IPv6
> > support, we'll need to be able to call some of these functions in a  
> > more
> > granular fashion.
> >
> > Reorganize these functions and add prototypes to the header so that  
> > they
> > can be called individually, and change the nfsd program to call those
> > routines individually.
> >
> > Change nfssvc_setfds to take a different set of args and change it to
> > use getaddrinfo to look up addresses. This simplifies the code in the
> > core nfsd program significantly and should make adding IPv6 support
> > easier.
> >
> > Finally, change nfsd to use xlog routines for logging and add a -- 
> > debug
> > switch to enable sending output to stderr rather than syslog.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > support/include/nfslib.h |    7 ++-
> > support/nfs/nfssvc.c     |  226 +++++++++++++++++++++++++++++ 
> > +----------------
> > utils/nfsd/nfsd.c        |  109 ++++++++++++++---------
> > 3 files changed, 219 insertions(+), 123 deletions(-)
> >
> > diff --git a/support/include/nfslib.h b/support/include/nfslib.h
> > index ae98650..fe24fe3 100644
> > --- a/support/include/nfslib.h
> > +++ b/support/include/nfslib.h
> > @@ -20,6 +20,7 @@
> > #include <paths.h>
> > #include <rpcsvc/nfs_prot.h>
> > #include <nfs/nfs.h>
> > +#include <netdb.h>
> > #include "xlog.h"
> >
> > #ifndef _PATH_EXPORTS
> > @@ -130,7 +131,11 @@ int			wildmat(char *text, char *pattern);
> >  * nfsd library functions.
> >  */
> > int			nfsctl(int, struct nfsctl_arg *, union nfsctl_res *);
> > -int			nfssvc(int port, int nrservs, unsigned int versbits, int  
> > minorvers4, unsigned int portbits, char *haddr);
> > +int			nfssvc_inuse(void);
> > +int			nfssvc_setfds(const struct addrinfo *hints, const char *node,
> > +					const char *port);
> > +void			nfssvc_setvers(unsigned int ctlbits, int minorvers4);
> > +int			nfssvc_threads(unsigned short port, int nrservs);
> > int			nfsaddclient(struct nfsctl_client *clp);
> > int			nfsdelclient(struct nfsctl_client *clp);
> > int			nfsexport(struct nfsctl_export *exp);
> > diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> > index 33c15a7..e7f3262 100644
> > --- a/support/nfs/nfssvc.c
> > +++ b/support/nfs/nfssvc.c
> > @@ -10,113 +10,179 @@
> > #include <config.h>
> > #endif
> >
> > +#include <sys/types.h>
> > #include <sys/socket.h>
> > +#include <netdb.h>
> > #include <netinet/in.h>
> > #include <arpa/inet.h>
> > #include <unistd.h>
> > #include <fcntl.h>
> > #include <errno.h>
> > -#include <syslog.h>
> > -
> >
> > #include "nfslib.h"
> > +#include "xlog.h"
> >
> > #define NFSD_PORTS_FILE     "/proc/fs/nfsd/portlist"
> > #define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> > #define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
> >
> > -static void
> > -nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> > +/*
> > + * Are there already sockets configured? If not, then it is safe to  
> > try to
> > + * open some and pass them through.
> > + *
> > + * Note: If the user explicitly asked for 'udp', then we should  
> > probably check
> > + * if that is open, and should open it if not. However we don't  
> > yet. All
> > + * sockets have to be opened when the first daemon is started.
> > + */
> > +int
> > +nfssvc_inuse(void)
> > {
> > -	int fd, n, on=1;
> > +	int fd, n;
> > 	char buf[BUFSIZ];
> > -	int udpfd = -1, tcpfd = -1;
> > -	struct sockaddr_in sin;
> >
> > 	fd = open(NFSD_PORTS_FILE, O_RDONLY);
> > +
> > +	/* problem opening file, assume that nothing is configured */
> > 	if (fd < 0)
> > -		return;
> > +		return 0;
> > +
> > 	n = read(fd, buf, BUFSIZ);
> > 	close(fd);
> > +
> > 	if (n != 0)
> > -		return;
> > -	/* there are no ports currently open, so it is safe to
> > -	 * try to open some and pass them through.
> > -	 * Note: If the user explicitly asked for 'udp', then
> > -	 * we should probably check if that is open, and should
> > -	 * open it if not.  However we don't yet.  All sockets
> > -	 * have to be opened when the first daemon is started.
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +nfssvc_setfds(const struct addrinfo *hints, const char *node, const  
> > char *port)
> > +{
> > +	int fd, on = 1, fac = L_ERROR;
> > +	int sockfd = -1, rc = 0;
> > +	char buf[BUFSIZ];
> > +	struct addrinfo *addrhead = NULL, *addr;
> > +	char *proto, *family;
> > +
> > +	/*
> > +	 * if file can't be opened, then assume that it's not available and
> > +	 * that the caller should just fall back to the old nfsctl interface
> > 	 */
> > 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
> > 	if (fd < 0)
> > -		return;
> > -	sin.sin_family = AF_INET;
> > -	sin.sin_port   = htons(port);
> > -	sin.sin_addr.s_addr =  inet_addr(haddr);
> > -
> > -	if (NFSCTL_UDPISSET(ctlbits)) {
> > -		udpfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> > -		if (udpfd < 0) {
> > -			syslog(LOG_ERR, "nfssvc: unable to create UPD socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > -			exit(1);
> > -		}
> > -		if (bind(udpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> > -			syslog(LOG_ERR, "nfssvc: unable to bind UPD socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > -			exit(1);
> > -		}
> > +		return 0;
> > +
> > +	switch(hints->ai_family) {
> > +	case AF_INET:
> > +		family = "inet";
> > +		break;
> > +	case AF_INET6:
> > +		family = "inet6";
> > +		break;
> > +	default:
> > +		/* FIXME: should never happen -- return error here? */
> > +		family = "unknown family";
> > +	}
> > +
> > +	rc = getaddrinfo(node, port, hints, &addrhead);
> > +	if (rc == EAI_NONAME && !strcmp(port, "nfs"))
> > +		rc = getaddrinfo(node, "2049", hints, &addrhead);
> 
> Here (and in the getservbyname(3) call below) perhaps you could use  
> NFS_PORT or something similar.
> 
> I recommend breaking this patch up.  It's a lot to change at once, and  
> we might benefit from being able to bisect over these changes.
> 

Fair enough. It's sort of hard to break this up since a lot of the
changes depend on each other. I can probably do a better job than this
though. Let me see what I can do.

> > +
> > +	if (rc != 0) {
> > +		xlog(L_ERROR, "unable to resolve %s:%s to %s address: "
> > +				"%s", node ? node : "ANYADDR", port, family,
> > +				rc == EAI_SYSTEM ? strerror(errno) :
> > +					gai_strerror(rc));
> > +		goto error;
> > 	}
> >
> > -	if (NFSCTL_TCPISSET(ctlbits)) {
> > -		tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > -		if (tcpfd < 0) {
> > -			syslog(LOG_ERR, "nfssvc: unable to createt tcp socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > -			exit(1);
> > +	addr = addrhead;
> > +	while(addr) {
> > +		/* skip non-TCP / non-UDP sockets */
> > +		switch(addr->ai_protocol) {
> > +		case IPPROTO_UDP:
> > +			proto = "UDP";
> > +			break;
> > +		case IPPROTO_TCP:
> > +			proto = "TCP";
> > +			break;
> > +		default:
> > +			addr = addr->ai_next;
> > +			continue;
> > 		}
> > -		if (setsockopt(tcpfd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on))  
> > < 0) {
> > -			syslog(LOG_ERR, "nfssvc: unable to set SO_REUSEADDR: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > -			exit(1);
> > +
> > +		xlog(D_GENERAL, "Creating %s %s socket.", family, proto);
> > +
> > +		/* open socket and prepare to hand it off to kernel */
> > +		sockfd = socket(addr->ai_family, addr->ai_socktype,
> > +				addr->ai_protocol);
> > +		if (sockfd < 0) {
> > +			xlog(L_ERROR, "unable to create %s %s socket: "
> > +				"errno %d (%s)", family, proto, errno,
> > +				strerror(errno));
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -		if (bind(tcpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> > -			syslog(LOG_ERR, "nfssvc: unable to bind TCP socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > -			exit(1);
> > +		if (addr->ai_protocol == IPPROTO_TCP &&
> > +		    setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &on,  
> > sizeof(on))) {
> > +			xlog(L_ERROR, "unable to set SO_REUSEADDR on %s "
> > +				"socket: errno %d (%s)", family, errno,
> > +				strerror(errno));
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -		if (listen(tcpfd, 64) < 0){
> > -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > -			exit(1);
> > +		if (bind(sockfd, addr->ai_addr, addr->ai_addrlen)) {
> > +			xlog(L_ERROR, "unable to bind %s %s socket: "
> > +				"errno %d (%s)", family, proto, errno,
> > +				strerror(errno));
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -	}
> > -	if (udpfd >= 0) {
> > -		snprintf(buf, BUFSIZ,"%d\n", udpfd);
> > -		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -			syslog(LOG_ERR,
> > -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> > -			       errno, strerror(errno));
> > +		if (addr->ai_protocol == IPPROTO_TCP && listen(sockfd, 64)) {
> > +			xlog(L_ERROR, "unable to create listening socket: "
> > +				"errno %d (%s)", errno, strerror(errno));
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -		close(fd);
> > -		fd = -1;
> > -	}
> > -	if (tcpfd >= 0) {
> > +
> > 		if (fd < 0)
> > 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
> > -		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
> > +
> > +		if (fd < 0) {
> > +			xlog(L_ERROR, "couldn't open ports file: errno "
> > +				      "%d (%s)", errno, strerror(errno));
> > +			goto error;
> > +		}
> > +
> > +		snprintf(buf, BUFSIZ, "%d\n", sockfd);
> > 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -			syslog(LOG_ERR,
> > -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> > -			       errno, strerror(errno));
> > +			/*
> > +			 * this error may be common on older kernels that don't
> > +			 * support IPv6, so turn into a debug message.
> > +			 */
> > +			if (errno == EAFNOSUPPORT)
> > +				fac = D_ALL;
> > +			xlog(fac, "writing fd to kernel failed: errno %d (%s)",
> > +				  errno, strerror(errno));
> > +			rc = errno;
> > +			goto error;
> > 		}
> > +		close(fd);
> > +		fd = -1;
> > +		addr = addr->ai_next;
> > 	}
> > -	close(fd);
> >
> > -	return;
> > +error:
> > +	if (fd >= 0)
> > +		close(fd);
> > +	if (addrhead)
> > +		freeaddrinfo(addrhead);
> > +
> > +	return rc;
> > }
> > -static void
> > -nfssvc_versbits(unsigned int ctlbits, int minorvers4)
> > +
> > +void
> > +nfssvc_setvers(unsigned int ctlbits, int minorvers4)
> > {
> > 	int fd, n, off;
> > 	char buf[BUFSIZ], *ptr;
> > @@ -140,27 +206,21 @@ nfssvc_versbits(unsigned int ctlbits, int  
> > minorvers4)
> > 				    n);
> > 	snprintf(ptr+off, BUFSIZ - off, "\n");
> > 	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -		syslog(LOG_ERR, "nfssvc: Setting version failed: errno %d (%s)",
> > +		xlog(L_ERROR, "Setting version failed: errno %d (%s)",
> > 			errno, strerror(errno));
> > 	}
> > 	close(fd);
> >
> > 	return;
> > }
> > +
> > int
> > -nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
> > -	unsigned protobits, char *haddr)
> > +nfssvc_threads(unsigned short port, const int nrservs)
> > {
> > 	struct nfsctl_arg	arg;
> > +	struct servent *ent;
> > 	int fd;
> >
> > -	/* Note: must set versions before fds so that
> > -	 * the ports get registered with portmap against correct
> > -	 * versions
> > -	 */
> > -	nfssvc_versbits(versbits, minorvers4);
> > -	nfssvc_setfds(port, protobits, haddr);
> > -
> > 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
> > 	if (fd < 0)
> > 		fd = open("/proc/fs/nfs/threads", O_WRONLY);
> > @@ -180,6 +240,14 @@ nfssvc(int port, int nrservs, unsigned int  
> > versbits, int minorvers4,
> > 			return 0;
> > 	}
> >
> > +	if (!port) {
> > +		ent = getservbyname ("nfs", "udp");
> > +		if (ent != NULL)
> > +			port = ntohs (ent->s_port);
> > +		else
> > +			port = 2049;
> > +	}
> > +
> 
> In addition to using NFS_PORT (or something similar), you should  
> remove the blank before the "(" to match the same style used elsewhere  
> in this file.
> 

Ahh, didn't realize we had a #define for this (the original nfsd code
didn't use it). I'll switch it to use that (always preferred over
"magic numbers"). Good catch on the parens -- leftover from cut and
paste job...

> >
> > 	arg.ca_version = NFSCTL_VERSION;
> > 	arg.ca_svc.svc_nthreads = nrservs;
> > 	arg.ca_svc.svc_port = port;
> > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> > index c7bd003..bdc71db 100644
> > --- a/utils/nfsd/nfsd.c
> > +++ b/utils/nfsd/nfsd.c
> > @@ -18,13 +18,13 @@
> > #include <string.h>
> > #include <errno.h>
> > #include <getopt.h>
> > -#include <syslog.h>
> > #include <netdb.h>
> > #include <sys/socket.h>
> > #include <netinet/in.h>
> > #include <arpa/inet.h>
> >
> > #include "nfslib.h"
> > +#include "xlog.h"
> >
> > static void	usage(const char *);
> >
> > @@ -37,48 +37,48 @@ static struct option longopts[] =
> > 	{ "no-udp", 0, 0, 'U' },
> > 	{ "port", 1, 0, 'P' },
> > 	{ "port", 1, 0, 'p' },
> > +	{ "debug", 0, 0, 'd' },
> > 	{ NULL, 0, 0, 0 }
> > };
> > unsigned int protobits = NFSCTL_ALLBITS;
> > unsigned int versbits = NFSCTL_ALLBITS;
> > int minorvers4 = NFSD_MAXMINORVERS4;		/* nfsv4 minor version */
> > -char *haddr = NULL;
> >
> > int
> > main(int argc, char **argv)
> > {
> > -	int	count = 1, c, error, port, fd, found_one;
> > -	struct servent *ent;
> > -	struct hostent *hp;
> > -	char *p;
> > -
> > -	ent = getservbyname ("nfs", "udp");
> > -	if (ent != NULL)
> > -		port = ntohs (ent->s_port);
> > -	else
> > -		port = 2049;
> > -
> > -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,  
> > NULL)) != EOF) {
> > +	int	count = 1, c, error, portnum = 0, fd, found_one;
> > +	char *p, *port = "nfs";
> > +	char *haddr = NULL;
> > +	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
> > +
> > +	xlog_syslog(1);
> > +	xlog_stderr(0);
> > +
> > +	while ((c = getopt_long(argc, argv, "dH:hN:p:P:TU", longopts,  
> > NULL)) != EOF) {
> > 		switch(c) {
> > +		case 'd':
> > +			xlog_config(D_ALL, 1);
> > +			xlog_syslog(0);
> > +			xlog_stderr(1);
> > +			break;
> > 		case 'H':
> > -			if (inet_addr(optarg) != INADDR_NONE) {
> > +			/*
> > +			 * for now, this only handles one -H option. Use the
> > +			 * first one specified.
> > +			 */
> 
> Interesting comment.  Did the old version allow you to specify more  
> than one?  Can you run rpc.nfsd more than once, specifying a different  
> "-H" on each?
> 
> >
> > +			if (!haddr)
> > 				haddr = strdup(optarg);
> > -			} else if ((hp = gethostbyname(optarg)) != NULL) {
> > -				haddr = inet_ntoa((*(struct in_addr*)(hp->h_addr_list[0])));
> > -			} else {
> > -				fprintf(stderr, "%s: Unknown hostname: %s\n",
> > -					argv[0], optarg);
> > -				usage(argv [0]);
> > -			}
> > 			break;
> > 		case 'P':	/* XXX for nfs-server compatibility */
> > 		case 'p':
> > -			port = atoi(optarg);
> > -			if (port <= 0 || port > 65535) {
> > +			portnum = atoi(optarg);
> > +			if (portnum <= 0 || portnum > 65535) {
> > 				fprintf(stderr, "%s: bad port number: %s\n",
> > 					argv[0], optarg);
> > -				usage(argv [0]);
> > +				usage(argv[0]);
> > 			}
> > +			port = strdup(optarg);
> > 			break;
> > 		case 'N':
> > 			switch((c = strtol(optarg, &p, 0))) {
> > @@ -108,25 +108,21 @@ main(int argc, char **argv)
> > 			usage(argv[0]);
> > 		}
> > 	}
> > -	/*
> > -	 * Do some sanity checking, if the ctlbits are set
> > -	 */
> > -	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> > -		fprintf(stderr, "invalid protocol specified\n");
> > -		exit(1);
> > -	}
> > +
> > +	xlog_open("nfsd");
> 
> How about "rpc.nfsd" or even argv[0] ?
> 
> >
> > +
> > 	found_one = 0;
> > 	for (c = NFSD_MINVERS; c <= NFSD_MAXVERS; c++) {
> > 		if (NFSCTL_VERISSET(versbits, c))
> > 			found_one = 1;
> > 	}
> > 	if (!found_one) {
> > -		fprintf(stderr, "no version specified\n");
> > +		xlog(L_ERROR, "no version specified");
> > 		exit(1);
> > 	}			
> >
> > 	if (NFSCTL_VERISSET(versbits, 4) && !NFSCTL_TCPISSET(protobits)) {
> > -		fprintf(stderr, "version 4 requires the TCP protocol\n");
> > +		xlog(L_ERROR, "version 4 requires the TCP protocol");
> > 		exit(1);
> > 	}
> > 	if (haddr == NULL) {
> > @@ -135,24 +131,51 @@ main(int argc, char **argv)
> > 	}
> >
> > 	if (chdir(NFS_STATEDIR)) {
> > -		fprintf(stderr, "%s: chdir(%s) failed: %s\n",
> > -			argv [0], NFS_STATEDIR, strerror(errno));
> > +		xlog(L_ERROR, "chdir(%s) failed: %s",
> > +			NFS_STATEDIR, strerror(errno));
> > 		exit(1);
> > 	}
> >
> > 	if (optind < argc) {
> > 		if ((count = atoi(argv[optind])) < 0) {
> > 			/* insane # of servers */
> > -			fprintf(stderr,
> > -				"%s: invalid server count (%d), using 1\n",
> > +			xlog(L_ERROR,
> > +				"invalid server count (%d), using 1",
> > 				argv[0], count);
> > 			count = 1;
> > 		}
> > 	}
> > +
> > +	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> > +		xlog(L_ERROR, "invalid protocol specified");
> > +		exit(1);
> > +	}
> > +
> > +	if (!NFSCTL_UDPISSET(protobits))
> > +		hints.ai_protocol = IPPROTO_TCP;
> > +	else if (!NFSCTL_TCPISSET(protobits))
> > +		hints.ai_protocol = IPPROTO_UDP;
> > +
> > +	hints.ai_family = AF_INET;
> > +
> > +	/*
> > +	 * must set versions before the fd's so that the right versions get
> > +	 * registered with rpcbind. Note that on older kernels w/o the right
> > +	 * interfaces, these are a no-op.
> > +	 */
> > +	if (!nfssvc_inuse()) {
> > +		nfssvc_setvers(versbits, minorvers4);
> > +		error = nfssvc_setfds(&hints, haddr, port);
> > +		if (error)
> > +			goto out;
> > +	}
> > +
> > 	/* KLUDGE ALERT:
> > 	   Some kernels let nfsd kernel threads inherit open files
> > 	   from the program that spawns them (i.e. us).  So close
> > 	   everything before spawning kernel threads.  --Chip */
> > +	xlog_syslog(1);
> > +	xlog_stderr(0);
> > 	fd = open("/dev/null", O_RDWR);
> > 	if (fd == -1)
> > 		perror("/dev/null");
> > @@ -163,13 +186,13 @@ main(int argc, char **argv)
> > 	}
> > 	closeall(3);
> >
> > -	openlog("nfsd", LOG_PID, LOG_DAEMON);
> > -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > haddr)) < 0) {
> > +	if ((error = nfssvc_threads(portnum, count)) < 0) {
> > 		int e = errno;
> > -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
> > -		closelog();
> > +		xlog(L_ERROR, "error starting threads: %s", strerror(e));
> > 	}
> >
> > +out:
> > +	free(haddr);
> > 	return (error != 0);
> > }
> >
> > @@ -177,7 +200,7 @@ static void
> > usage(const char *prog)
> > {
> > 	fprintf(stderr, "Usage:\n"
> > -		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version  
> > version ] [-T|--no-tcp] [-U|--no-udp] nrservs\n",
> > +		"%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version  
> > version ] [-T|--no-tcp] [-U|--no-udp] [-d|--debug] nrservs\n",
> > 		prog);
> > 	exit(2);
> > }
> > -- 
> > 1.6.0.6
> >
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 4/6] nfs-utils: set IPV6_V6ONLY on nfssvc IPv6 sockets

[Jeff Layton]

IPv6 sockets for knfsd can't be allowed to accept IPv4 packets. Set the
correct option to prevent it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 support/nfs/nfssvc.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index e7f3262..1bb64f0 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -123,6 +123,13 @@ nfssvc_setfds(const struct addrinfo *hints, const char *node, const char *port)
 			rc = errno;
 			goto error;
 		}
+		if (addr->ai_family == AF_INET6 &&
+		    setsockopt(sockfd, IPPROTO_IPV6, IPV6_V6ONLY, &on, sizeof(on))) {
+			xlog(L_ERROR, "unable to set IPV6_V6ONLY: "
+				"errno %d (%s)\n", errno, strerror(errno));
+			rc = -errno;
+			goto error;
+		}
 		if (addr->ai_protocol == IPPROTO_TCP &&
 		    setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on))) {
 			xlog(L_ERROR, "unable to set SO_REUSEADDR on %s "
-- 
1.6.0.6

[PATCH 5/6] nfs-utils: add IPv6 support to nfsd

[Jeff Layton]

Add support for handing off IPv6 sockets to the kernel for nfsd. One of
the main goals here is to not change the behavior of options and not to
add any new ones, so this patch attempts to do that.

We also don't want to break anything in the event that someone has an
rpc.nfsd program built with IPv6 capability, but the knfsd doesn't
support IPv6. Ditto for the cases where IPv6 is either not compiled in
or is compiled in and blacklisted.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 support/include/nfs/nfs.h |   15 +++-
 utils/nfsd/nfsd.c         |  170 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 141 insertions(+), 44 deletions(-)

diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
index 00b0028..a64eb0a 100644
--- a/support/include/nfs/nfs.h
+++ b/support/include/nfs/nfs.h
@@ -42,14 +42,21 @@ struct nfs_fh_old {
 #define NFSCTL_GETFD		7	/* get an fh by path (used by mountd) */
 #define NFSCTL_GETFS		8	/* get an fh by path with max size (used by mountd) */
 
+#define NFSCTL_UDPBIT		      (1 << (17 - 1))
+#define NFSCTL_TCPBIT		      (1 << (18 - 1))
+
 #define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << ((_v) - 1))) 
-#define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (17 - 1))) 
-#define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (18 - 1))) 
+#define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~NFSCTL_UDPBIT) 
+#define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~NFSCTL_TCPBIT) 
 
 #define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1))) 
-#define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & (1 << (17 - 1))) 
-#define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & (1 << (18 - 1))) 
+#define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & NFSCTL_UDPBIT) 
+#define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & NFSCTL_TCPBIT) 
+
+#define NFSCTL_UDPSET(_cltbits)       ((_cltbits) |= NFSCTL_UDPBIT)
+#define NFSCTL_TCPSET(_cltbits)       ((_cltbits) |= NFSCTL_TCPBIT)
 
+#define NFSCTL_ANYPROTO(_cltbits)     ((_cltbits) & (NFSCTL_UDPBIT | NFSCTL_TCPBIT))
 #define NFSCTL_ALLBITS (~0)
 
 /* SVC */
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index bdc71db..efacd12 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -40,17 +40,82 @@ static struct option longopts[] =
 	{ "debug", 0, 0, 'd' },
 	{ NULL, 0, 0, 0 }
 };
-unsigned int protobits = NFSCTL_ALLBITS;
-unsigned int versbits = NFSCTL_ALLBITS;
-int minorvers4 = NFSD_MAXMINORVERS4;		/* nfsv4 minor version */
+
+/* given a family and ctlbits, disable any that aren't listed in netconfig */
+#ifdef HAVE_LIBTIRPC
+static void
+nfsd_enable_protos(unsigned int *proto4, unsigned int *proto6)
+{
+	struct netconfig *nconf;
+	unsigned int *famproto;
+	void *handle;
+
+	xlog(D_GENERAL, "Checking netconfig for visible protocols.");
+
+	handle = setnetconfig();
+	while((nconf = getnetconfig(handle))) {
+		if (!(nconf->nc_flag & NC_VISIBLE))
+			continue;
+
+		if (!strcmp(nconf->nc_protofmly, NC_INET))
+			famproto = proto4;
+		else if (!strcmp(nconf->nc_protofmly, NC_INET6))
+			famproto = proto6;
+		else
+			continue;
+
+		if (!strcmp(nconf->nc_proto, NC_TCP))
+			NFSCTL_TCPSET(*famproto);
+		else if (!strcmp(nconf->nc_proto, NC_UDP))
+			NFSCTL_UDPSET(*famproto);
+
+		xlog(D_GENERAL, "Enabling %s %s.", nconf->nc_protofmly,
+			nconf->nc_proto);
+	}
+	endnetconfig(handle);
+	return;
+}
+#else /* HAVE_LIBTIRPC */
+static void
+nfsd_enable_protos(unsigned int *proto4, unsigned int *proto6)
+{
+	/* Enable all IPv4 protocols if no TIRPC support */
+	*proto4 = NFSCTL_ALLBITS;
+	*proto6 = 0;
+}
+#endif /* HAVE_LIBTIRPC */
+
+/* returns 1 on success, 0 on failure */
+static int
+nfsd_set_socket(const int family, const unsigned int protobits,
+		const char *host, const char *port)
+{
+	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
+
+	hints.ai_family = family;
+
+	if (!NFSCTL_ANYPROTO(protobits))
+		return 0;
+	else if (!NFSCTL_UDPISSET(protobits))
+		hints.ai_protocol = IPPROTO_TCP;
+	else if (!NFSCTL_TCPISSET(protobits))
+		hints.ai_protocol = IPPROTO_UDP;
+
+	return nfssvc_setfds(&hints, host, port) ? 0 : 1;
+}
 
 int
 main(int argc, char **argv)
 {
-	int	count = 1, c, error, portnum = 0, fd, found_one;
+	int	count = 1, c, error = 0, portnum = 0, fd, found_one;
 	char *p, *port = "nfs";
 	char *haddr = NULL;
-	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
+	int minorvers4 = NFSD_MAXMINORVERS4;	/* nfsv4 minor version */
+	unsigned int versbits = NFSCTL_ALLBITS;
+	unsigned int protobits = NFSCTL_ALLBITS;
+	unsigned int proto4 = 0;
+	unsigned int proto6 = 0;
+	int socket_up = 0;
 
 	xlog_syslog(1);
 	xlog_stderr(0);
@@ -109,8 +174,38 @@ main(int argc, char **argv)
 		}
 	}
 
+	if (optind < argc) {
+		if ((count = atoi(argv[optind])) < 0) {
+			/* insane # of servers */
+			fprintf(stderr,
+				"%s: invalid server count (%d), using 1\n",
+				argv[0], count);
+			count = 1;
+		} else if (count == 0) {
+			/*
+			 * don't bother setting anything else if the threads
+			 * are coming down anyway.
+			 */
+			socket_up = 1;
+			goto set_threads;
+		}
+	}
+
 	xlog_open("nfsd");
 
+	nfsd_enable_protos(&proto4, &proto6);
+
+	if (!NFSCTL_TCPISSET(protobits)) {
+		NFSCTL_TCPUNSET(proto4);
+		NFSCTL_TCPUNSET(proto6);
+	}
+
+	if (!NFSCTL_UDPISSET(protobits)) {
+		NFSCTL_UDPUNSET(proto4);
+		NFSCTL_UDPUNSET(proto6);
+	}
+
+	/* make sure that at least one version is enabled */
 	found_one = 0;
 	for (c = NFSD_MINVERS; c <= NFSD_MAXVERS; c++) {
 		if (NFSCTL_VERISSET(versbits, c))
@@ -121,14 +216,12 @@ main(int argc, char **argv)
 		exit(1);
 	}			
 
-	if (NFSCTL_VERISSET(versbits, 4) && !NFSCTL_TCPISSET(protobits)) {
+	if (NFSCTL_VERISSET(versbits, 4) &&
+	    !NFSCTL_TCPISSET(proto4) &&
+	    !NFSCTL_TCPISSET(proto6)) {
 		xlog(L_ERROR, "version 4 requires the TCP protocol");
 		exit(1);
 	}
-	if (haddr == NULL) {
-		struct in_addr in = {INADDR_ANY}; 
-		haddr = strdup(inet_ntoa(in));
-	}
 
 	if (chdir(NFS_STATEDIR)) {
 		xlog(L_ERROR, "chdir(%s) failed: %s",
@@ -136,49 +229,46 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	if (optind < argc) {
-		if ((count = atoi(argv[optind])) < 0) {
-			/* insane # of servers */
-			xlog(L_ERROR,
-				"invalid server count (%d), using 1",
-				argv[0], count);
-			count = 1;
-		}
+	/* we can only change number of threads if nfsd is already up */
+	if (nfssvc_inuse()) {
+		socket_up = 1;
+		goto set_threads;
 	}
 
-	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
-		xlog(L_ERROR, "invalid protocol specified");
-		exit(1);
-	}
-
-	if (!NFSCTL_UDPISSET(protobits))
-		hints.ai_protocol = IPPROTO_TCP;
-	else if (!NFSCTL_TCPISSET(protobits))
-		hints.ai_protocol = IPPROTO_UDP;
-
-	hints.ai_family = AF_INET;
-
 	/*
 	 * must set versions before the fd's so that the right versions get
 	 * registered with rpcbind. Note that on older kernels w/o the right
 	 * interfaces, these are a no-op.
 	 */
-	if (!nfssvc_inuse()) {
-		nfssvc_setvers(versbits, minorvers4);
-		error = nfssvc_setfds(&hints, haddr, port);
-		if (error)
-			goto out;
+	nfssvc_setvers(versbits, minorvers4);
+ 
+	if (nfsd_set_socket(AF_INET, proto4, haddr, port))
+		socket_up = 1;
+
+#ifdef IPV6_SUPPORTED
+	if (nfsd_set_socket(AF_INET6, proto6, haddr, port))
+		socket_up = 1;
+#endif /* IPV6_SUPPORTED */
+
+set_threads:
+	/* don't start any threads if unable to hand off any sockets */
+	if (!socket_up) {
+		xlog(L_ERROR, "unable to set any sockets for nfsd");
+		goto out;
 	}
+	error = 0;
 
-	/* KLUDGE ALERT:
-	   Some kernels let nfsd kernel threads inherit open files
-	   from the program that spawns them (i.e. us).  So close
-	   everything before spawning kernel threads.  --Chip */
+	/*
+	 * KLUDGE ALERT:
+	 * Some kernels let nfsd kernel threads inherit open files
+	 * from the program that spawns them (i.e. us).  So close
+	 * everything before spawning kernel threads.  --Chip
+	 */
 	xlog_syslog(1);
 	xlog_stderr(0);
 	fd = open("/dev/null", O_RDWR);
 	if (fd == -1)
-		perror("/dev/null");
+		xlog(L_ERROR, "unable to open /dev/null: %s", strerror(errno));
 	else {
 		(void) dup2(fd, 0);
 		(void) dup2(fd, 1);
-- 
1.6.0.6

Re: [PATCH 5/6] nfs-utils: add IPv6 support to nfsd

[Chuck Lever]

On Jun 2, 2009, at 7:43 AM, Jeff Layton wrote:

> Add support for handing off IPv6 sockets to the kernel for nfsd. One  
> of
> the main goals here is to not change the behavior of options and not  
> to
> add any new ones, so this patch attempts to do that.
>
> We also don't want to break anything in the event that someone has an
> rpc.nfsd program built with IPv6 capability, but the knfsd doesn't
> support IPv6. Ditto for the cases where IPv6 is either not compiled in
> or is compiled in and blacklisted.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/include/nfs/nfs.h |   15 +++-
> utils/nfsd/nfsd.c         |  170 +++++++++++++++++++++++++++++++++ 
> +-----------
> 2 files changed, 141 insertions(+), 44 deletions(-)
>
> diff --git a/support/include/nfs/nfs.h b/support/include/nfs/nfs.h
> index 00b0028..a64eb0a 100644
> --- a/support/include/nfs/nfs.h
> +++ b/support/include/nfs/nfs.h
> @@ -42,14 +42,21 @@ struct nfs_fh_old {
> #define NFSCTL_GETFD		7	/* get an fh by path (used by mountd) */
> #define NFSCTL_GETFS		8	/* get an fh by path with max size (used by  
> mountd) */
>
> +#define NFSCTL_UDPBIT		      (1 << (17 - 1))
> +#define NFSCTL_TCPBIT		      (1 << (18 - 1))
> +
> #define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << ((_v) -  
> 1)))
> -#define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (17 -  
> 1)))
> -#define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (18 -  
> 1)))
> +#define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~NFSCTL_UDPBIT)
> +#define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~NFSCTL_TCPBIT)
>
> #define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1)))
> -#define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & (1 << (17 - 1)))
> -#define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & (1 << (18 - 1)))
> +#define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & NFSCTL_UDPBIT)
> +#define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & NFSCTL_TCPBIT)
> +
> +#define NFSCTL_UDPSET(_cltbits)       ((_cltbits) |= NFSCTL_UDPBIT)
> +#define NFSCTL_TCPSET(_cltbits)       ((_cltbits) |= NFSCTL_TCPBIT)
>
> +#define NFSCTL_ANYPROTO(_cltbits)     ((_cltbits) & (NFSCTL_UDPBIT  
> | NFSCTL_TCPBIT))
> #define NFSCTL_ALLBITS (~0)
>
> /* SVC */
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index bdc71db..efacd12 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -40,17 +40,82 @@ static struct option longopts[] =
> 	{ "debug", 0, 0, 'd' },
> 	{ NULL, 0, 0, 0 }
> };
> -unsigned int protobits = NFSCTL_ALLBITS;
> -unsigned int versbits = NFSCTL_ALLBITS;
> -int minorvers4 = NFSD_MAXMINORVERS4;		/* nfsv4 minor version */
> +
> +/* given a family and ctlbits, disable any that aren't listed in  
> netconfig */
> +#ifdef HAVE_LIBTIRPC
> +static void
> +nfsd_enable_protos(unsigned int *proto4, unsigned int *proto6)
> +{
> +	struct netconfig *nconf;
> +	unsigned int *famproto;
> +	void *handle;
> +
> +	xlog(D_GENERAL, "Checking netconfig for visible protocols.");
> +
> +	handle = setnetconfig();
> +	while((nconf = getnetconfig(handle))) {
> +		if (!(nconf->nc_flag & NC_VISIBLE))
> +			continue;
> +
> +		if (!strcmp(nconf->nc_protofmly, NC_INET))
> +			famproto = proto4;
> +		else if (!strcmp(nconf->nc_protofmly, NC_INET6))
> +			famproto = proto6;
> +		else
> +			continue;
> +
> +		if (!strcmp(nconf->nc_proto, NC_TCP))
> +			NFSCTL_TCPSET(*famproto);
> +		else if (!strcmp(nconf->nc_proto, NC_UDP))
> +			NFSCTL_UDPSET(*famproto);
> +
> +		xlog(D_GENERAL, "Enabling %s %s.", nconf->nc_protofmly,
> +			nconf->nc_proto);
> +	}
> +	endnetconfig(handle);
> +	return;
> +}
> +#else /* HAVE_LIBTIRPC */
> +static void
> +nfsd_enable_protos(unsigned int *proto4, unsigned int *proto6)
> +{
> +	/* Enable all IPv4 protocols if no TIRPC support */
> +	*proto4 = NFSCTL_ALLBITS;
> +	*proto6 = 0;
> +}
> +#endif /* HAVE_LIBTIRPC */
> +
> +/* returns 1 on success, 0 on failure */
> +static int
> +nfsd_set_socket(const int family, const unsigned int protobits,
> +		const char *host, const char *port)
> +{
> +	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
> +
> +	hints.ai_family = family;
> +
> +	if (!NFSCTL_ANYPROTO(protobits))
> +		return 0;
> +	else if (!NFSCTL_UDPISSET(protobits))
> +		hints.ai_protocol = IPPROTO_TCP;
> +	else if (!NFSCTL_TCPISSET(protobits))
> +		hints.ai_protocol = IPPROTO_UDP;
> +
> +	return nfssvc_setfds(&hints, host, port) ? 0 : 1;
> +}
>
> int
> main(int argc, char **argv)
> {
> -	int	count = 1, c, error, portnum = 0, fd, found_one;
> +	int	count = 1, c, error = 0, portnum = 0, fd, found_one;
> 	char *p, *port = "nfs";
> 	char *haddr = NULL;
> -	struct addrinfo hints = { .ai_flags = AI_PASSIVE | AI_ADDRCONFIG };
> +	int minorvers4 = NFSD_MAXMINORVERS4;	/* nfsv4 minor version */
> +	unsigned int versbits = NFSCTL_ALLBITS;
> +	unsigned int protobits = NFSCTL_ALLBITS;
> +	unsigned int proto4 = 0;
> +	unsigned int proto6 = 0;
> +	int socket_up = 0;
>
> 	xlog_syslog(1);
> 	xlog_stderr(0);
> @@ -109,8 +174,38 @@ main(int argc, char **argv)
> 		}
> 	}
>
> +	if (optind < argc) {
> +		if ((count = atoi(argv[optind])) < 0) {
> +			/* insane # of servers */
> +			fprintf(stderr,
> +				"%s: invalid server count (%d), using 1\n",
> +				argv[0], count);
> +			count = 1;
> +		} else if (count == 0) {
> +			/*
> +			 * don't bother setting anything else if the threads
> +			 * are coming down anyway.
> +			 */
> +			socket_up = 1;
> +			goto set_threads;
> +		}
> +	}
> +
> 	xlog_open("nfsd");
>
> +	nfsd_enable_protos(&proto4, &proto6);
> +
> +	if (!NFSCTL_TCPISSET(protobits)) {
> +		NFSCTL_TCPUNSET(proto4);
> +		NFSCTL_TCPUNSET(proto6);
> +	}
> +
> +	if (!NFSCTL_UDPISSET(protobits)) {
> +		NFSCTL_UDPUNSET(proto4);
> +		NFSCTL_UDPUNSET(proto6);
> +	}
> +
> +	/* make sure that at least one version is enabled */
> 	found_one = 0;
> 	for (c = NFSD_MINVERS; c <= NFSD_MAXVERS; c++) {
> 		if (NFSCTL_VERISSET(versbits, c))
> @@ -121,14 +216,12 @@ main(int argc, char **argv)
> 		exit(1);
> 	}			
>
> -	if (NFSCTL_VERISSET(versbits, 4) && !NFSCTL_TCPISSET(protobits)) {
> +	if (NFSCTL_VERISSET(versbits, 4) &&
> +	    !NFSCTL_TCPISSET(proto4) &&
> +	    !NFSCTL_TCPISSET(proto6)) {
> 		xlog(L_ERROR, "version 4 requires the TCP protocol");
> 		exit(1);
> 	}
> -	if (haddr == NULL) {
> -		struct in_addr in = {INADDR_ANY};
> -		haddr = strdup(inet_ntoa(in));
> -	}
>
> 	if (chdir(NFS_STATEDIR)) {
> 		xlog(L_ERROR, "chdir(%s) failed: %s",
> @@ -136,49 +229,46 @@ main(int argc, char **argv)
> 		exit(1);
> 	}
>
> -	if (optind < argc) {
> -		if ((count = atoi(argv[optind])) < 0) {
> -			/* insane # of servers */
> -			xlog(L_ERROR,
> -				"invalid server count (%d), using 1",
> -				argv[0], count);
> -			count = 1;
> -		}
> +	/* we can only change number of threads if nfsd is already up */
> +	if (nfssvc_inuse()) {
> +		socket_up = 1;
> +		goto set_threads;
> 	}
>
> -	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> -		xlog(L_ERROR, "invalid protocol specified");
> -		exit(1);
> -	}
> -
> -	if (!NFSCTL_UDPISSET(protobits))
> -		hints.ai_protocol = IPPROTO_TCP;
> -	else if (!NFSCTL_TCPISSET(protobits))
> -		hints.ai_protocol = IPPROTO_UDP;
> -
> -	hints.ai_family = AF_INET;
> -
> 	/*
> 	 * must set versions before the fd's so that the right versions get
> 	 * registered with rpcbind. Note that on older kernels w/o the right
> 	 * interfaces, these are a no-op.
> 	 */
> -	if (!nfssvc_inuse()) {
> -		nfssvc_setvers(versbits, minorvers4);
> -		error = nfssvc_setfds(&hints, haddr, port);
> -		if (error)
> -			goto out;
> +	nfssvc_setvers(versbits, minorvers4);
> +
> +	if (nfsd_set_socket(AF_INET, proto4, haddr, port))
> +		socket_up = 1;
> +
> +#ifdef IPV6_SUPPORTED
> +	if (nfsd_set_socket(AF_INET6, proto6, haddr, port))
> +		socket_up = 1;
> +#endif /* IPV6_SUPPORTED */
> +
> +set_threads:
> +	/* don't start any threads if unable to hand off any sockets */
> +	if (!socket_up) {
> +		xlog(L_ERROR, "unable to set any sockets for nfsd");
> +		goto out;
> 	}
> +	error = 0;
>
> -	/* KLUDGE ALERT:
> -	   Some kernels let nfsd kernel threads inherit open files
> -	   from the program that spawns them (i.e. us).  So close
> -	   everything before spawning kernel threads.  --Chip */
> +	/*
> +	 * KLUDGE ALERT:
> +	 * Some kernels let nfsd kernel threads inherit open files
> +	 * from the program that spawns them (i.e. us).  So close
> +	 * everything before spawning kernel threads.  --Chip
> +	 */
> 	xlog_syslog(1);
> 	xlog_stderr(0);
> 	fd = open("/dev/null", O_RDWR);
> 	if (fd == -1)
> -		perror("/dev/null");
> +		xlog(L_ERROR, "unable to open /dev/null: %s", strerror(errno));

("%m") does the same thing as ("%s", strerror(errno))

>
> 	else {
> 		(void) dup2(fd, 0);
> 		(void) dup2(fd, 1);

Should you check the return code from these?

>
> -- 
> 1.6.0.6
>

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

Re: [PATCH 5/6] nfs-utils: add IPv6 support to nfsd

[Jeff Layton]

On Tue, 2 Jun 2009 11:35:18 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

Forgot a couple of comments from the earlier email:

> > 		case 'H':
> > -			if (inet_addr(optarg) != INADDR_NONE) {
> > +			/*
> > +			 * for now, this only handles one -H option. Use the
> > +			 * first one specified.
> > +			 */  
> 
> Interesting comment.  Did the old version allow you to specify more  
> than one?  Can you run rpc.nfsd more than once, specifying a different  
> "-H" on each?

The only option that matters once nfsd is up is the number of threads
-- any others are ignored. This applies to the -H option as well, so
it'll only matter for the first run of rpc.nfsd. You could specify -H
multiple times on a single nfsd run, but only the last one would
matter. With this patch, I changed it to be the first one since that
seemed more intuitive to me.

We could eventually allow nfsd to take multiple -H arguments, but
that's really outside the scope of this set.

> > +
> > +	xlog_open("nfsd");  
> 
> How about "rpc.nfsd" or even argv[0] ?

rpc.nfsd is probably easier. basename(argv[0]) is probably the best
thing though. I'll fix it up to use that.

> > +	/*
> > +	 * KLUDGE ALERT:
> > +	 * Some kernels let nfsd kernel threads inherit open files
> > +	 * from the program that spawns them (i.e. us).  So close
> > +	 * everything before spawning kernel threads.  --Chip
> > +	 */
> > 	xlog_syslog(1);
> > 	xlog_stderr(0);
> > 	fd = open("/dev/null", O_RDWR);
> > 	if (fd == -1)
> > -		perror("/dev/null");
> > +		xlog(L_ERROR, "unable to open /dev/null: %s", strerror(errno));
> 
> ("%m") does the same thing as ("%s", strerror(errno))
> 

Nice...ok I'll change it (and other spots if there are any).

> >
> > 	else {
> > 		(void) dup2(fd, 0);
> > 		(void) dup2(fd, 1);
> 
> Should you check the return code from these?
> 

The existing code doesn't. If we want to change that it should prob be
a separate patch. I wonder though whether we need to do all of this
stuff with /dev/null. Stupid question: is there some reason we can't
just close stdin/stdout/stderr?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 5/6] nfs-utils: add IPv6 support to nfsd

[Chuck Lever]

On Jun 2, 2009, at 2:55 PM, Jeff Layton wrote:

> On Tue, 2 Jun 2009 11:35:18 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
> Forgot a couple of comments from the earlier email:
>
>>> 		case 'H':
>>> -			if (inet_addr(optarg) != INADDR_NONE) {
>>> +			/*
>>> +			 * for now, this only handles one -H option. Use the
>>> +			 * first one specified.
>>> +			 */
>>
>> Interesting comment.  Did the old version allow you to specify more
>> than one?  Can you run rpc.nfsd more than once, specifying a  
>> different
>> "-H" on each?
>
> The only option that matters once nfsd is up is the number of threads
> -- any others are ignored. This applies to the -H option as well, so
> it'll only matter for the first run of rpc.nfsd. You could specify -H
> multiple times on a single nfsd run, but only the last one would
> matter. With this patch, I changed it to be the first one since that
> seemed more intuitive to me.

Everything else I'm aware of (like mount.nfs) uses "rightmost wins."   
So the last "-H" would be the one that takes effect.  One way might be  
more intuitive than another, but it seems like everything else takes  
the last command-line option, not the first.

> We could eventually allow nfsd to take multiple -H arguments, but
> that's really outside the scope of this set.

Ignoring all but one makes sense.  The comment suggested that rpc.nfsd  
might have taken more than one at one point.

>>> +
>>> +	xlog_open("nfsd");
>>
>> How about "rpc.nfsd" or even argv[0] ?
>
> rpc.nfsd is probably easier. basename(argv[0]) is probably the best
> thing though. I'll fix it up to use that.
>
>>> +	/*
>>> +	 * KLUDGE ALERT:
>>> +	 * Some kernels let nfsd kernel threads inherit open files
>>> +	 * from the program that spawns them (i.e. us).  So close
>>> +	 * everything before spawning kernel threads.  --Chip
>>> +	 */
>>> 	xlog_syslog(1);
>>> 	xlog_stderr(0);
>>> 	fd = open("/dev/null", O_RDWR);
>>> 	if (fd == -1)
>>> -		perror("/dev/null");
>>> +		xlog(L_ERROR, "unable to open /dev/null: %s", strerror(errno));
>>
>> ("%m") does the same thing as ("%s", strerror(errno))
>>
>
> Nice...ok I'll change it (and other spots if there are any).
>
>>>
>>> 	else {
>>> 		(void) dup2(fd, 0);
>>> 		(void) dup2(fd, 1);
>>
>> Should you check the return code from these?
>>
>
> The existing code doesn't. If we want to change that it should prob be
> a separate patch. I wonder though whether we need to do all of this
> stuff with /dev/null. Stupid question: is there some reason we can't
> just close stdin/stdout/stderr?

It's hard to say what's right.  openlog(3) creates an open file  
descriptor, for instance, so that would be inherited by the kernel in  
this case if closelog(3) wasn't done.  The question is whether we  
still care about kernels that old.

I think closing them all, and re-opening if we need to report an  
error, is OK.  I saw some evidence recently that closeall() wasn't  
working at all, so you might want to check that.

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

Re: [PATCH 5/6] nfs-utils: add IPv6 support to nfsd

[Jeff Layton]

On Tue, 2 Jun 2009 15:13:15 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 2, 2009, at 2:55 PM, Jeff Layton wrote:
> 
> > On Tue, 2 Jun 2009 11:35:18 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> > Forgot a couple of comments from the earlier email:
> >
> >>> 		case 'H':
> >>> -			if (inet_addr(optarg) != INADDR_NONE) {
> >>> +			/*
> >>> +			 * for now, this only handles one -H option. Use the
> >>> +			 * first one specified.
> >>> +			 */
> >>
> >> Interesting comment.  Did the old version allow you to specify more
> >> than one?  Can you run rpc.nfsd more than once, specifying a  
> >> different
> >> "-H" on each?
> >
> > The only option that matters once nfsd is up is the number of threads
> > -- any others are ignored. This applies to the -H option as well, so
> > it'll only matter for the first run of rpc.nfsd. You could specify -H
> > multiple times on a single nfsd run, but only the last one would
> > matter. With this patch, I changed it to be the first one since that
> > seemed more intuitive to me.
> 
> Everything else I'm aware of (like mount.nfs) uses "rightmost wins."   
> So the last "-H" would be the one that takes effect.  One way might be  
> more intuitive than another, but it seems like everything else takes  
> the last command-line option, not the first.
> 
> > We could eventually allow nfsd to take multiple -H arguments, but
> > that's really outside the scope of this set.
> 
> Ignoring all but one makes sense.  The comment suggested that rpc.nfsd  
> might have taken more than one at one point.
> 
> >>> +
> >>> +	xlog_open("nfsd");
> >>
> >> How about "rpc.nfsd" or even argv[0] ?
> >
> > rpc.nfsd is probably easier. basename(argv[0]) is probably the best
> > thing though. I'll fix it up to use that.
> >
> >>> +	/*
> >>> +	 * KLUDGE ALERT:
> >>> +	 * Some kernels let nfsd kernel threads inherit open files
> >>> +	 * from the program that spawns them (i.e. us).  So close
> >>> +	 * everything before spawning kernel threads.  --Chip
> >>> +	 */
> >>> 	xlog_syslog(1);
> >>> 	xlog_stderr(0);
> >>> 	fd = open("/dev/null", O_RDWR);
> >>> 	if (fd == -1)
> >>> -		perror("/dev/null");
> >>> +		xlog(L_ERROR, "unable to open /dev/null: %s", strerror(errno));
> >>
> >> ("%m") does the same thing as ("%s", strerror(errno))
> >>
> >
> > Nice...ok I'll change it (and other spots if there are any).
> >
> >>>
> >>> 	else {
> >>> 		(void) dup2(fd, 0);
> >>> 		(void) dup2(fd, 1);
> >>
> >> Should you check the return code from these?
> >>
> >
> > The existing code doesn't. If we want to change that it should prob be
> > a separate patch. I wonder though whether we need to do all of this
> > stuff with /dev/null. Stupid question: is there some reason we can't
> > just close stdin/stdout/stderr?
> 
> It's hard to say what's right.  openlog(3) creates an open file  
> descriptor, for instance, so that would be inherited by the kernel in  
> this case if closelog(3) wasn't done.  The question is whether we  
> still care about kernels that old.
>
> I think closing them all, and re-opening if we need to report an  
> error, is OK.  I saw some evidence recently that closeall() wasn't  
> working at all, so you might want to check that.
> 

Heh...as a matter of fact you seem to be right. closeall isn't doing
what it's supposed to be doing for me either.

Given that this is broken (and apparently has been for a long time),
maybe it's better to just not worry about closing those fd's? At some
point, I think we have to tell people with old kernels to take a hike
wrt to newer userland code. ;)

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 6/6] nfs-utils: update the nfsd manpage

[Jeff Layton]

Add some clarification about the purpose of the program, info about the
--debug option, and a note about how it behaves when TI-RPC support is
built in.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/nfsd/nfsd.man |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
index 4eb3e21..94b041e 100644
--- a/utils/nfsd/nfsd.man
+++ b/utils/nfsd/nfsd.man
@@ -13,8 +13,9 @@ The
 program implements the user level part of the NFS service. The
 main functionality is handled by the
 .B nfsd
-kernel module; the user space program merely starts the specified
-number of kernel threads.
+kernel module. The user space program merely specifies what sort of sockets
+the kernel service should listen on, what NFS versions it should support, and
+how many kernel threads it should use.
 .P
 The
 .B rpc.mountd
@@ -22,6 +23,11 @@ server provides an ancillary service needed to satisfy mount requests
 by NFS clients.
 .SH OPTIONS
 .TP
+.B \-d " or " \-\-debug
+enable debug log messages and log messages to stderr instead of syslog.
+By default, nfsd logs all messages to syslog, except for errors generated
+during option processing which go to stderr.
+.TP
 .B \-H " or " \-\-host  hostname
 specify a particular hostname (or address) that NFS requests will
 be accepted on. By default,
@@ -75,12 +81,19 @@ In particular
 .B rpc.nfsd 0
 will stop all threads and thus close any open connections.
 
+.SH NOTES
+If the program is built with TI-RPC support, it will enable any protocol and
+address family combinations that are marked visible in the
+.B netconfig
+database.
+
 .SH SEE ALSO
 .BR rpc.mountd (8),
 .BR exports (5),
 .BR exportfs (8),
 .BR rpc.rquotad (8),
-.BR nfsstat (8).
+.BR nfsstat (8),
+.BR netconfig(5).
 .SH AUTHOR
 Olaf Kirch, Bill Hawes, H. J. Lu, G. Allan Morris III,
 and a host of others.
-- 
1.6.0.6