[PATCH 00/10] nfs-utils: add IPv6 support to rpc.nfsd (try #5)

[PATCH 00/10] nfs-utils: add IPv6 support to rpc.nfsd (try #5)

[Jeff Layton]

This is the fifth 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 relatively minor cleanups and
refinements.

I've also taken greater care to break up the patchset into smaller
pieces. This does mean that multiple patches touch the same areas of
code but the individual patches should be a bit smaller.

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 (10):
  nfs-utils: don't link libexport.a and libmisc.a to nfsd
  nfs-utils: clean up option parsing in nfsd.c
  nfs-utils: convert rpc.nfsd to use xlog()
  nfs-utils: clean up NFSCTL_* macros for handling protocol bits
  nfs-utils: move check for active knfsd to helper function
  nfs-utils: convert nfssvc_setfds to use getaddrinfo
  nfs-utils: break up the nfssvc interface
  nfs-utils: add IPv6 support to nfssvc_setfds
  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      |  261 +++++++++++++++++++++++++++++++--------------
 utils/nfsd/Makefile.am    |    4 +-
 utils/nfsd/nfsd.c         |  258 ++++++++++++++++++++++++++++++++------------
 utils/nfsd/nfsd.man       |   19 +++-
 6 files changed, 401 insertions(+), 163 deletions(-)

[PATCH 01/10] 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.2.2

[PATCH 02/10] 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.2.2

[PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Jeff Layton]

...and add a --debug option to make nfsd log messages to stderr.

rpc.nfsd is usually run out of init scripts in which case logging to
syslog makes more sense. Make that the default and add a switch that
makes log messages go to stderr. Eventually however nfsd has to close
stderr, at which point the program switches to logging to syslog
unconditionally.

For now, stderr gets closed rather early, so --debug isn't terribly
helpful. Later patches will make rpc.nfsd delay closing of stderr longer
which should make it more useful.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 support/nfs/nfssvc.c |   46 +++++++++++++++++-------------------
 utils/nfsd/nfsd.c    |   63 +++++++++++++++++++++++++++++++-------------------
 2 files changed, 61 insertions(+), 48 deletions(-)

diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 33c15a7..3e6bd31 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -16,10 +16,9 @@
 #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"
@@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *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));
+			xlog(L_ERROR, "unable to create UDP socket: "
+				"errno %d (%m)", 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));
+			xlog(L_ERROR, "unable to bind UDP socket: "
+				"errno %d (%m)", errno);
 			exit(1);
 		}
 	}
@@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
 	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));
+			xlog(L_ERROR, "unable to createt tcp socket: "
+				"errno %d (%m)", errno);
 			exit(1);
 		}
 		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));
+			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
+				"errno %d (%m)", errno);
 			exit(1);
 		}
 		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));
+			xlog(L_ERROR, "unable to bind TCP socket: "
+				"errno %d (%m)", errno);
 			exit(1);
 		}
 		if (listen(tcpfd, 64) < 0){
-			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
-				"errno %d (%s)\n", errno, strerror(errno));
+			xlog(L_ERROR, "unable to create listening socket: "
+				"errno %d (%m)", errno);
 			exit(1);
 		}
 	}
 	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));
+			xlog(L_ERROR, 
+			       "writing fds to kernel failed: errno %d (%m)", 
+			       errno);
 		}
 		close(fd);
 		fd = -1;
@@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
 		snprintf(buf, BUFSIZ,"%d\n", tcpfd); 
 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
-			syslog(LOG_ERR, 
-			       "nfssvc: writing fds to kernel failed: errno %d (%s)", 
-			       errno, strerror(errno));
+			xlog(L_ERROR, 
+			       "writing fds to kernel failed: errno %d (%m)", 
+			       errno);
 		}
 	}
 	close(fd);
@@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int minorvers4)
 				    minorvers4 > 0 ? '+' : '-',
 				    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)", 
-			errno, strerror(errno));
-	}
+	if (write(fd, buf, strlen(buf)) != strlen(buf))
+		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
+
 	close(fd);
 
 	return;
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index c7bd003..6dfea67 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,6 +37,7 @@ 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;
@@ -50,7 +51,7 @@ main(int argc, char **argv)
 	int	count = 1, c, error, port, fd, found_one;
 	struct servent *ent;
 	struct hostent *hp;
-	char *p;
+	char *p, *progname;
 
 	ent = getservbyname ("nfs", "udp");
 	if (ent != NULL)
@@ -58,8 +59,22 @@ main(int argc, char **argv)
 	else
 		port = 2049;
 
-	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts, NULL)) != EOF) {
+	progname = strdup(basename(argv[0]));
+	if (!progname) {
+		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
+		exit(1);
+	}
+
+	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) {
 				haddr = strdup(optarg);
@@ -67,8 +82,8 @@ main(int argc, char **argv)
 				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]);
+					progname, optarg);
+				usage(progname);
 			}
 			break;
 		case 'P':	/* XXX for nfs-server compatibility */
@@ -76,8 +91,8 @@ main(int argc, char **argv)
 			port = atoi(optarg);
 			if (port <= 0 || port > 65535) {
 				fprintf(stderr, "%s: bad port number: %s\n",
-					argv[0], optarg);
-				usage(argv [0]);
+					progname, optarg);
+				usage(progname);
 			}
 			break;
 		case 'N':
@@ -105,14 +120,17 @@ main(int argc, char **argv)
 		default:
 			fprintf(stderr, "Invalid argument: '%c'\n", c);
 		case 'h':
-			usage(argv[0]);
+			usage(progname);
 		}
 	}
+
+	xlog_open(progname);
+
 	/*
 	 * Do some sanity checking, if the ctlbits are set
 	 */
 	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
-		fprintf(stderr, "invalid protocol specified\n");
+		xlog(L_ERROR, "invalid protocol specified");
 		exit(1);
 	}
 	found_one = 0;
@@ -121,12 +139,12 @@ main(int argc, char **argv)
 			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,17 +153,15 @@ 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: %m", NFS_STATEDIR);
 		exit(1);
 	}
 
 	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);
+			xlog(L_ERROR, "invalid server count (%d), using 1",
+				      count);
 			count = 1;
 		}
 	}
@@ -155,21 +171,20 @@ main(int argc, char **argv)
 	   everything before spawning kernel threads.  --Chip */
 	fd = open("/dev/null", O_RDWR);
 	if (fd == -1)
-		perror("/dev/null");
+		xlog(L_ERROR, "Unable to open /dev/null: %m");
 	else {
 		(void) dup2(fd, 0);
 		(void) dup2(fd, 1);
 		(void) dup2(fd, 2);
 	}
+	xlog_syslog(1);
+	xlog_stderr(0);
 	closeall(3);
 
-	openlog("nfsd", LOG_PID, LOG_DAEMON);
-	if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0) {
-		int e = errno;
-		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
-		closelog();
-	}
+	if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0)
+		xlog(L_ERROR, "nfssvc (%m)");
 
+	free(progname);
 	return (error != 0);
 }
 
@@ -177,7 +192,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.2.2

[PATCH 04/10] nfs-utils: clean up NFSCTL_* macros for handling protocol bits

[Jeff Layton]

They are a little hard to follow currently. Clean them up and add new
macros that can set these bits in addition to the ones that unset them.

Also add a new macro that reports when any valid protocol bit is set.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 support/include/nfs/nfs.h |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 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 */
-- 
1.6.2.2

[PATCH 05/10] nfs-utils: move check for active knfsd to helper function

[Jeff Layton]

nfssvc_setfds checks to see if knfsd is already running. Move this
check to a helper function. Eventually the nfsd code will call this
directly.

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

diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 3e6bd31..8b15c4d 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -24,28 +24,46 @@
 #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 1;
+
+	return 0;
+}
+
+static void
+nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
+{
+	int fd, n, on=1;
+	char buf[BUFSIZ];
+	int udpfd = -1, tcpfd = -1;
+	struct sockaddr_in sin;
+
+	if (nfssvc_inuse())
 		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.
-	 */
+
 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
 	if (fd < 0)
 		return;
-- 
1.6.2.2

[PATCH 06/10] nfs-utils: convert nfssvc_setfds to use getaddrinfo

[Jeff Layton]

Convert nfssvc_setfds to use getaddrinfo. Change the args that it takes
and fix up nfssvc function to pass in the proper args. The things that
nfssvc has to do to call the new nfssvc_setfds is a little cumbersome
for now, but that will eventually be cleaned up in a later patch.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 support/nfs/nfssvc.c |  191 ++++++++++++++++++++++++++++++++++----------------
 utils/nfsd/nfsd.c    |   18 +++--
 2 files changed, 140 insertions(+), 69 deletions(-)

diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 8b15c4d..168414c 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -10,7 +10,9 @@
 #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>
@@ -53,85 +55,148 @@ nfssvc_inuse(void)
 	return 0;
 }
 
-static void
-nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
+static int
+nfssvc_setfds(const struct addrinfo *hints, const char *node, const char *port)
 {
-	int fd, n, on=1;
+	int fd, on = 1, fac = L_ERROR;
+	int sockfd = -1, rc = 0;
 	char buf[BUFSIZ];
-	int udpfd = -1, tcpfd = -1;
-	struct sockaddr_in sin;
-
-	if (nfssvc_inuse())
-		return;
+	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) {
-			xlog(L_ERROR, "unable to create UDP socket: "
-				"errno %d (%m)", errno);
-			exit(1);
-		}
-		if (bind(udpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
-			xlog(L_ERROR, "unable to bind UDP socket: "
-				"errno %d (%m)", errno);
-			exit(1);
-		}
+		return 0;
+
+	switch(hints->ai_family) {
+	case AF_INET:
+		family = "inet";
+		break;
+	default:
+		xlog(L_ERROR, "Unknown address family specified: %d\n",
+				hints->ai_family);
+		rc = EAFNOSUPPORT;
+		goto error;
 	}
 
-	if (NFSCTL_TCPISSET(ctlbits)) {
-		tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
-		if (tcpfd < 0) {
-			xlog(L_ERROR, "unable to createt tcp socket: "
-				"errno %d (%m)", errno);
-			exit(1);
+	rc = getaddrinfo(node, port, hints, &addrhead);
+	if (rc == EAI_NONAME && !strcmp(port, "nfs")) {
+		snprintf(buf, BUFSIZ, "%d", NFS_PORT);
+		rc = getaddrinfo(node, buf, 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;
+	}
+
+	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) {
-			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
-				"errno %d (%m)", 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 (%m)", family, proto, errno);
+			rc = errno;
+			goto error;
 		}
-		if (bind(tcpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
-			xlog(L_ERROR, "unable to bind TCP socket: "
-				"errno %d (%m)", 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 (%m)", family, errno);
+			rc = errno;
+			goto error;
+		}
+		if (bind(sockfd, addr->ai_addr, addr->ai_addrlen)) {
+			xlog(L_ERROR, "unable to bind %s %s socket: "
+				"errno %d (%m)", family, proto, errno);
+			rc = errno;
+			goto error;
 		}
-		if (listen(tcpfd, 64) < 0){
+		if (addr->ai_protocol == IPPROTO_TCP && listen(sockfd, 64)) {
 			xlog(L_ERROR, "unable to create listening socket: "
 				"errno %d (%m)", errno);
-			exit(1);
+			rc = errno;
+			goto error;
 		}
-	}
-	if (udpfd >= 0) {
-		snprintf(buf, BUFSIZ,"%d\n", udpfd); 
-		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
-			xlog(L_ERROR, 
-			       "writing fds to kernel failed: errno %d (%m)", 
-			       errno);
-		}
-		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 (%m)", errno);
+			goto error;
+		}
+		snprintf(buf, BUFSIZ, "%d\n", sockfd); 
 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
-			xlog(L_ERROR, 
-			       "writing fds to kernel failed: errno %d (%m)", 
-			       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 (%m)",
+				  errno);
+			rc = errno;
+			goto error;
 		}
+		close(fd);
+		close(sockfd);
+		sockfd = fd = -1;
+		addr = addr->ai_next;
 	}
-	close(fd);
+error:
+	if (fd >= 0)
+		close(fd);
+	if (sockfd >= 0)
+		close(sockfd);
+	if (addrhead)
+		freeaddrinfo(addrhead);
+	return rc;
+}
 
-	return;
+static int
+nfssvc_set_sockets(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 EPROTOTYPE;
+	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);
 }
+
 static void
 nfssvc_versbits(unsigned int ctlbits, int minorvers4)
 {
@@ -169,13 +234,17 @@ nfssvc(int port, int nrservs, unsigned int versbits, int minorvers4,
 {
 	struct nfsctl_arg	arg;
 	int fd;
+	char portstr[BUFSIZ];
 
 	/* 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);
+	if (!nfssvc_inuse()) {
+		nfssvc_versbits(versbits, minorvers4);
+		snprintf(portstr, BUFSIZ, "%d", port);
+		nfssvc_set_sockets(AF_INET, protobits, haddr, portstr);
+	}
 
 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
 	if (fd < 0)
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 6dfea67..e9d0bf9 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -76,14 +76,16 @@ main(int argc, char **argv)
 			xlog_stderr(1);
 			break;
 		case 'H':
-			if (inet_addr(optarg) != INADDR_NONE) {
-				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",
-					progname, optarg);
-				usage(progname);
+			/*
+			 * for now, this only handles one -H option. Use the
+			 * last one specified.
+			 */
+			free(haddr);
+			haddr = strdup(optarg);
+			if (!haddr) {
+				fprintf(stderr, "%s: unable to allocate "
+					"memory.\n", progname);
+				exit(1);
 			}
 			break;
 		case 'P':	/* XXX for nfs-server compatibility */
-- 
1.6.2.2

[PATCH 07/10] nfs-utils: break up the nfssvc interface

[Jeff Layton]

Currently, the only public interface to the routines in nfssvc.c is
nfssvc(). This means that we do an awful lot of work after closing
stderr that could be done while it's still available.

Add prototypes to the header so that more functions in nfssvc.c can be
called individually, and change the nfsd program to call those routines
individually.

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

diff --git a/support/include/nfslib.h b/support/include/nfslib.h
index ae98650..14b8ba9 100644
--- a/support/include/nfslib.h
+++ b/support/include/nfslib.h
@@ -130,7 +130,12 @@ 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_set_sockets(const int family,
+					const unsigned int protobits,
+					const char *host, 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 168414c..89fb5be 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -179,7 +179,7 @@ error:
 	return rc;
 }
 
-static int
+int
 nfssvc_set_sockets(const int family, const unsigned int protobits,
 		   const char *host, const char *port)
 {
@@ -197,8 +197,8 @@ nfssvc_set_sockets(const int family, const unsigned int protobits,
 	return nfssvc_setfds(&hints, host, port);
 }
 
-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;
@@ -228,23 +228,13 @@ nfssvc_versbits(unsigned int ctlbits, int minorvers4)
 
 	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;
-	char portstr[BUFSIZ];
-
-	/* Note: must set versions before fds so that
-	 * the ports get registered with portmap against correct
-	 * versions
-	 */
-	if (!nfssvc_inuse()) {
-		nfssvc_versbits(versbits, minorvers4);
-		snprintf(portstr, BUFSIZ, "%d", port);
-		nfssvc_set_sockets(AF_INET, protobits, haddr, portstr);
-	}
 
 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
 	if (fd < 0)
@@ -265,6 +255,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 = NFS_PORT;
+	}
+
 	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 e9d0bf9..012847b 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -43,21 +43,14 @@ static struct option longopts[] =
 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, *progname;
-
-	ent = getservbyname ("nfs", "udp");
-	if (ent != NULL)
-		port = ntohs (ent->s_port);
-	else
-		port = 2049;
+	int	count = 1, c, error, portnum = 0, fd, found_one;
+	char *p, *progname, *port;
+	char *haddr = NULL;
+	int	socket_up = 0;
 
 	progname = strdup(basename(argv[0]));
 	if (!progname) {
@@ -65,6 +58,12 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
+	port = strdup("nfs");
+	if (!port) {
+		fprintf(stderr, "%s: unable to allocate memory.\n", progname);
+		exit(1);
+	}
+
 	xlog_syslog(1);
 	xlog_stderr(0);
 
@@ -90,12 +89,20 @@ main(int argc, char **argv)
 			break;
 		case 'P':	/* XXX for nfs-server compatibility */
 		case 'p':
-			port = atoi(optarg);
-			if (port <= 0 || port > 65535) {
+			/* only the last -p option has any effect */
+			portnum = atoi(optarg);
+			if (portnum <= 0 || portnum > 65535) {
 				fprintf(stderr, "%s: bad port number: %s\n",
 					progname, optarg);
 				usage(progname);
 			}
+			free(port);
+			port = strdup(optarg);
+			if (!port) {
+				fprintf(stderr, "%s: unable to allocate "
+						"memory.\n", progname);
+				exit(1);
+			}
 			break;
 		case 'N':
 			switch((c = strtol(optarg, &p, 0))) {
@@ -167,10 +174,38 @@ main(int argc, char **argv)
 			count = 1;
 		}
 	}
-	/* 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 */
+
+	/* can only change number of threads if nfsd is already up */
+	if (nfssvc_inuse()) {
+		socket_up = 1;
+		goto set_threads;
+	}
+
+	/*
+	 * 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.
+	 */
+	nfssvc_setvers(versbits, minorvers4);
+
+	error = nfssvc_set_sockets(AF_INET, protobits, haddr, port);
+	if (!error)
+		socket_up = 1;
+
+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
+	 */
 	fd = open("/dev/null", O_RDWR);
 	if (fd == -1)
 		xlog(L_ERROR, "Unable to open /dev/null: %m");
@@ -183,9 +218,11 @@ main(int argc, char **argv)
 	xlog_stderr(0);
 	closeall(3);
 
-	if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0)
-		xlog(L_ERROR, "nfssvc (%m)");
-
+	if ((error = nfssvc_threads(portnum, count)) < 0)
+		xlog(L_ERROR, "error starting threads: %s", strerror(error));
+out:
+	free(port);
+	free(haddr);
 	free(progname);
 	return (error != 0);
 }
-- 
1.6.2.2

[PATCH 08/10] nfs-utils: add IPv6 support to nfssvc_setfds

[Jeff Layton]

Allow nfssvc_setfds to properly deal with AF_INET6.

IPv6 sockets for knfsd can't be allowed to accept IPv4 packets. Set the
correct option to prevent that from occurring on IPv6 sockets.

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

diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
index 89fb5be..19f1a4c 100644
--- a/support/nfs/nfssvc.c
+++ b/support/nfs/nfssvc.c
@@ -76,6 +76,11 @@ nfssvc_setfds(const struct addrinfo *hints, const char *node, const char *port)
 	case AF_INET:
 		family = "inet";
 		break;
+#ifdef IPV6_SUPPORTED
+	case AF_INET6:
+		family = "inet6";
+		break;
+#endif /* IPV6_SUPPORTED */
 	default:
 		xlog(L_ERROR, "Unknown address family specified: %d\n",
 				hints->ai_family);
@@ -123,6 +128,15 @@ nfssvc_setfds(const struct addrinfo *hints, const char *node, const char *port)
 			rc = errno;
 			goto error;
 		}
+#ifdef IPV6_SUPPORTED
+		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;
+		}
+#endif /* IPV6_SUPPORTED */
 		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.2.2

[PATCH 09/10] 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>
---
 utils/nfsd/nfsd.c |  114 +++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 89 insertions(+), 25 deletions(-)

diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 012847b..8d753f2 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -40,17 +40,63 @@ 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 */
 
 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, *progname, *port;
 	char *haddr = NULL;
 	int	socket_up = 0;
+	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;
 
 	progname = strdup(basename(argv[0]));
 	if (!progname) {
@@ -133,15 +179,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(progname);
 
-	/*
-	 * Do some sanity checking, if the ctlbits are set
-	 */
-	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
-		xlog(L_ERROR, "invalid protocol specified");
-		exit(1);
+	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))
@@ -152,29 +221,18 @@ 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: %m", NFS_STATEDIR);
 		exit(1);
 	}
 
-	if (optind < argc) {
-		if ((count = atoi(argv[optind])) < 0) {
-			/* insane # of servers */
-			xlog(L_ERROR, "invalid server count (%d), using 1",
-				      count);
-			count = 1;
-		}
-	}
-
 	/* can only change number of threads if nfsd is already up */
 	if (nfssvc_inuse()) {
 		socket_up = 1;
@@ -187,10 +245,16 @@ main(int argc, char **argv)
 	 * interfaces, these are a no-op.
 	 */
 	nfssvc_setvers(versbits, minorvers4);
+ 
+	error = nfssvc_set_sockets(AF_INET, proto4, haddr, port);
+	if (!error)
+		socket_up = 1;
 
-	error = nfssvc_set_sockets(AF_INET, protobits, haddr, port);
+#ifdef IPV6_SUPPORTED
+	error = nfssvc_set_sockets(AF_INET6, proto6, haddr, port);
 	if (!error)
 		socket_up = 1;
+#endif /* IPV6_SUPPORTED */
 
 set_threads:
 	/* don't start any threads if unable to hand off any sockets */
-- 
1.6.2.2

[PATCH 10/10] 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.2.2

Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Chuck Lever]

On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:

> ...and add a --debug option to make nfsd log messages to stderr.
>
> rpc.nfsd is usually run out of init scripts in which case logging to
> syslog makes more sense. Make that the default and add a switch that
> makes log messages go to stderr. Eventually however nfsd has to close
> stderr, at which point the program switches to logging to syslog
> unconditionally.
>
> For now, stderr gets closed rather early, so --debug isn't terribly
> helpful. Later patches will make rpc.nfsd delay closing of stderr  
> longer
> which should make it more useful.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/nfs/nfssvc.c |   46 +++++++++++++++++-------------------
> utils/nfsd/nfsd.c    |   63 ++++++++++++++++++++++++++++++ 
> +-------------------
> 2 files changed, 61 insertions(+), 48 deletions(-)
>
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 33c15a7..3e6bd31 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -16,10 +16,9 @@
> #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"
> @@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> char *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));
> +			xlog(L_ERROR, "unable to create UDP socket: "
> +				"errno %d (%m)", 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));
> +			xlog(L_ERROR, "unable to bind UDP socket: "
> +				"errno %d (%m)", errno);
> 			exit(1);
> 		}
> 	}
> @@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> char *haddr)
> 	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));
> +			xlog(L_ERROR, "unable to createt tcp socket: "
> +				"errno %d (%m)", errno);
> 			exit(1);
> 		}
> 		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));
> +			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
> +				"errno %d (%m)", errno);
> 			exit(1);
> 		}
> 		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));
> +			xlog(L_ERROR, "unable to bind TCP socket: "
> +				"errno %d (%m)", errno);
> 			exit(1);
> 		}
> 		if (listen(tcpfd, 64) < 0){
> -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
> -				"errno %d (%s)\n", errno, strerror(errno));
> +			xlog(L_ERROR, "unable to create listening socket: "
> +				"errno %d (%m)", errno);
> 			exit(1);
> 		}
> 	}
> 	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));
> +			xlog(L_ERROR,
> +			       "writing fds to kernel failed: errno %d (%m)",
> +			       errno);
> 		}
> 		close(fd);
> 		fd = -1;
> @@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> char *haddr)
> 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
> 		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
> 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> -			syslog(LOG_ERR,
> -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> -			       errno, strerror(errno));
> +			xlog(L_ERROR,
> +			       "writing fds to kernel failed: errno %d (%m)",
> +			       errno);
> 		}
> 	}
> 	close(fd);
> @@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int  
> minorvers4)
> 				    minorvers4 > 0 ? '+' : '-',
> 				    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)",
> -			errno, strerror(errno));
> -	}
> +	if (write(fd, buf, strlen(buf)) != strlen(buf))
> +		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
> +
> 	close(fd);
>
> 	return;
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index c7bd003..6dfea67 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,6 +37,7 @@ 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;
> @@ -50,7 +51,7 @@ main(int argc, char **argv)
> 	int	count = 1, c, error, port, fd, found_one;
> 	struct servent *ent;
> 	struct hostent *hp;
> -	char *p;
> +	char *p, *progname;
>
> 	ent = getservbyname ("nfs", "udp");
> 	if (ent != NULL)
> @@ -58,8 +59,22 @@ main(int argc, char **argv)
> 	else
> 		port = 2049;
>
> -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,  
> NULL)) != EOF) {
> +	progname = strdup(basename(argv[0]));
> +	if (!progname) {
> +		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
> +		exit(1);
> +	}
> +
> +	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) {
> 				haddr = strdup(optarg);
> @@ -67,8 +82,8 @@ main(int argc, char **argv)
> 				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]);
> +					progname, optarg);
> +				usage(progname);
> 			}
> 			break;
> 		case 'P':	/* XXX for nfs-server compatibility */
> @@ -76,8 +91,8 @@ main(int argc, char **argv)
> 			port = atoi(optarg);
> 			if (port <= 0 || port > 65535) {
> 				fprintf(stderr, "%s: bad port number: %s\n",
> -					argv[0], optarg);
> -				usage(argv [0]);
> +					progname, optarg);
> +				usage(progname);
> 			}
> 			break;
> 		case 'N':
> @@ -105,14 +120,17 @@ main(int argc, char **argv)
> 		default:
> 			fprintf(stderr, "Invalid argument: '%c'\n", c);
> 		case 'h':
> -			usage(argv[0]);
> +			usage(progname);
> 		}
> 	}
> +
> +	xlog_open(progname);
> +
> 	/*
> 	 * Do some sanity checking, if the ctlbits are set
> 	 */
> 	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> -		fprintf(stderr, "invalid protocol specified\n");
> +		xlog(L_ERROR, "invalid protocol specified");
> 		exit(1);
> 	}
> 	found_one = 0;
> @@ -121,12 +139,12 @@ main(int argc, char **argv)
> 			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,17 +153,15 @@ 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: %m", NFS_STATEDIR);
> 		exit(1);
> 	}
>
> 	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);
> +			xlog(L_ERROR, "invalid server count (%d), using 1",
> +				      count);
> 			count = 1;
> 		}
> 	}
> @@ -155,21 +171,20 @@ main(int argc, char **argv)
> 	   everything before spawning kernel threads.  --Chip */
> 	fd = open("/dev/null", O_RDWR);
> 	if (fd == -1)
> -		perror("/dev/null");
> +		xlog(L_ERROR, "Unable to open /dev/null: %m");
> 	else {
> 		(void) dup2(fd, 0);
> 		(void) dup2(fd, 1);
> 		(void) dup2(fd, 2);
> 	}
> +	xlog_syslog(1);
> +	xlog_stderr(0);
> 	closeall(3);
>
> -	openlog("nfsd", LOG_PID, LOG_DAEMON);
> -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> haddr)) < 0) {
> -		int e = errno;
> -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
> -		closelog();
> -	}
> +	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> haddr)) < 0)
> +		xlog(L_ERROR, "nfssvc (%m)");

Nit: Maybe get rid of the "nfssvc" here as well?

>
>
> +	free(progname);

You go to the trouble of freeing progname here, but you exit in some  
cases in the code above without freeing.  I usually copy argv[0] into  
a static buffer to avoid all that strdup(3) bother.

> 	return (error != 0);
> }
>
> @@ -177,7 +192,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.2.2
>

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

Re: [PATCH 08/10] nfs-utils: add IPv6 support to nfssvc_setfds

[Chuck Lever]

On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:

> Allow nfssvc_setfds to properly deal with AF_INET6.
>
> IPv6 sockets for knfsd can't be allowed to accept IPv4 packets. Set  
> the
> correct option to prevent that from occurring on IPv6 sockets.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/nfs/nfssvc.c |   14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 89fb5be..19f1a4c 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -76,6 +76,11 @@ nfssvc_setfds(const struct addrinfo *hints, const  
> char *node, const char *port)
> 	case AF_INET:
> 		family = "inet";
> 		break;
> +#ifdef IPV6_SUPPORTED
> +	case AF_INET6:
> +		family = "inet6";
> +		break;
> +#endif /* IPV6_SUPPORTED */
> 	default:
> 		xlog(L_ERROR, "Unknown address family specified: %d\n",
> 				hints->ai_family);
> @@ -123,6 +128,15 @@ nfssvc_setfds(const struct addrinfo *hints,  
> const char *node, const char *port)
> 			rc = errno;
> 			goto error;
> 		}
> +#ifdef IPV6_SUPPORTED
> +		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));

I'm still looking at the other patches, but I notice you could use %m  
here too.

> +			rc = -errno;
> +			goto error;
> +		}
> +#endif /* IPV6_SUPPORTED */
> 		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.2.2
>

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

Re: [PATCH 08/10] nfs-utils: add IPv6 support to nfssvc_setfds

[Steve Dickson]

Chuck Lever wrote:
> 
> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
> 
>> Allow nfssvc_setfds to properly deal with AF_INET6.
>>
>> IPv6 sockets for knfsd can't be allowed to accept IPv4 packets. Set the
>> correct option to prevent that from occurring on IPv6 sockets.
>>
>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> ---
>> support/nfs/nfssvc.c |   14 ++++++++++++++
>> 1 files changed, 14 insertions(+), 0 deletions(-)
>>
>> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
>> index 89fb5be..19f1a4c 100644
>> --- a/support/nfs/nfssvc.c
>> +++ b/support/nfs/nfssvc.c
>> @@ -76,6 +76,11 @@ nfssvc_setfds(const struct addrinfo *hints, const
>> char *node, const char *port)
>>     case AF_INET:
>>         family = "inet";
>>         break;
>> +#ifdef IPV6_SUPPORTED
>> +    case AF_INET6:
>> +        family = "inet6";
>> +        break;
>> +#endif /* IPV6_SUPPORTED */
>>     default:
>>         xlog(L_ERROR, "Unknown address family specified: %d\n",
>>                 hints->ai_family);
>> @@ -123,6 +128,15 @@ nfssvc_setfds(const struct addrinfo *hints, const
>> char *node, const char *port)
>>             rc = errno;
>>             goto error;
>>         }
>> +#ifdef IPV6_SUPPORTED
>> +        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));
> 
> I'm still looking at the other patches, but I notice you could use %m
> here too.
I kinda like having both... the errno *and* the error string...

steved.

Re: [PATCH 08/10] nfs-utils: add IPv6 support to nfssvc_setfds

[Steve Dickson]

Steve Dickson wrote:
>>> +            xlog(L_ERROR, "unable to set IPV6_V6ONLY: "
>>> +                "errno %d (%s)\n", errno, strerror(errno));
>> I'm still looking at the other patches, but I notice you could use %m
>> here too.
> I kinda like having both... the errno *and* the error string...
> 
Ah... I see your point... sometimes '%m' is used and sometimes 
strerror() is used... I'll clean that up... and switch 
everything to strerror().... 

steved.

Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Jeff Layton]

On Wed, 3 Jun 2009 16:01:24 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
> 
> > ...and add a --debug option to make nfsd log messages to stderr.
> >
> > rpc.nfsd is usually run out of init scripts in which case logging to
> > syslog makes more sense. Make that the default and add a switch that
> > makes log messages go to stderr. Eventually however nfsd has to close
> > stderr, at which point the program switches to logging to syslog
> > unconditionally.
> >
> > For now, stderr gets closed rather early, so --debug isn't terribly
> > helpful. Later patches will make rpc.nfsd delay closing of stderr  
> > longer
> > which should make it more useful.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > support/nfs/nfssvc.c |   46 +++++++++++++++++-------------------
> > utils/nfsd/nfsd.c    |   63 ++++++++++++++++++++++++++++++ 
> > +-------------------
> > 2 files changed, 61 insertions(+), 48 deletions(-)
> >
> > diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> > index 33c15a7..3e6bd31 100644
> > --- a/support/nfs/nfssvc.c
> > +++ b/support/nfs/nfssvc.c
> > @@ -16,10 +16,9 @@
> > #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"
> > @@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> > char *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));
> > +			xlog(L_ERROR, "unable to create UDP socket: "
> > +				"errno %d (%m)", 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));
> > +			xlog(L_ERROR, "unable to bind UDP socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 	}
> > @@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> > char *haddr)
> > 	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));
> > +			xlog(L_ERROR, "unable to createt tcp socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 		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));
> > +			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 		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));
> > +			xlog(L_ERROR, "unable to bind TCP socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 		if (listen(tcpfd, 64) < 0){
> > -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > +			xlog(L_ERROR, "unable to create listening socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 	}
> > 	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));
> > +			xlog(L_ERROR,
> > +			       "writing fds to kernel failed: errno %d (%m)",
> > +			       errno);
> > 		}
> > 		close(fd);
> > 		fd = -1;
> > @@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> > char *haddr)
> > 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
> > 		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
> > 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -			syslog(LOG_ERR,
> > -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> > -			       errno, strerror(errno));
> > +			xlog(L_ERROR,
> > +			       "writing fds to kernel failed: errno %d (%m)",
> > +			       errno);
> > 		}
> > 	}
> > 	close(fd);
> > @@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int  
> > minorvers4)
> > 				    minorvers4 > 0 ? '+' : '-',
> > 				    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)",
> > -			errno, strerror(errno));
> > -	}
> > +	if (write(fd, buf, strlen(buf)) != strlen(buf))
> > +		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
> > +
> > 	close(fd);
> >
> > 	return;
> > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> > index c7bd003..6dfea67 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,6 +37,7 @@ 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;
> > @@ -50,7 +51,7 @@ main(int argc, char **argv)
> > 	int	count = 1, c, error, port, fd, found_one;
> > 	struct servent *ent;
> > 	struct hostent *hp;
> > -	char *p;
> > +	char *p, *progname;
> >
> > 	ent = getservbyname ("nfs", "udp");
> > 	if (ent != NULL)
> > @@ -58,8 +59,22 @@ main(int argc, char **argv)
> > 	else
> > 		port = 2049;
> >
> > -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,  
> > NULL)) != EOF) {
> > +	progname = strdup(basename(argv[0]));
> > +	if (!progname) {
> > +		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
> > +		exit(1);
> > +	}
> > +
> > +	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) {
> > 				haddr = strdup(optarg);
> > @@ -67,8 +82,8 @@ main(int argc, char **argv)
> > 				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]);
> > +					progname, optarg);
> > +				usage(progname);
> > 			}
> > 			break;
> > 		case 'P':	/* XXX for nfs-server compatibility */
> > @@ -76,8 +91,8 @@ main(int argc, char **argv)
> > 			port = atoi(optarg);
> > 			if (port <= 0 || port > 65535) {
> > 				fprintf(stderr, "%s: bad port number: %s\n",
> > -					argv[0], optarg);
> > -				usage(argv [0]);
> > +					progname, optarg);
> > +				usage(progname);
> > 			}
> > 			break;
> > 		case 'N':
> > @@ -105,14 +120,17 @@ main(int argc, char **argv)
> > 		default:
> > 			fprintf(stderr, "Invalid argument: '%c'\n", c);
> > 		case 'h':
> > -			usage(argv[0]);
> > +			usage(progname);
> > 		}
> > 	}
> > +
> > +	xlog_open(progname);
> > +
> > 	/*
> > 	 * Do some sanity checking, if the ctlbits are set
> > 	 */
> > 	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> > -		fprintf(stderr, "invalid protocol specified\n");
> > +		xlog(L_ERROR, "invalid protocol specified");
> > 		exit(1);
> > 	}
> > 	found_one = 0;
> > @@ -121,12 +139,12 @@ main(int argc, char **argv)
> > 			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,17 +153,15 @@ 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: %m", NFS_STATEDIR);
> > 		exit(1);
> > 	}
> >
> > 	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);
> > +			xlog(L_ERROR, "invalid server count (%d), using 1",
> > +				      count);
> > 			count = 1;
> > 		}
> > 	}
> > @@ -155,21 +171,20 @@ main(int argc, char **argv)
> > 	   everything before spawning kernel threads.  --Chip */
> > 	fd = open("/dev/null", O_RDWR);
> > 	if (fd == -1)
> > -		perror("/dev/null");
> > +		xlog(L_ERROR, "Unable to open /dev/null: %m");
> > 	else {
> > 		(void) dup2(fd, 0);
> > 		(void) dup2(fd, 1);
> > 		(void) dup2(fd, 2);
> > 	}
> > +	xlog_syslog(1);
> > +	xlog_stderr(0);
> > 	closeall(3);
> >
> > -	openlog("nfsd", LOG_PID, LOG_DAEMON);
> > -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > haddr)) < 0) {
> > -		int e = errno;
> > -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
> > -		closelog();
> > -	}
> > +	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > haddr)) < 0)
> > +		xlog(L_ERROR, "nfssvc (%m)");
> 
> Nit: Maybe get rid of the "nfssvc" here as well?
> 

Well, the "nfssvc" here tells us that the nfssvc() call returned an
error. Later patches will clean that up when I break up the nfssvc()
interface. That said though, there's a bug there. %m isn't appropriate
since errno isn't being set. Whoops!

> >
> >
> > +	free(progname);
> 
> You go to the trouble of freeing progname here, but you exit in some  
> cases in the code above without freeing.  I usually copy argv[0] into  
> a static buffer to avoid all that strdup(3) bother.
> 

Six of one, half dozen of another. I try to be good about freeing
memory that's been allocated in case this code gets reorganized in the
future. The existing code is pretty bad about it, but it doesn't
matter much since this program isn't long running.

As far as allocating a static buffer for it, the problem there is that
you don't know how big a buffer you'll need. Using strdup means less
memory is being used.

> > 	return (error != 0);
> > }
> >
> > @@ -177,7 +192,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.2.2
> >
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 08/10] nfs-utils: add IPv6 support to nfssvc_setfds

[Chuck Lever]

On Jun 3, 2009, at 4:16 PM, Steve Dickson wrote:

> Chuck Lever wrote:
>>
>> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
>>
>>> Allow nfssvc_setfds to properly deal with AF_INET6.
>>>
>>> IPv6 sockets for knfsd can't be allowed to accept IPv4 packets.  
>>> Set the
>>> correct option to prevent that from occurring on IPv6 sockets.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> support/nfs/nfssvc.c |   14 ++++++++++++++
>>> 1 files changed, 14 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
>>> index 89fb5be..19f1a4c 100644
>>> --- a/support/nfs/nfssvc.c
>>> +++ b/support/nfs/nfssvc.c
>>> @@ -76,6 +76,11 @@ nfssvc_setfds(const struct addrinfo *hints, const
>>> char *node, const char *port)
>>>    case AF_INET:
>>>        family = "inet";
>>>        break;
>>> +#ifdef IPV6_SUPPORTED
>>> +    case AF_INET6:
>>> +        family = "inet6";
>>> +        break;
>>> +#endif /* IPV6_SUPPORTED */
>>>    default:
>>>        xlog(L_ERROR, "Unknown address family specified: %d\n",
>>>                hints->ai_family);
>>> @@ -123,6 +128,15 @@ nfssvc_setfds(const struct addrinfo *hints,  
>>> const
>>> char *node, const char *port)
>>>            rc = errno;
>>>            goto error;
>>>        }
>>> +#ifdef IPV6_SUPPORTED
>>> +        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));
>>
>> I'm still looking at the other patches, but I notice you could use %m
>> here too.
> I kinda like having both... the errno *and* the error string...

You can use %m and print the errno too.

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

Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Steve Dickson]

Jeff Layton wrote:
> On Wed, 3 Jun 2009 16:01:24 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
>>
>>> ...and add a --debug option to make nfsd log messages to stderr.
>>>
>>> rpc.nfsd is usually run out of init scripts in which case logging to
>>> syslog makes more sense. Make that the default and add a switch that
>>> makes log messages go to stderr. Eventually however nfsd has to close
>>> stderr, at which point the program switches to logging to syslog
>>> unconditionally.
>>>
>>> For now, stderr gets closed rather early, so --debug isn't terribly
>>> helpful. Later patches will make rpc.nfsd delay closing of stderr  
>>> longer
>>> which should make it more useful.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> support/nfs/nfssvc.c |   46 +++++++++++++++++-------------------
>>> utils/nfsd/nfsd.c    |   63 ++++++++++++++++++++++++++++++ 
>>> +-------------------
>>> 2 files changed, 61 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
>>> index 33c15a7..3e6bd31 100644
>>> --- a/support/nfs/nfssvc.c
>>> +++ b/support/nfs/nfssvc.c
>>> @@ -16,10 +16,9 @@
>>> #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"
>>> @@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
>>> char *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));
>>> +			xlog(L_ERROR, "unable to create UDP socket: "
>>> +				"errno %d (%m)", 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));
>>> +			xlog(L_ERROR, "unable to bind UDP socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 	}
>>> @@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
>>> char *haddr)
>>> 	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));
>>> +			xlog(L_ERROR, "unable to createt tcp socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 		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));
>>> +			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 		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));
>>> +			xlog(L_ERROR, "unable to bind TCP socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 		if (listen(tcpfd, 64) < 0){
>>> -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
>>> -				"errno %d (%s)\n", errno, strerror(errno));
>>> +			xlog(L_ERROR, "unable to create listening socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 	}
>>> 	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));
>>> +			xlog(L_ERROR,
>>> +			       "writing fds to kernel failed: errno %d (%m)",
>>> +			       errno);
>>> 		}
>>> 		close(fd);
>>> 		fd = -1;
>>> @@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
>>> char *haddr)
>>> 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
>>> 		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
>>> 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
>>> -			syslog(LOG_ERR,
>>> -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
>>> -			       errno, strerror(errno));
>>> +			xlog(L_ERROR,
>>> +			       "writing fds to kernel failed: errno %d (%m)",
>>> +			       errno);
>>> 		}
>>> 	}
>>> 	close(fd);
>>> @@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int  
>>> minorvers4)
>>> 				    minorvers4 > 0 ? '+' : '-',
>>> 				    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)",
>>> -			errno, strerror(errno));
>>> -	}
>>> +	if (write(fd, buf, strlen(buf)) != strlen(buf))
>>> +		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
>>> +
>>> 	close(fd);
>>>
>>> 	return;
>>> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
>>> index c7bd003..6dfea67 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,6 +37,7 @@ 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;
>>> @@ -50,7 +51,7 @@ main(int argc, char **argv)
>>> 	int	count = 1, c, error, port, fd, found_one;
>>> 	struct servent *ent;
>>> 	struct hostent *hp;
>>> -	char *p;
>>> +	char *p, *progname;
>>>
>>> 	ent = getservbyname ("nfs", "udp");
>>> 	if (ent != NULL)
>>> @@ -58,8 +59,22 @@ main(int argc, char **argv)
>>> 	else
>>> 		port = 2049;
>>>
>>> -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,  
>>> NULL)) != EOF) {
>>> +	progname = strdup(basename(argv[0]));
>>> +	if (!progname) {
>>> +		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
>>> +		exit(1);
>>> +	}
>>> +
>>> +	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) {
>>> 				haddr = strdup(optarg);
>>> @@ -67,8 +82,8 @@ main(int argc, char **argv)
>>> 				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]);
>>> +					progname, optarg);
>>> +				usage(progname);
>>> 			}
>>> 			break;
>>> 		case 'P':	/* XXX for nfs-server compatibility */
>>> @@ -76,8 +91,8 @@ main(int argc, char **argv)
>>> 			port = atoi(optarg);
>>> 			if (port <= 0 || port > 65535) {
>>> 				fprintf(stderr, "%s: bad port number: %s\n",
>>> -					argv[0], optarg);
>>> -				usage(argv [0]);
>>> +					progname, optarg);
>>> +				usage(progname);
>>> 			}
>>> 			break;
>>> 		case 'N':
>>> @@ -105,14 +120,17 @@ main(int argc, char **argv)
>>> 		default:
>>> 			fprintf(stderr, "Invalid argument: '%c'\n", c);
>>> 		case 'h':
>>> -			usage(argv[0]);
>>> +			usage(progname);
>>> 		}
>>> 	}
>>> +
>>> +	xlog_open(progname);
>>> +
>>> 	/*
>>> 	 * Do some sanity checking, if the ctlbits are set
>>> 	 */
>>> 	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
>>> -		fprintf(stderr, "invalid protocol specified\n");
>>> +		xlog(L_ERROR, "invalid protocol specified");
>>> 		exit(1);
>>> 	}
>>> 	found_one = 0;
>>> @@ -121,12 +139,12 @@ main(int argc, char **argv)
>>> 			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,17 +153,15 @@ 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: %m", NFS_STATEDIR);
>>> 		exit(1);
>>> 	}
>>>
>>> 	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);
>>> +			xlog(L_ERROR, "invalid server count (%d), using 1",
>>> +				      count);
>>> 			count = 1;
>>> 		}
>>> 	}
>>> @@ -155,21 +171,20 @@ main(int argc, char **argv)
>>> 	   everything before spawning kernel threads.  --Chip */
>>> 	fd = open("/dev/null", O_RDWR);
>>> 	if (fd == -1)
>>> -		perror("/dev/null");
>>> +		xlog(L_ERROR, "Unable to open /dev/null: %m");
>>> 	else {
>>> 		(void) dup2(fd, 0);
>>> 		(void) dup2(fd, 1);
>>> 		(void) dup2(fd, 2);
>>> 	}
>>> +	xlog_syslog(1);
>>> +	xlog_stderr(0);
>>> 	closeall(3);
>>>
>>> -	openlog("nfsd", LOG_PID, LOG_DAEMON);
>>> -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
>>> haddr)) < 0) {
>>> -		int e = errno;
>>> -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
>>> -		closelog();
>>> -	}
>>> +	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
>>> haddr)) < 0)
>>> +		xlog(L_ERROR, "nfssvc (%m)");
>> Nit: Maybe get rid of the "nfssvc" here as well?
Hmm... I would think this needs to be a 
    xlog(LOG_ERR, "nfssvc: errno %d (%s)", e, strerror(e));

>>
> 
> Well, the "nfssvc" here tells us that the nfssvc() call returned an
> error. Later patches will clean that up when I break up the nfssvc()
> interface. That said though, there's a bug there. %m isn't appropriate
> since errno isn't being set. Whoops!
> 
>>>
>>> +	free(progname);
>> You go to the trouble of freeing progname here, but you exit in some  
>> cases in the code above without freeing.  I usually copy argv[0] into  
>> a static buffer to avoid all that strdup(3) bother.
>>
> 
> Six of one, half dozen of another. I try to be good about freeing
> memory that's been allocated in case this code gets reorganized in the
> future. The existing code is pretty bad about it, but it doesn't
> matter much since this program isn't long running.
> 
> As far as allocating a static buffer for it, the problem there is that
> you don't know how big a buffer you'll need. Using strdup means less
> memory is being used.
Since this is not a long standing daemon (i.e. it exits) and as 
long as the free() does cause a problem.. I don't see a problem with it..


steved.

Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Jeff Layton]

On Wed, 3 Jun 2009 16:22:14 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 3 Jun 2009 16:01:24 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> > > -	openlog("nfsd", LOG_PID, LOG_DAEMON);
> > > -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > > haddr)) < 0) {
> > > -		int e = errno;
> > > -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
> > > -		closelog();
> > > -	}
> > > +	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > > haddr)) < 0)
> > > +		xlog(L_ERROR, "nfssvc (%m)");
> > 
> > Nit: Maybe get rid of the "nfssvc" here as well?
> > 
> 
> Well, the "nfssvc" here tells us that the nfssvc() call returned an
> error. Later patches will clean that up when I break up the nfssvc()
> interface. That said though, there's a bug there. %m isn't appropriate
> since errno isn't being set. Whoops!
> 

Ahh nm. In most cases, errno will be set on error. The exception is if
the code ended up using the legacy nfsctl() call. In that case, I don't
think it gets set. Hopefully that's rare enough not to matter much.


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 05/10] nfs-utils: move check for active knfsd to helper function

[Chuck Lever]

On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:

> nfssvc_setfds checks to see if knfsd is already running. Move this
> check to a helper function. Eventually the nfsd code will call this
> directly.

Some of this isn't your fault, but this code could use a little extra  
clean up as you split it up, IMO.

> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/nfs/nfssvc.c |   44 ++++++++++++++++++++++++++++++ 
> +-------------
> 1 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 3e6bd31..8b15c4d 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -24,28 +24,46 @@
> #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;

read(2) returns a ssize_t result, not an int.

> 	char buf[BUFSIZ];

Eesh.  BUFSIZ looks like it's two pages on my system, so adding this  
helper makes the stack requirements in this path over 16KB.

Might be better if we used something a little more stack friendly  
here.  The kernel bounds the size of the read(2) result to  
SIMPLE_TRANSACTION_LIMIT, which is less than a page, for example.   
Making it a static would keep "buf" off the stack, too; or checking  
how much buffer we really need (like only a few bytes, for this  
particular check) might be better.

> -	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);

Nit: Using "sizeof(buf)" might be slightly more maintainable than  
"BUFSIZ".

> 	close(fd);
> +
> 	if (n != 0)

If read(2) returns -1, we return 1 here.  How about "return (n > 0);" ?

> +		return 1;
> +
> +	return 0;
> +}
> +
> +static void
> +nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> +{
> +	int fd, n, on=1;

Another compiler warning issue: "n" is probably now unused in  
nfssvc_setfds().

>
> +	char buf[BUFSIZ];
> +	int udpfd = -1, tcpfd = -1;
> +	struct sockaddr_in sin;
> +
> +	if (nfssvc_inuse())
> 		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.
> -	 */
> +
> 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
> 	if (fd < 0)
> 		return;
> -- 
> 1.6.2.2
>

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

Re: [PATCH 06/10] nfs-utils: convert nfssvc_setfds to use getaddrinfo

[Chuck Lever]

On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:

> Convert nfssvc_setfds to use getaddrinfo. Change the args that it  
> takes
> and fix up nfssvc function to pass in the proper args. The things that
> nfssvc has to do to call the new nfssvc_setfds is a little cumbersome
> for now, but that will eventually be cleaned up in a later patch.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/nfs/nfssvc.c |  191 +++++++++++++++++++++++++++++++++ 
> +----------------
> utils/nfsd/nfsd.c    |   18 +++--
> 2 files changed, 140 insertions(+), 69 deletions(-)
>
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 8b15c4d..168414c 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -10,7 +10,9 @@
> #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>
> @@ -53,85 +55,148 @@ nfssvc_inuse(void)
> 	return 0;
> }
>
> -static void
> -nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> +static int
> +nfssvc_setfds(const struct addrinfo *hints, const char *node, const  
> char *port)
> {
> -	int fd, n, on=1;
> +	int fd, on = 1, fac = L_ERROR;
> +	int sockfd = -1, rc = 0;
> 	char buf[BUFSIZ];

Same comment as earlier; BUFSIZ is probably unnecessarily large.

> -	int udpfd = -1, tcpfd = -1;
> -	struct sockaddr_in sin;
> -
> -	if (nfssvc_inuse())
> -		return;
> +	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) {
> -			xlog(L_ERROR, "unable to create UDP socket: "
> -				"errno %d (%m)", errno);
> -			exit(1);
> -		}
> -		if (bind(udpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> -			xlog(L_ERROR, "unable to bind UDP socket: "
> -				"errno %d (%m)", errno);
> -			exit(1);
> -		}
> +		return 0;
> +
> +	switch(hints->ai_family) {
> +	case AF_INET:
> +		family = "inet";
> +		break;
> +	default:
> +		xlog(L_ERROR, "Unknown address family specified: %d\n",
> +				hints->ai_family);
> +		rc = EAFNOSUPPORT;
> +		goto error;
> 	}
>
> -	if (NFSCTL_TCPISSET(ctlbits)) {
> -		tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> -		if (tcpfd < 0) {
> -			xlog(L_ERROR, "unable to createt tcp socket: "
> -				"errno %d (%m)", errno);
> -			exit(1);
> +	rc = getaddrinfo(node, port, hints, &addrhead);
> +	if (rc == EAI_NONAME && !strcmp(port, "nfs")) {
> +		snprintf(buf, BUFSIZ, "%d", NFS_PORT);
> +		rc = getaddrinfo(node, buf, hints, &addrhead);
> +	}

So, I wonder about this semantic.  Should rpc.nfsd fail outright if  
the specified port isn't usable?  If it succeeds, but silently creates  
a listener on a port that wasn't specified, that might be a little  
surprising to some administrators.

Also, passing sizeof(buf) instead of BUFSIZ might be more maintainable.

> +
> +	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;
> +	}
> +
> +	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) {
> -			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
> -				"errno %d (%m)", 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 (%m)", family, proto, errno);
> +			rc = errno;
> +			goto error;
> 		}
> -		if (bind(tcpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> -			xlog(L_ERROR, "unable to bind TCP socket: "
> -				"errno %d (%m)", 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 (%m)", family, errno);
> +			rc = errno;
> +			goto error;
> +		}
> +		if (bind(sockfd, addr->ai_addr, addr->ai_addrlen)) {
> +			xlog(L_ERROR, "unable to bind %s %s socket: "
> +				"errno %d (%m)", family, proto, errno);
> +			rc = errno;
> +			goto error;
> 		}
> -		if (listen(tcpfd, 64) < 0){
> +		if (addr->ai_protocol == IPPROTO_TCP && listen(sockfd, 64)) {
> 			xlog(L_ERROR, "unable to create listening socket: "
> 				"errno %d (%m)", errno);
> -			exit(1);
> +			rc = errno;
> +			goto error;
> 		}
> -	}
> -	if (udpfd >= 0) {
> -		snprintf(buf, BUFSIZ,"%d\n", udpfd);
> -		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> -			xlog(L_ERROR,
> -			       "writing fds to kernel failed: errno %d (%m)",
> -			       errno);
> -		}
> -		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 (%m)", errno);
> +			goto error;
> +		}
> +		snprintf(buf, BUFSIZ, "%d\n", sockfd);
> 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> -			xlog(L_ERROR,
> -			       "writing fds to kernel failed: errno %d (%m)",
> -			       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 (%m)",
> +				  errno);
> +			rc = errno;
> +			goto error;
> 		}
> +		close(fd);
> +		close(sockfd);
> +		sockfd = fd = -1;
> +		addr = addr->ai_next;
> 	}
> -	close(fd);
> +error:
> +	if (fd >= 0)
> +		close(fd);
> +	if (sockfd >= 0)
> +		close(sockfd);
> +	if (addrhead)
> +		freeaddrinfo(addrhead);
> +	return rc;
> +}
>
> -	return;
> +static int
> +nfssvc_set_sockets(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 EPROTOTYPE;
> +	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);
> }
> +
> static void
> nfssvc_versbits(unsigned int ctlbits, int minorvers4)
> {
> @@ -169,13 +234,17 @@ nfssvc(int port, int nrservs, unsigned int  
> versbits, int minorvers4,
> {
> 	struct nfsctl_arg	arg;
> 	int fd;
> +	char portstr[BUFSIZ];

Ditto.

> 	/* 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);
> +	if (!nfssvc_inuse()) {
> +		nfssvc_versbits(versbits, minorvers4);
> +		snprintf(portstr, BUFSIZ, "%d", port);
> +		nfssvc_set_sockets(AF_INET, protobits, haddr, portstr);
> +	}
>
> 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
> 	if (fd < 0)
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 6dfea67..e9d0bf9 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -76,14 +76,16 @@ main(int argc, char **argv)
> 			xlog_stderr(1);
> 			break;
> 		case 'H':
> -			if (inet_addr(optarg) != INADDR_NONE) {
> -				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",
> -					progname, optarg);
> -				usage(progname);
> +			/*
> +			 * for now, this only handles one -H option. Use the
> +			 * last one specified.
> +			 */
> +			free(haddr);
> +			haddr = strdup(optarg);
> +			if (!haddr) {
> +				fprintf(stderr, "%s: unable to allocate "
> +					"memory.\n", progname);
> +				exit(1);

Aside: I've been trying to stick with EXIT_SUCCESS and EXIT_FAILURE  
when calling exit(3).

> 			}
> 			break;
> 		case 'P':	/* XXX for nfs-server compatibility */
> -- 
> 1.6.2.2
>

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

Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Jeff Layton]

On Thu, 4 Jun 2009 15:38:32 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
> 
> > ...and add a --debug option to make nfsd log messages to stderr.
> >
> > rpc.nfsd is usually run out of init scripts in which case logging to
> > syslog makes more sense. Make that the default and add a switch that
> > makes log messages go to stderr. Eventually however nfsd has to close
> > stderr, at which point the program switches to logging to syslog
> > unconditionally.
> >
> > For now, stderr gets closed rather early, so --debug isn't terribly
> > helpful. Later patches will make rpc.nfsd delay closing of stderr  
> > longer
> > which should make it more useful.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > support/nfs/nfssvc.c |   46 +++++++++++++++++-------------------
> > utils/nfsd/nfsd.c    |   63 ++++++++++++++++++++++++++++++ 
> > +-------------------
> > 2 files changed, 61 insertions(+), 48 deletions(-)
> >
> > diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> > index 33c15a7..3e6bd31 100644
> > --- a/support/nfs/nfssvc.c
> > +++ b/support/nfs/nfssvc.c
> 
> One wonders why this code is not under utils/nfsd/ .  rpc.nfsd seems  
> to be its only caller.
> 

Yep...not sure either. I could move it, I suppose.

> > @@ -16,10 +16,9 @@
> > #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"
> > @@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> > char *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));
> > +			xlog(L_ERROR, "unable to create UDP socket: "
> > +				"errno %d (%m)", 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));
> > +			xlog(L_ERROR, "unable to bind UDP socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 	}
> > @@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> > char *haddr)
> > 	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));
> > +			xlog(L_ERROR, "unable to createt tcp socket: "
> 
> You could correct the misspelling here (createt), while you're at it.
> 

I think I fix that in a later patch, but I'll fix it here too.

> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 		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));
> > +			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 		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));
> > +			xlog(L_ERROR, "unable to bind TCP socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 		if (listen(tcpfd, 64) < 0){
> > -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
> > -				"errno %d (%s)\n", errno, strerror(errno));
> > +			xlog(L_ERROR, "unable to create listening socket: "
> > +				"errno %d (%m)", errno);
> > 			exit(1);
> > 		}
> > 	}
> > 	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));
> > +			xlog(L_ERROR,
> > +			       "writing fds to kernel failed: errno %d (%m)",
> > +			       errno);
> > 		}
> > 		close(fd);
> > 		fd = -1;
> > @@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits,  
> > char *haddr)
> > 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
> > 		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
> > 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -			syslog(LOG_ERR,
> > -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
> > -			       errno, strerror(errno));
> > +			xlog(L_ERROR,
> > +			       "writing fds to kernel failed: errno %d (%m)",
> > +			       errno);
> > 		}
> > 	}
> > 	close(fd);
> > @@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int  
> > minorvers4)
> > 				    minorvers4 > 0 ? '+' : '-',
> > 				    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)",
> > -			errno, strerror(errno));
> > -	}
> > +	if (write(fd, buf, strlen(buf)) != strlen(buf))
> > +		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
> > +
> > 	close(fd);
> >
> > 	return;
> > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> > index c7bd003..6dfea67 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,6 +37,7 @@ 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;
> > @@ -50,7 +51,7 @@ main(int argc, char **argv)
> > 	int	count = 1, c, error, port, fd, found_one;
> > 	struct servent *ent;
> > 	struct hostent *hp;
> > -	char *p;
> > +	char *p, *progname;
> >
> > 	ent = getservbyname ("nfs", "udp");
> > 	if (ent != NULL)
> > @@ -58,8 +59,22 @@ main(int argc, char **argv)
> > 	else
> > 		port = 2049;
> >
> > -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,  
> > NULL)) != EOF) {
> > +	progname = strdup(basename(argv[0]));
> > +	if (!progname) {
> > +		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
> > +		exit(1);
> > +	}
> > +
> > +	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) {
> > 				haddr = strdup(optarg);
> > @@ -67,8 +82,8 @@ main(int argc, char **argv)
> > 				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]);
> > +					progname, optarg);
> > +				usage(progname);
> > 			}
> > 			break;
> > 		case 'P':	/* XXX for nfs-server compatibility */
> > @@ -76,8 +91,8 @@ main(int argc, char **argv)
> > 			port = atoi(optarg);
> > 			if (port <= 0 || port > 65535) {
> > 				fprintf(stderr, "%s: bad port number: %s\n",
> > -					argv[0], optarg);
> > -				usage(argv [0]);
> > +					progname, optarg);
> > +				usage(progname);
> > 			}
> > 			break;
> > 		case 'N':
> > @@ -105,14 +120,17 @@ main(int argc, char **argv)
> > 		default:
> > 			fprintf(stderr, "Invalid argument: '%c'\n", c);
> > 		case 'h':
> > -			usage(argv[0]);
> > +			usage(progname);
> > 		}
> > 	}
> > +
> > +	xlog_open(progname);
> > +
> > 	/*
> > 	 * Do some sanity checking, if the ctlbits are set
> > 	 */
> > 	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
> > -		fprintf(stderr, "invalid protocol specified\n");
> > +		xlog(L_ERROR, "invalid protocol specified");
> 
> Nit: This code looks like additional command line option parsing, so  
> fprintf(stderr) might be entirely adequate.  Any reason to promote  
> these to xlog() ?
> 

I think when I asked you about this before you said that you thought
that nfsd should always log to syslog whenever possible. Once the
command line parsing is done, it's safe to switch to syslog...

I'm starting to wonder though whether that's the ideal thing. Given
that rpc.nfsd isn't really a daemon per-se, maybe it would be better to
make it send output to stderr instead? The parent processes for init
scripts usually take care of logging stderr to syslog. If someone is
running interactively then it probably makes sense to give them
feedback on stderr rather than requiring them to run with --debug...

> > 		exit(1);
> > 	}
> > 	found_one = 0;
> > @@ -121,12 +139,12 @@ main(int argc, char **argv)
> > 			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,17 +153,15 @@ 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: %m", NFS_STATEDIR);
> > 		exit(1);
> > 	}
> >
> > 	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);
> > +			xlog(L_ERROR, "invalid server count (%d), using 1",
> > +				      count);
> > 			count = 1;
> > 		}
> > 	}
> > @@ -155,21 +171,20 @@ main(int argc, char **argv)
> > 	   everything before spawning kernel threads.  --Chip */
> > 	fd = open("/dev/null", O_RDWR);
> > 	if (fd == -1)
> > -		perror("/dev/null");
> > +		xlog(L_ERROR, "Unable to open /dev/null: %m");
> > 	else {
> > 		(void) dup2(fd, 0);
> > 		(void) dup2(fd, 1);
> > 		(void) dup2(fd, 2);
> > 	}
> > +	xlog_syslog(1);
> > +	xlog_stderr(0);
> > 	closeall(3);
> >
> > -	openlog("nfsd", LOG_PID, LOG_DAEMON);
> > -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > haddr)) < 0) {
> > -		int e = errno;
> > -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
> > -		closelog();
> > -	}
> > +	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,  
> > haddr)) < 0)
> > +		xlog(L_ERROR, "nfssvc (%m)");
> >
> > +	free(progname);
> > 	return (error != 0);
> > }
> >
> > @@ -177,7 +192,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.2.2
> >
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 05/10] nfs-utils: move check for active knfsd to helper function

[Jeff Layton]

On Thu, 4 Jun 2009 16:00:43 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
> 
> > nfssvc_setfds checks to see if knfsd is already running. Move this
> > check to a helper function. Eventually the nfsd code will call this
> > directly.
> 
> Some of this isn't your fault, but this code could use a little extra  
> clean up as you split it up, IMO.
> 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > support/nfs/nfssvc.c |   44 ++++++++++++++++++++++++++++++ 
> > +-------------
> > 1 files changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> > index 3e6bd31..8b15c4d 100644
> > --- a/support/nfs/nfssvc.c
> > +++ b/support/nfs/nfssvc.c
> > @@ -24,28 +24,46 @@
> > #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;
> 
> read(2) returns a ssize_t result, not an int.
> 
> > 	char buf[BUFSIZ];
> 
> Eesh.  BUFSIZ looks like it's two pages on my system, so adding this  
> helper makes the stack requirements in this path over 16KB.
> 
> Might be better if we used something a little more stack friendly  
> here.  The kernel bounds the size of the read(2) result to  
> SIMPLE_TRANSACTION_LIMIT, which is less than a page, for example.   
> Making it a static would keep "buf" off the stack, too; or checking  
> how much buffer we really need (like only a few bytes, for this  
> particular check) might be better.
> 

Hmm good catch -- I'll admit that I never bothered to look at what BUFSIZ
actually is. SIMPLE_TRANSACTION_LIMIT sounds more reasonable, but we
can probably get away with even less than that for this.

> > -	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);
> 
> Nit: Using "sizeof(buf)" might be slightly more maintainable than  
> "BUFSIZ".
> 
> > 	close(fd);
> > +
> > 	if (n != 0)
> 
> If read(2) returns -1, we return 1 here.  How about "return (n > 0);" ?
> 

Good catch -- will fix.

> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static void
> > +nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> > +{
> > +	int fd, n, on=1;
> 
> Another compiler warning issue: "n" is probably now unused in  
> nfssvc_setfds().
> 

I think that gets removed later (part of the problem with trying to
break up huge patches into smaller ones is that you miss stuff like
this). I'll see if it can be removed here.

> >
> > +	char buf[BUFSIZ];
> > +	int udpfd = -1, tcpfd = -1;
> > +	struct sockaddr_in sin;
> > +
> > +	if (nfssvc_inuse())
> > 		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.
> > -	 */
> > +
> > 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
> > 	if (fd < 0)
> > 		return;
> > -- 
> > 1.6.2.2
> >
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com

Thanks!
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 06/10] nfs-utils: convert nfssvc_setfds to use getaddrinfo

[Jeff Layton]

On Thu, 4 Jun 2009 16:17:25 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
> 
> > Convert nfssvc_setfds to use getaddrinfo. Change the args that it  
> > takes
> > and fix up nfssvc function to pass in the proper args. The things that
> > nfssvc has to do to call the new nfssvc_setfds is a little cumbersome
> > for now, but that will eventually be cleaned up in a later patch.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > support/nfs/nfssvc.c |  191 +++++++++++++++++++++++++++++++++ 
> > +----------------
> > utils/nfsd/nfsd.c    |   18 +++--
> > 2 files changed, 140 insertions(+), 69 deletions(-)
> >
> > diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> > index 8b15c4d..168414c 100644
> > --- a/support/nfs/nfssvc.c
> > +++ b/support/nfs/nfssvc.c
> > @@ -10,7 +10,9 @@
> > #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>
> > @@ -53,85 +55,148 @@ nfssvc_inuse(void)
> > 	return 0;
> > }
> >
> > -static void
> > -nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> > +static int
> > +nfssvc_setfds(const struct addrinfo *hints, const char *node, const  
> > char *port)
> > {
> > -	int fd, n, on=1;
> > +	int fd, on = 1, fac = L_ERROR;
> > +	int sockfd = -1, rc = 0;
> > 	char buf[BUFSIZ];
> 
> Same comment as earlier; BUFSIZ is probably unnecessarily large.
> 

Yup. Will give that a hard look.

> > -	int udpfd = -1, tcpfd = -1;
> > -	struct sockaddr_in sin;
> > -
> > -	if (nfssvc_inuse())
> > -		return;
> > +	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) {
> > -			xlog(L_ERROR, "unable to create UDP socket: "
> > -				"errno %d (%m)", errno);
> > -			exit(1);
> > -		}
> > -		if (bind(udpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> > -			xlog(L_ERROR, "unable to bind UDP socket: "
> > -				"errno %d (%m)", errno);
> > -			exit(1);
> > -		}
> > +		return 0;
> > +
> > +	switch(hints->ai_family) {
> > +	case AF_INET:
> > +		family = "inet";
> > +		break;
> > +	default:
> > +		xlog(L_ERROR, "Unknown address family specified: %d\n",
> > +				hints->ai_family);
> > +		rc = EAFNOSUPPORT;
> > +		goto error;
> > 	}
> >
> > -	if (NFSCTL_TCPISSET(ctlbits)) {
> > -		tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > -		if (tcpfd < 0) {
> > -			xlog(L_ERROR, "unable to createt tcp socket: "
> > -				"errno %d (%m)", errno);
> > -			exit(1);
> > +	rc = getaddrinfo(node, port, hints, &addrhead);
> > +	if (rc == EAI_NONAME && !strcmp(port, "nfs")) {
> > +		snprintf(buf, BUFSIZ, "%d", NFS_PORT);
> > +		rc = getaddrinfo(node, buf, hints, &addrhead);
> > +	}
> 
> So, I wonder about this semantic.  Should rpc.nfsd fail outright if  
> the specified port isn't usable?  If it succeeds, but silently creates  
> a listener on a port that wasn't specified, that might be a little  
> surprising to some administrators.
> 

Well, I think that's the semantic we have here. It should only retry
with NFS_PORT if a '-p' option wasn't specified (that's what
the !strcmp is all about).

> Also, passing sizeof(buf) instead of BUFSIZ might be more maintainable.
> 

Yep, I'll fix that here and in the earlier patch.

> > +
> > +	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;
> > +	}
> > +
> > +	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) {
> > -			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
> > -				"errno %d (%m)", 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 (%m)", family, proto, errno);
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -		if (bind(tcpfd, (struct  sockaddr  *)&sin, sizeof(sin)) < 0){
> > -			xlog(L_ERROR, "unable to bind TCP socket: "
> > -				"errno %d (%m)", 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 (%m)", family, errno);
> > +			rc = errno;
> > +			goto error;
> > +		}
> > +		if (bind(sockfd, addr->ai_addr, addr->ai_addrlen)) {
> > +			xlog(L_ERROR, "unable to bind %s %s socket: "
> > +				"errno %d (%m)", family, proto, errno);
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -		if (listen(tcpfd, 64) < 0){
> > +		if (addr->ai_protocol == IPPROTO_TCP && listen(sockfd, 64)) {
> > 			xlog(L_ERROR, "unable to create listening socket: "
> > 				"errno %d (%m)", errno);
> > -			exit(1);
> > +			rc = errno;
> > +			goto error;
> > 		}
> > -	}
> > -	if (udpfd >= 0) {
> > -		snprintf(buf, BUFSIZ,"%d\n", udpfd);
> > -		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -			xlog(L_ERROR,
> > -			       "writing fds to kernel failed: errno %d (%m)",
> > -			       errno);
> > -		}
> > -		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 (%m)", errno);
> > +			goto error;
> > +		}
> > +		snprintf(buf, BUFSIZ, "%d\n", sockfd);
> > 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
> > -			xlog(L_ERROR,
> > -			       "writing fds to kernel failed: errno %d (%m)",
> > -			       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 (%m)",
> > +				  errno);
> > +			rc = errno;
> > +			goto error;
> > 		}
> > +		close(fd);
> > +		close(sockfd);
> > +		sockfd = fd = -1;
> > +		addr = addr->ai_next;
> > 	}
> > -	close(fd);
> > +error:
> > +	if (fd >= 0)
> > +		close(fd);
> > +	if (sockfd >= 0)
> > +		close(sockfd);
> > +	if (addrhead)
> > +		freeaddrinfo(addrhead);
> > +	return rc;
> > +}
> >
> > -	return;
> > +static int
> > +nfssvc_set_sockets(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 EPROTOTYPE;
> > +	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);
> > }
> > +
> > static void
> > nfssvc_versbits(unsigned int ctlbits, int minorvers4)
> > {
> > @@ -169,13 +234,17 @@ nfssvc(int port, int nrservs, unsigned int  
> > versbits, int minorvers4,
> > {
> > 	struct nfsctl_arg	arg;
> > 	int fd;
> > +	char portstr[BUFSIZ];
> 
> Ditto.
> 
> > 	/* 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);
> > +	if (!nfssvc_inuse()) {
> > +		nfssvc_versbits(versbits, minorvers4);
> > +		snprintf(portstr, BUFSIZ, "%d", port);
> > +		nfssvc_set_sockets(AF_INET, protobits, haddr, portstr);
> > +	}
> >
> > 	fd = open(NFSD_THREAD_FILE, O_WRONLY);
> > 	if (fd < 0)
> > diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> > index 6dfea67..e9d0bf9 100644
> > --- a/utils/nfsd/nfsd.c
> > +++ b/utils/nfsd/nfsd.c
> > @@ -76,14 +76,16 @@ main(int argc, char **argv)
> > 			xlog_stderr(1);
> > 			break;
> > 		case 'H':
> > -			if (inet_addr(optarg) != INADDR_NONE) {
> > -				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",
> > -					progname, optarg);
> > -				usage(progname);
> > +			/*
> > +			 * for now, this only handles one -H option. Use the
> > +			 * last one specified.
> > +			 */
> > +			free(haddr);
> > +			haddr = strdup(optarg);
> > +			if (!haddr) {
> > +				fprintf(stderr, "%s: unable to allocate "
> > +					"memory.\n", progname);
> > +				exit(1);
> 
> Aside: I've been trying to stick with EXIT_SUCCESS and EXIT_FAILURE  
> when calling exit(3).
> 

Ok. I'll have a look at that too. There's value in being consistent
here I think.

> > 			}
> > 			break;
> > 		case 'P':	/* XXX for nfs-server compatibility */
> > -- 
> > 1.6.2.2
> >
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Chuck Lever]

On Jun 4, 2009, at 4:25 PM, Jeff Layton wrote:

> On Thu, 4 Jun 2009 15:38:32 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
>>
>> On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:
>>
>>> ...and add a --debug option to make nfsd log messages to stderr.
>>>
>>> rpc.nfsd is usually run out of init scripts in which case logging to
>>> syslog makes more sense. Make that the default and add a switch that
>>> makes log messages go to stderr. Eventually however nfsd has to  
>>> close
>>> stderr, at which point the program switches to logging to syslog
>>> unconditionally.
>>>
>>> For now, stderr gets closed rather early, so --debug isn't terribly
>>> helpful. Later patches will make rpc.nfsd delay closing of stderr
>>> longer
>>> which should make it more useful.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> support/nfs/nfssvc.c |   46 +++++++++++++++++-------------------
>>> utils/nfsd/nfsd.c    |   63 ++++++++++++++++++++++++++++++
>>> +-------------------
>>> 2 files changed, 61 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
>>> index 33c15a7..3e6bd31 100644
>>> --- a/support/nfs/nfssvc.c
>>> +++ b/support/nfs/nfssvc.c
>>
>> One wonders why this code is not under utils/nfsd/ .  rpc.nfsd seems
>> to be its only caller.
>>
>
> Yep...not sure either. I could move it, I suppose.
>
>>> @@ -16,10 +16,9 @@
>>> #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"
>>> @@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits,
>>> char *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));
>>> +			xlog(L_ERROR, "unable to create UDP socket: "
>>> +				"errno %d (%m)", 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));
>>> +			xlog(L_ERROR, "unable to bind UDP socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 	}
>>> @@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits,
>>> char *haddr)
>>> 	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));
>>> +			xlog(L_ERROR, "unable to createt tcp socket: "
>>
>> You could correct the misspelling here (createt), while you're at it.
>>
>
> I think I fix that in a later patch, but I'll fix it here too.
>
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 		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));
>>> +			xlog(L_ERROR, "unable to set SO_REUSEADDR: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 		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));
>>> +			xlog(L_ERROR, "unable to bind TCP socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 		if (listen(tcpfd, 64) < 0){
>>> -			syslog(LOG_ERR, "nfssvc: unable to create listening socket: "
>>> -				"errno %d (%s)\n", errno, strerror(errno));
>>> +			xlog(L_ERROR, "unable to create listening socket: "
>>> +				"errno %d (%m)", errno);
>>> 			exit(1);
>>> 		}
>>> 	}
>>> 	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));
>>> +			xlog(L_ERROR,
>>> +			       "writing fds to kernel failed: errno %d (%m)",
>>> +			       errno);
>>> 		}
>>> 		close(fd);
>>> 		fd = -1;
>>> @@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits,
>>> char *haddr)
>>> 			fd = open(NFSD_PORTS_FILE, O_WRONLY);
>>> 		snprintf(buf, BUFSIZ,"%d\n", tcpfd);
>>> 		if (write(fd, buf, strlen(buf)) != strlen(buf)) {
>>> -			syslog(LOG_ERR,
>>> -			       "nfssvc: writing fds to kernel failed: errno %d (%s)",
>>> -			       errno, strerror(errno));
>>> +			xlog(L_ERROR,
>>> +			       "writing fds to kernel failed: errno %d (%m)",
>>> +			       errno);
>>> 		}
>>> 	}
>>> 	close(fd);
>>> @@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int
>>> minorvers4)
>>> 				    minorvers4 > 0 ? '+' : '-',
>>> 				    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)",
>>> -			errno, strerror(errno));
>>> -	}
>>> +	if (write(fd, buf, strlen(buf)) != strlen(buf))
>>> +		xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno);
>>> +
>>> 	close(fd);
>>>
>>> 	return;
>>> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
>>> index c7bd003..6dfea67 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,6 +37,7 @@ 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;
>>> @@ -50,7 +51,7 @@ main(int argc, char **argv)
>>> 	int	count = 1, c, error, port, fd, found_one;
>>> 	struct servent *ent;
>>> 	struct hostent *hp;
>>> -	char *p;
>>> +	char *p, *progname;
>>>
>>> 	ent = getservbyname ("nfs", "udp");
>>> 	if (ent != NULL)
>>> @@ -58,8 +59,22 @@ main(int argc, char **argv)
>>> 	else
>>> 		port = 2049;
>>>
>>> -	while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts,
>>> NULL)) != EOF) {
>>> +	progname = strdup(basename(argv[0]));
>>> +	if (!progname) {
>>> +		fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]);
>>> +		exit(1);
>>> +	}
>>> +
>>> +	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) {
>>> 				haddr = strdup(optarg);
>>> @@ -67,8 +82,8 @@ main(int argc, char **argv)
>>> 				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]);
>>> +					progname, optarg);
>>> +				usage(progname);
>>> 			}
>>> 			break;
>>> 		case 'P':	/* XXX for nfs-server compatibility */
>>> @@ -76,8 +91,8 @@ main(int argc, char **argv)
>>> 			port = atoi(optarg);
>>> 			if (port <= 0 || port > 65535) {
>>> 				fprintf(stderr, "%s: bad port number: %s\n",
>>> -					argv[0], optarg);
>>> -				usage(argv [0]);
>>> +					progname, optarg);
>>> +				usage(progname);
>>> 			}
>>> 			break;
>>> 		case 'N':
>>> @@ -105,14 +120,17 @@ main(int argc, char **argv)
>>> 		default:
>>> 			fprintf(stderr, "Invalid argument: '%c'\n", c);
>>> 		case 'h':
>>> -			usage(argv[0]);
>>> +			usage(progname);
>>> 		}
>>> 	}
>>> +
>>> +	xlog_open(progname);
>>> +
>>> 	/*
>>> 	 * Do some sanity checking, if the ctlbits are set
>>> 	 */
>>> 	if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) {
>>> -		fprintf(stderr, "invalid protocol specified\n");
>>> +		xlog(L_ERROR, "invalid protocol specified");
>>
>> Nit: This code looks like additional command line option parsing, so
>> fprintf(stderr) might be entirely adequate.  Any reason to promote
>> these to xlog() ?
>
> I think when I asked you about this before you said that you thought
> that nfsd should always log to syslog whenever possible.

Remember I said that after I mentioned I hadn't looked at this code  
closely ;-)

> Once the
> command line parsing is done, it's safe to switch to syslog...

Yes, the question is where that point is.

> I'm starting to wonder though whether that's the ideal thing. Given
> that rpc.nfsd isn't really a daemon per-se, maybe it would be better  
> to
> make it send output to stderr instead? The parent processes for init
> scripts usually take care of logging stderr to syslog. If someone is
> running interactively then it probably makes sense to give them
> feedback on stderr rather than requiring them to run with --debug...

This is one of those cases where different init script behavior across  
distributions is a challenge.  It might be better to provide command- 
line options to direct the output, rather than a simple "debug"  
option.  Just a thought.

>>> 		exit(1);
>>> 	}
>>> 	found_one = 0;
>>> @@ -121,12 +139,12 @@ main(int argc, char **argv)
>>> 			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,17 +153,15 @@ 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: %m", NFS_STATEDIR);
>>> 		exit(1);
>>> 	}
>>>
>>> 	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);
>>> +			xlog(L_ERROR, "invalid server count (%d), using 1",
>>> +				      count);
>>> 			count = 1;
>>> 		}
>>> 	}
>>> @@ -155,21 +171,20 @@ main(int argc, char **argv)
>>> 	   everything before spawning kernel threads.  --Chip */
>>> 	fd = open("/dev/null", O_RDWR);
>>> 	if (fd == -1)
>>> -		perror("/dev/null");
>>> +		xlog(L_ERROR, "Unable to open /dev/null: %m");
>>> 	else {
>>> 		(void) dup2(fd, 0);
>>> 		(void) dup2(fd, 1);
>>> 		(void) dup2(fd, 2);
>>> 	}
>>> +	xlog_syslog(1);
>>> +	xlog_stderr(0);
>>> 	closeall(3);
>>>
>>> -	openlog("nfsd", LOG_PID, LOG_DAEMON);
>>> -	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,
>>> haddr)) < 0) {
>>> -		int e = errno;
>>> -		syslog(LOG_ERR, "nfssvc: %s", strerror(e));
>>> -		closelog();
>>> -	}
>>> +	if ((error = nfssvc(port, count, versbits, minorvers4, protobits,
>>> haddr)) < 0)
>>> +		xlog(L_ERROR, "nfssvc (%m)");
>>>
>>> +	free(progname);
>>> 	return (error != 0);
>>> }
>>>
>>> @@ -177,7 +192,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.2.2
>>>
>>
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>>
>>
>>
>
>
> -- 
> Jeff Layton <jlayton@redhat.com>

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

Re: [PATCH 08/10] nfs-utils: add IPv6 support to nfssvc_setfds

[Chuck Lever]

On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:

> Allow nfssvc_setfds to properly deal with AF_INET6.
>
> IPv6 sockets for knfsd can't be allowed to accept IPv4 packets. Set  
> the
> correct option to prevent that from occurring on IPv6 sockets.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/nfs/nfssvc.c |   14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 89fb5be..19f1a4c 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -76,6 +76,11 @@ nfssvc_setfds(const struct addrinfo *hints, const  
> char *node, const char *port)
> 	case AF_INET:
> 		family = "inet";
> 		break;
> +#ifdef IPV6_SUPPORTED
> +	case AF_INET6:
> +		family = "inet6";
> +		break;
> +#endif /* IPV6_SUPPORTED */
> 	default:
> 		xlog(L_ERROR, "Unknown address family specified: %d\n",
> 				hints->ai_family);
> @@ -123,6 +128,15 @@ nfssvc_setfds(const struct addrinfo *hints,  
> const char *node, const char *port)
> 			rc = errno;
> 			goto error;
> 		}
> +#ifdef IPV6_SUPPORTED
> +		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;

Maybe "rc = errno;" ?

> +			goto error;
> +		}
> +#endif /* IPV6_SUPPORTED */
> 		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.2.2
>

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