[PATCH 00/15] Fixes for nfs-utils-1.2.4

[PATCH 00/15] Fixes for nfs-utils-1.2.4

[Chuck Lever]

Steve-

Here are fifteen patches which address several mount and mountd bugs
and correct other minor problems in nfs-utils-1.2.3.  Now that the
bake-a-thon event has passed, folks should have some spare moments to
review these.

This series includes the final version of the patch that addresses the
"mountd -p <port> does not work" issue.

The series does not include a patch to address the "remount erases a
mount point's version setting" issue.  The remount problem is not a
1.2.3 regression, it's been around for a very long time.  I'm still
exploring solutions.

These patches are also available in my nfs-utils git repo on
git.linux-nfs.org.

---

Chuck Lever (14):
      mount.nfs: don't show "remount" flag in /etc/mtab
      umount.nfs: Distinguish between nfs4 and nfs mounts
      mount.nfs: mountproto does not support RDMA
      mount.nfs: Eliminate compiler warnings in utils/mount/network.c
      mount.nfs: Eliminate compiler warning in utils/mount/parse_opt.c
      mount.nfs: Eliminate compiler warning in utils/mount/nfsumount.c
      mount.nfs: Eliminate compiler warning in utils/mount/nfsumount.c
      mount.nfs: Eliminate compiler warnings in utils/mount/mount.c
      mount.nfs: Eliminate compiler warning in utils/mount/mount.c
      mount.nfs: Eliminate compiler warnings in utils/mount/version.h
      mount.nfs: Eliminate compiler warning in utils/mount/mount.c
      mount.nfs: Eliminate compiler warnings
      libnfs.a: Allow multiple RPC listeners to share listener port number
      mountd: Clear mountd registrations at start up

Trond Myklebust (1):
      export: Ensure that we free struct exportent->e_uuid


 support/export/export.c       |    1 
 support/nfs/exports.c         |    2 
 support/nfs/svc_create.c      |  252 ++++++++++++++++++++++++++++++++++++++++-
 utils/mount/fstab.c           |   29 +++--
 utils/mount/mount.c           |   10 +-
 utils/mount/mount_config.h    |   21 ++-
 utils/mount/mount_constants.h |    4 +
 utils/mount/network.c         |   29 +++--
 utils/mount/nfsumount.c       |  120 ++++++++++++++++++--
 utils/mount/parse_opt.c       |    2 
 utils/mount/version.h         |    6 -
 utils/mountd/mountd.c         |    1 
 12 files changed, 417 insertions(+), 60 deletions(-)

-- 
Chuck Lever

[PATCH 01/15] mountd: Clear mountd registrations at start up

[Chuck Lever]

Clear stale MNT registrations before mountd tries to create fresh
listeners, to ensure that mountd starts.  This is also what statd
does.

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

 utils/mountd/mountd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index d309950..7e0cf6a 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -840,6 +840,7 @@ main(int argc, char **argv)
 	if (new_cache)
 		cache_open();
 
+	unregister_services();
 	if (version2()) {
 		listeners += nfs_svc_create("mountd", MOUNTPROG,
 					MOUNTVERS, mount_dispatch, port);

[PATCH 02/15] libnfs.a: Allow multiple RPC listeners to share listener port number

[Chuck Lever]

Normally, when "-p" is not specified on the mountd command line, the
TI-RPC library chooses random port numbers for each listener.  If a
port number _is_ specified on the command line, all the listeners
will get the same port number, so SO_REUSEADDR needs to be set on
each socket.

Thus we can't let TI-RPC create the listener sockets for us in this
case; we must create them ourselves and then set SO_REUSEADDR (and
other socket options) by hand.

Different versions of the same RPC program have to share the same
listener and SVCXPRT, so we have to cache xprts we create, and re-use
them when additional requests for registration come from the
application.

Though it doesn't look like it, this fix was "copied" from the legacy
rpc_init() function.  It's more complicated for TI-RPC, of course,
since a TI-RPC application can set up listeners with a nearly
arbitrary number of address families and socket types, not just the
two listeners that legacy RPC applications can set up (one for AF_INET
UDP and one for AF_INET TCP).

See:
  https://bugzilla.linux-nfs.org/show_bug.cgi?id=190

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

 support/nfs/svc_create.c |  252 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 246 insertions(+), 6 deletions(-)

diff --git a/support/nfs/svc_create.c b/support/nfs/svc_create.c
index 59ba505..b3f75ed 100644
--- a/support/nfs/svc_create.c
+++ b/support/nfs/svc_create.c
@@ -27,6 +27,7 @@
 #include <memory.h>
 #include <signal.h>
 #include <unistd.h>
+#include <errno.h>
 #include <netdb.h>
 
 #include <netinet/in.h>
@@ -41,11 +42,68 @@
 #include "tcpwrapper.h"
 #endif
 
+#include "sockaddr.h"
 #include "rpcmisc.h"
 #include "xlog.h"
 
 #ifdef HAVE_LIBTIRPC
 
+#define SVC_CREATE_XPRT_CACHE_SIZE	(8)
+static SVCXPRT *svc_create_xprt_cache[SVC_CREATE_XPRT_CACHE_SIZE] = { NULL, };
+
+/*
+ * Cache an SVC xprt, in case there are more programs or versions to
+ * register against it.
+ */
+static void
+svc_create_cache_xprt(SVCXPRT *xprt)
+{
+	unsigned int i;
+
+	/* Check if we've already got this one... */
+	for (i = 0; i < SVC_CREATE_XPRT_CACHE_SIZE; i++)
+		if (svc_create_xprt_cache[i] == xprt)
+			return;
+
+	/* No, we don't.  Cache it. */
+	for (i = 0; i < SVC_CREATE_XPRT_CACHE_SIZE; i++)
+		if (svc_create_xprt_cache[i] == NULL) {
+			svc_create_xprt_cache[i] = xprt;
+			return;
+		}
+
+	xlog(L_ERROR, "%s: Failed to cache an xprt", __func__);
+}
+
+/*
+ * Find a previously cached SVC xprt structure with the given bind address
+ * and transport semantics.
+ *
+ * Returns pointer to a cached SVC xprt.
+ *
+ * If no matching SVC XPRT can be found, NULL is returned.
+ */
+static SVCXPRT *
+svc_create_find_xprt(const struct sockaddr *bindaddr, const struct netconfig *nconf)
+{
+	unsigned int i;
+
+	for (i = 0; i < SVC_CREATE_XPRT_CACHE_SIZE; i++) {
+		SVCXPRT *xprt = svc_create_xprt_cache[i];
+		struct sockaddr *sap;
+
+		if (xprt == NULL)
+			continue;
+		if (strcmp(nconf->nc_netid, xprt->xp_netid) != 0)
+			continue;
+		sap = (struct sockaddr *)xprt->xp_ltaddr.buf;
+		if (!nfs_compare_sockaddr(bindaddr, sap))
+			continue;
+		return xprt;
+	}
+	return NULL;
+}
+
 /*
  * Set up an appropriate bind address, given @port and @nconf.
  *
@@ -98,17 +156,113 @@ svc_create_bindaddr(struct netconfig *nconf, const uint16_t port)
 	return ai;
 }
 
+/*
+ * Create a listener socket on a specific bindaddr, and set
+ * special socket options to allow it to share the same port
+ * as other listeners.
+ *
+ * Returns an open, bound, and possibly listening network
+ * socket on success.
+ *
+ * Otherwise returns -1 if some error occurs.
+ */
+static int
+svc_create_sock(const struct sockaddr *sap, socklen_t salen,
+		struct netconfig *nconf)
+{
+	int fd, type, protocol;
+	int one = 1;
+
+	switch(nconf->nc_semantics) {
+	case NC_TPI_CLTS:
+		type = SOCK_DGRAM;
+		break;
+	case NC_TPI_COTS_ORD:
+		type = SOCK_STREAM;
+		break;
+	default:
+		xlog(D_GENERAL, "%s: Unrecognized bind address semantics: %u",
+			__func__, nconf->nc_semantics);
+		return -1;
+	}
+
+	if (strcmp(nconf->nc_proto, NC_UDP) == 0)
+		protocol = (int)IPPROTO_UDP;
+	else if (strcmp(nconf->nc_proto, NC_TCP) == 0)
+		protocol = (int)IPPROTO_TCP;
+	else {
+		xlog(D_GENERAL, "%s: Unrecognized bind address protocol: %s",
+			__func__, nconf->nc_proto);
+		return -1;
+	}
+
+	fd = socket((int)sap->sa_family, type, protocol);
+	if (fd == -1) {
+		xlog(L_ERROR, "Could not make a socket: (%d) %m",
+			errno);
+		return -1;
+	}
+
+#ifdef IPV6_SUPPORTED
+	if (sap->sa_family == AF_INET6) {
+		if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY,
+				&one, sizeof(one)) == -1) {
+			xlog(L_ERROR, "Failed to set IPV6_V6ONLY: (%d) %m",
+				errno);
+			(void)close(fd);
+			return -1;
+		}
+	}
+#endif	/* IPV6_SUPPORTED */
+
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
+		       &one, sizeof(one)) == -1) {
+		xlog(L_ERROR, "Failed to set SO_REUSEADDR: (%d) %m",
+			errno);
+		(void)close(fd);
+		return -1;
+	}
+
+	if (bind(fd, sap, salen) == -1) {
+		xlog(L_ERROR, "Could not bind socket: (%d) %m",
+			errno);
+		(void)close(fd);
+		return -1;
+	}
+
+	if (nconf->nc_semantics == NC_TPI_COTS_ORD)
+		if (listen(fd, SOMAXCONN) == -1) {
+			xlog(L_ERROR, "Could not listen on socket: (%d) %m",
+				errno);
+			(void)close(fd);
+			return -1;
+		}
+
+	return fd;
+}
+
+/*
+ * The simple case is allowing the TI-RPC library to create a
+ * transport itself, given just the bind address and transport
+ * semantics.
+ *
+ * Our local xprt cache is ignored in this path, since the
+ * caller is not interested in sharing listeners or ports, and
+ * the library automatically avoids ports already in use.
+ *
+ * Returns the count of started listeners (one or zero).
+ */
 static unsigned int
-svc_create_nconf(const char *name, const rpcprog_t program,
+svc_create_nconf_rand_port(const char *name, const rpcprog_t program,
 		const rpcvers_t version,
 		void (*dispatch)(struct svc_req *, SVCXPRT *),
-		const uint16_t port, struct netconfig *nconf)
+		struct netconfig *nconf)
 {
 	struct t_bind bindaddr;
 	struct addrinfo *ai;
 	SVCXPRT	*xprt;
 
-	ai = svc_create_bindaddr(nconf, port);
+	ai = svc_create_bindaddr(nconf, 0);
 	if (ai == NULL)
 		return 0;
 
@@ -119,7 +273,7 @@ svc_create_nconf(const char *name, const rpcprog_t program,
 	freeaddrinfo(ai);
 	if (xprt == NULL) {
 		xlog(D_GENERAL, "Failed to create listener xprt "
-				"(%s, %u, %s)", name, version, nconf->nc_netid);
+			"(%s, %u, %s)", name, version, nconf->nc_netid);
 		return 0;
 	}
 
@@ -133,6 +287,93 @@ svc_create_nconf(const char *name, const rpcprog_t program,
 	return 1;
 }
 
+/*
+ * If a port is specified on the command line, that port value will be
+ * the same for all listeners created here.  Create each listener
+ * socket in advance and set SO_REUSEADDR, rather than allowing the
+ * RPC library to create the listeners for us on a randomly chosen
+ * port via svc_tli_create(RPC_ANYFD).
+ *
+ * Some callers want to listen for more than one RPC version using the
+ * same port number.  For example, mountd could want to listen for MNT
+ * version 1, 2, and 3 requests.  This means mountd must use the same
+ * set of listener sockets for multiple RPC versions, since, on one
+ * system, you can't have two listener sockets with the exact same
+ * bind address (and port) and transport protocol.
+ *
+ * To accomplish this, this function caches xprts as they are created.
+ * This cache is checked to see if a previously created xprt can be
+ * used, before creating a new xprt for this [program, version].  If
+ * there is a cached xprt with the same bindaddr and transport
+ * semantics, we simply register the new version with that xprt,
+ * rather than creating a fresh xprt for it.
+ *
+ * The xprt cache implemented here is local to a process.  Two
+ * separate RPC daemons can not share a set of listeners.
+ *
+ * Returns the count of started listeners (one or zero).
+ */
+static unsigned int
+svc_create_nconf_fixed_port(const char *name, const rpcprog_t program,
+		const rpcvers_t version,
+		void (*dispatch)(struct svc_req *, SVCXPRT *),
+		const uint16_t port, struct netconfig *nconf)
+{
+	struct addrinfo *ai;
+	SVCXPRT	*xprt;
+
+	ai = svc_create_bindaddr(nconf, port);
+	if (ai == NULL)
+		return 0;
+
+	xprt = svc_create_find_xprt(ai->ai_addr, nconf);
+	if (xprt == NULL) {
+		int fd;
+
+		fd = svc_create_sock(ai->ai_addr, ai->ai_addrlen, nconf);
+		if (fd == -1)
+			goto out_free;
+
+		xprt = svc_tli_create(fd, nconf, NULL, 0, 0);
+		if (xprt == NULL) {
+			xlog(D_GENERAL, "Failed to create listener xprt "
+				"(%s, %u, %s)", name, version, nconf->nc_netid);
+			(void)close(fd);
+			goto out_free;
+		}
+	}
+
+	if (!svc_reg(xprt, program, version, dispatch, nconf)) {
+		/* svc_reg(3) destroys @xprt in this case */
+		xlog(D_GENERAL, "Failed to register (%s, %u, %s)",
+				name, version, nconf->nc_netid);
+		goto out_free;
+	}
+
+	svc_create_cache_xprt(xprt);
+
+	freeaddrinfo(ai);
+	return 1;
+
+out_free:
+	freeaddrinfo(ai);
+	return 0;
+}
+
+static unsigned int
+svc_create_nconf(const char *name, const rpcprog_t program,
+		const rpcvers_t version,
+		void (*dispatch)(struct svc_req *, SVCXPRT *),
+		const uint16_t port, struct netconfig *nconf)
+{
+	if (port != 0)
+		return svc_create_nconf_fixed_port(name, program,
+			version, dispatch, port, nconf);
+
+	return svc_create_nconf_rand_port(name, program,
+			version, dispatch, nconf);
+}
+
 /**
  * nfs_svc_create - start up RPC svc listeners
  * @name: C string containing name of new service
@@ -145,8 +386,7 @@ svc_create_nconf(const char *name, const rpcprog_t program,
  * the RPC dispatcher.  Returns the number of started network transports.
  */
 unsigned int
-nfs_svc_create(__attribute__((unused)) char *name,
-		const rpcprog_t program, const rpcvers_t version,
+nfs_svc_create(char *name, const rpcprog_t program, const rpcvers_t version,
 		void (*dispatch)(struct svc_req *, SVCXPRT *),
 		const uint16_t port)
 {

[PATCH 03/15] export: Ensure that we free struct exportent->e_uuid

[Chuck Lever]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Currently, the exportent->e_uuid is initialised in
support/nfs/exports.c:parseopts(), but it is never freed.

Also ensure that exportent->e_uuid is duplicated correctly in
dupexportent().

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

Adjusted to account for the new export_free() helper.

Also, e_uuid points to memory that is always allocated with strdup(3),
not with xstrdup().  Thus it must be freed via free(3) and not via
xfree().

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

 support/export/export.c |    1 +
 support/nfs/exports.c   |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/support/export/export.c b/support/export/export.c
index f528603..4fda30a 100644
--- a/support/export/export.c
+++ b/support/export/export.c
@@ -38,6 +38,7 @@ export_free(nfs_export *exp)
 	xfree(exp->m_export.e_sqgids);
 	free(exp->m_export.e_mountpoint);
 	free(exp->m_export.e_fslocdata);
+	free(exp->m_export.e_uuid);
 
 	xfree(exp->m_export.e_hostname);
 	xfree(exp);
diff --git a/support/nfs/exports.c b/support/nfs/exports.c
index a93941c..1744ed6 100644
--- a/support/nfs/exports.c
+++ b/support/nfs/exports.c
@@ -332,6 +332,8 @@ dupexportent(struct exportent *dst, struct exportent *src)
 		dst->e_mountpoint = strdup(src->e_mountpoint);
 	if (src->e_fslocdata)
 		dst->e_fslocdata = strdup(src->e_fslocdata);
+	if (src->e_uuid)
+		dst->e_uuid = strdup(src->e_uuid);
 	dst->e_hostname = NULL;
 }
 

[PATCH 04/15] mount.nfs: Eliminate compiler warnings

[Chuck Lever]

Clean up.

fstab.c: In function ‘lock_mtab’:
fstab.c:385: warning: declaration of ‘errsv’ shadows a previous local
fstab.c:367: warning: shadowed declaration is here
fstab.c:407: warning: declaration of ‘errsv’ shadows a previous local
fstab.c:367: warning: shadowed declaration is here
fstab.c:417: warning: declaration of ‘tries’ shadows a previous local
fstab.c:325: warning: shadowed declaration is here
fstab.c:422: warning: declaration of ‘errsv’ shadows a previous local
fstab.c:367: warning: shadowed declaration is here

These are probably harmless.  Reusing a variable name, however, is a
little confusing to follow when reading the code.

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

 utils/mount/fstab.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
index 051fa38..a742e64 100644
--- a/utils/mount/fstab.c
+++ b/utils/mount/fstab.c
@@ -364,19 +364,22 @@ lock_mtab (void) {
 	/* Repeat until it was us who made the link */
 	while (!we_created_lockfile) {
 		struct flock flock;
-		int errsv, j;
+		int j;
 
 		j = link(linktargetfile, MOUNTED_LOCK);
-		errsv = errno;
 
-		if (j == 0)
-			we_created_lockfile = 1;
+		{
+			int errsv = errno;
 
-		if (j < 0 && errsv != EEXIST) {
-			(void) unlink(linktargetfile);
-			die (EX_FILEIO, _("can't link lock file %s: %s "
-			     "(use -n flag to override)"),
-			     MOUNTED_LOCK, strerror (errsv));
+			if (j == 0)
+				we_created_lockfile = 1;
+
+			if (j < 0 && errsv != EEXIST) {
+				(void) unlink(linktargetfile);
+				die (EX_FILEIO, _("can't link lock file %s: %s "
+				     "(use -n flag to override)"),
+				     MOUNTED_LOCK, strerror (errsv));
+			}
 		}
 
 		lockfile_fd = open (MOUNTED_LOCK, O_WRONLY);
@@ -414,7 +417,7 @@ lock_mtab (void) {
 			}
 			(void) unlink(linktargetfile);
 		} else {
-			static int tries = 0;
+			static int retries = 0;
 
 			/* Someone else made the link. Wait. */
 			alarm(LOCK_TIMEOUT);
@@ -428,10 +431,10 @@ lock_mtab (void) {
 			alarm(0);
 			/* Limit the number of iterations - maybe there
 			   still is some old /etc/mtab~ */
-			++tries;
-			if (tries % 200 == 0)
+			++retries;
+			if (retries % 200 == 0)
 			   usleep(30);
-			if (tries > 100000) {
+			if (retries > 100000) {
 				(void) unlink(linktargetfile);
 				close(lockfile_fd);
 				die (EX_FILEIO, _("Cannot create link %s\n"

[PATCH 05/15] mount.nfs: Eliminate compiler warning in utils/mount/mount.c

[Chuck Lever]

Clean up.

In file included from mount.c:41:
mount_config.h:35: warning: no previous prototype for ‘mount_config_opts’

Functions defined in include files are usually declared as "static
inline," eliminating the need for a forward declaration.

While I was there, I also fixed the macro that prevents including
mount_config.h multiple times, and fixed some white space damage.

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

 utils/mount/mount_config.h |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/utils/mount/mount_config.h b/utils/mount/mount_config.h
index 3023306..e86b4ba 100644
--- a/utils/mount/mount_config.h
+++ b/utils/mount/mount_config.h
@@ -1,7 +1,7 @@
-#ifndef _LINUX_MOUNT__CONFIG_H
-#define _LINUX_MOUNT_CONFIG__H
+#ifndef _LINUX_MOUNT_CONFIG_H
+#define _LINUX_MOUNT_CONFIG_H
 /*
- * mount_config.h -- mount configuration file routines 
+ * mount_config.h -- mount configuration file routines
  * Copyright (C) 2008 Red Hat, Inc <nfs@redhat.com>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -16,15 +16,13 @@
  *
  */
 
-inline void mount_config_init(char *);
-
 #ifdef MOUNT_CONFIG
 #include "conffile.h"
 #include "xlog.h"
 
 extern char *conf_get_mntopts(char *, char *, char *);
 
-inline void mount_config_init(char *program)
+static inline void mount_config_init(char *program)
 {
 	xlog_open(program);
 	/*
@@ -32,19 +30,22 @@ inline void mount_config_init(char *program)
 	 */
 	conf_init();
 }
-inline char *mount_config_opts(char *spec, 
+
+static inline char *mount_config_opts(char *spec,
 		char *mount_point, char *mount_opts)
 {
 	return conf_get_mntopts(spec, mount_point, mount_opts);
 }
+
 #else /* MOUNT_CONFIG */
 
-inline void mount_config_init(char *program) { }
+static inline void mount_config_init(char *program) { }
 
-inline char *mount_config_opts(char *spec, 
+static inline char *mount_config_opts(char *spec,
 		char *mount_point, char *mount_opts)
 {
 	return mount_opts;
 }
 #endif /* MOUNT_CONFIG */
-#endif
+
+#endif	/* _LINUX_MOUNT_CONFIG_H */

[PATCH 06/15] mount.nfs: Eliminate compiler warnings in utils/mount/version.h

[Chuck Lever]

Clean up.

In file included from mount.c:50:
version.h: In function ‘linux_version_code’:
version.h:48: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result
version.h:48: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result
version.h:48: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result

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

 utils/mount/version.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/utils/mount/version.h b/utils/mount/version.h
index 46552a1..af61a6f 100644
--- a/utils/mount/version.h
+++ b/utils/mount/version.h
@@ -42,9 +42,9 @@ static inline unsigned int linux_version_code(void)
 	if (uname(&my_utsname))
 		return 0;
 
-	p = atoi(strtok(my_utsname.release, "."));
-	q = atoi(strtok(NULL, "."));
-	r = atoi(strtok(NULL, "."));
+	p = (unsigned int)atoi(strtok(my_utsname.release, "."));
+	q = (unsigned int)atoi(strtok(NULL, "."));
+	r = (unsigned int)atoi(strtok(NULL, "."));
 	return MAKE_VERSION(p, q, r);
 }
 

[PATCH 07/15] mount.nfs: Eliminate compiler warning in utils/mount/mount.c

[Chuck Lever]

Clean up.

mount.c: At top level:
mount.c:324: warning: no previous prototype for ‘mount_usage’

mount_usage() has no callers outside of utils/mount/mount.c and no
prototype is provided in a header file.  Make it static.

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

 utils/mount/mount.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 82b9169..1c6a88f 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -321,7 +321,7 @@ static int add_mtab(char *spec, char *mount_point, char *fstype,
 	return result;
 }
 
-void mount_usage(void)
+static void mount_usage(void)
 {
 	printf(_("usage: %s remotetarget dir [-rvVwfnsih] [-o nfsoptions]\n"),
 		progname);

[PATCH 08/15] mount.nfs: Eliminate compiler warnings in utils/mount/mount.c

[Chuck Lever]

Clean up.

mount.c: In function ‘parse_opt’:
mount.c:354: warning: conversion to ‘size_t’ from ‘int’ may change the sign of the result
mount.c:354: warning: conversion to ‘int’ from ‘size_t’ may change the sign of the result
mount.c:359: warning: conversion to ‘size_t’ from ‘int’ may change the sign of the result
mount.c:359: warning: conversion to ‘int’ from ‘size_t’ may change the sign of the result
mount.c: In function ‘parse_opts’:
mount.c:374: warning: conversion to ‘int’ from ‘size_t’ may change the sign of the result
mount.c:377: warning: conversion to ‘size_t’ from ‘int’ may change the sign of the result

Character string lengths are usually size_t anyway.  We can easily
avoid the implicit type cast here.

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

 utils/mount/mount.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 1c6a88f..2909595 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -337,7 +337,7 @@ static void mount_usage(void)
 	printf(_("\tnfsoptions\tRefer to mount.nfs(8) or nfs(5)\n\n"));
 }
 
-static void parse_opt(const char *opt, int *mask, char *extra_opts, int len)
+static void parse_opt(const char *opt, int *mask, char *extra_opts, size_t len)
 {
 	const struct opt_map *om;
 
@@ -371,7 +371,7 @@ static void parse_opts(const char *options, int *flags, char **extra_opts)
 	if (options != NULL) {
 		char *opts = xstrdup(options);
 		char *opt, *p;
-		int len = strlen(opts) + 1;	/* include room for a null */
+		size_t len = strlen(opts) + 1;	/* include room for a null */
 		int open_quote = 0;
 
 		*extra_opts = xmalloc(len);

[PATCH 09/15] mount.nfs: Eliminate compiler warning in utils/mount/nfsumount.c

[Chuck Lever]

Clean up.

nfsumount.c:265: warning: no previous prototype for ‘nfsumount’

It's also a good idea if the compiler can ensure that the prototype
in nfsmount.h matches the actual function defined in nfsumount.c.

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

 utils/mount/nfsumount.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index 1514340..78a2178 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -31,6 +31,7 @@
 #include "nls.h"
 
 #include "mount_constants.h"
+#include "nfs_mount.h"
 #include "mount.h"
 #include "error.h"
 #include "network.h"

[PATCH 10/15] mount.nfs: Eliminate compiler warning in utils/mount/nfsumount.c

[Chuck Lever]

Clean up.

nfsumount.c:374: warning: ISO C forbids omitting the middle term of a ?: expression

This is also probably harmless, but let's make the code unambiguous.

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

 utils/mount/nfsumount.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index 78a2178..b1927de 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -110,7 +110,7 @@ static int del_mtab(const char *spec, const char *node)
 			res = try_remount(spec, node);
 			if (res)
 				goto writemtab;
-			return 0;
+			return EX_SUCCESS;
 		} else
 			umnt_err = errno;
 	}
@@ -128,7 +128,7 @@ static int del_mtab(const char *spec, const char *node)
 	}
 
 	if (res >= 0)
-		return 0;
+		return EX_SUCCESS;
 
 	if (umnt_err)
 		umount_error(umnt_err, node);
@@ -363,7 +363,7 @@ int nfsumount(int argc, char *argv[])
 		}
 	}
 
-	ret = 0;
+	ret = EX_SUCCESS;
 	if (mc) {
 		if (!lazy && strcmp(mc->m.mnt_type, "nfs4") != 0)
 			/* We ignore the error from nfs_umount23.
@@ -372,7 +372,7 @@ int nfsumount(int argc, char *argv[])
 			 * could cause /sbin/mount to retry!
 			 */
 			nfs_umount23(mc->m.mnt_fsname, mc->m.mnt_opts);
-		ret = del_mtab(mc->m.mnt_fsname, mc->m.mnt_dir) ?: ret;
+		ret = del_mtab(mc->m.mnt_fsname, mc->m.mnt_dir);
 	} else if (*spec != '/') {
 		if (!lazy)
 			ret = nfs_umount23(spec, "tcp,v3");

[PATCH 11/15] mount.nfs: Eliminate compiler warning in utils/mount/parse_opt.c

[Chuck Lever]

parse_opt.c: In function ‘po_rightmost’:
parse_opt.c:517: warning: conversion to ‘int’ from ‘unsigned int’ may change the sign of the result

"i" contains the function's result value, so it should be defined as
the same type as the function's return type.

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

 utils/mount/parse_opt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/mount/parse_opt.c b/utils/mount/parse_opt.c
index f0918f7..ab869d9 100644
--- a/utils/mount/parse_opt.c
+++ b/utils/mount/parse_opt.c
@@ -508,7 +508,7 @@ po_found_t po_get_numeric(struct mount_options *options, char *keyword, long *va
 int po_rightmost(struct mount_options *options, const char *keys[])
 {
 	struct mount_option *option;
-	unsigned int i;
+	int i;
 
 	if (options) {
 		for (option = options->tail; option; option = option->prev) {

[PATCH 12/15] mount.nfs: Eliminate compiler warnings in utils/mount/network.c

[Chuck Lever]

Clean up.

network.c: In function ‘get_socket’:
network.c:431: warning: dereferencing type-punned pointer might break strict-aliasing rules

network.c: In function ‘probe_bothports’:
network.c:759: warning: dereferencing type-punned pointer might break strict-aliasing rules
network.c:762: warning: dereferencing type-punned pointer might break strict-aliasing rules

network.c: In function ‘nfs_probe_statd’:
network.c:775: warning: dereferencing type-punned pointer might break strict-aliasing rules

network.c: In function ‘nfs_call_umount’:
network.c:904: warning: dereferencing type-punned pointer might break strict-aliasing rules

network.c: In function ‘nfs_ca_sockname’:
network.c:1106: warning: dereferencing type-punned pointer might break strict-aliasing rules
network.c:1112: warning: dereferencing type-punned pointer might break strict-aliasing rules

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

 utils/mount/network.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index d612427..62973cf 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -59,6 +59,8 @@
 #define CONNECT_TIMEOUT	(20)
 #define MOUNT_TIMEOUT	(30)
 
+#define SAFE_SOCKADDR(x)	(struct sockaddr *)(char *)(x)
+
 extern int nfs_mount_data_version;
 extern char *progname;
 extern int verbose;
@@ -428,12 +430,12 @@ static int get_socket(struct sockaddr_in *saddr, unsigned int p_prot,
 		if (bindresvport(so, &laddr) < 0)
 			goto err_bindresvport;
 	} else {
-		cc = bind(so, (struct sockaddr *)&laddr, namelen);
+		cc = bind(so, SAFE_SOCKADDR(&laddr), namelen);
 		if (cc < 0)
 			goto err_bind;
 	}
 	if (type == SOCK_STREAM || (conn && type == SOCK_DGRAM)) {
-		cc = connect_to(so, (struct sockaddr *)saddr, namelen,
+		cc = connect_to(so, SAFE_SOCKADDR(saddr), namelen,
 				timeout);
 		if (cc < 0)
 			goto err_connect;
@@ -756,11 +758,12 @@ int nfs_probe_bothports(const struct sockaddr *mnt_saddr,
  */
 int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
 {
-	return nfs_probe_bothports((struct sockaddr *)&mnt_server->saddr,
-					sizeof(mnt_server->saddr),
+	struct sockaddr *mnt_addr = SAFE_SOCKADDR(&mnt_server->saddr);
+	struct sockaddr *nfs_addr = SAFE_SOCKADDR(&nfs_server->saddr);
+
+	return nfs_probe_bothports(mnt_addr, sizeof(mnt_server->saddr),
 					&mnt_server->pmap,
-					(struct sockaddr *)&nfs_server->saddr,
-					sizeof(nfs_server->saddr),
+					nfs_addr, sizeof(nfs_server->saddr),
 					&nfs_server->pmap);
 }
 
@@ -772,7 +775,7 @@ static int nfs_probe_statd(void)
 	};
 	rpcprog_t program = nfs_getrpcbyname(NSMPROG, nfs_ns_pgmtbl);
 
-	return nfs_getport_ping((struct sockaddr *)&addr, sizeof(addr),
+	return nfs_getport_ping(SAFE_SOCKADDR(&addr), sizeof(addr),
 				program, (rpcvers_t)1, IPPROTO_UDP);
 }
 
@@ -901,7 +904,7 @@ int nfs_advise_umount(const struct sockaddr *sap, const socklen_t salen,
  */
 int nfs_call_umount(clnt_addr_t *mnt_server, dirpath *argp)
 {
-	struct sockaddr *sap = (struct sockaddr *)&mnt_server->saddr;
+	struct sockaddr *sap = SAFE_SOCKADDR(&mnt_server->saddr);
 	socklen_t salen = sizeof(mnt_server->saddr);
 	struct pmap *pmap = &mnt_server->pmap;
 	CLIENT *clnt;
@@ -1103,13 +1106,13 @@ static int nfs_ca_sockname(const struct sockaddr *sap, const socklen_t salen,
 
 	switch (sap->sa_family) {
 	case AF_INET:
-		if (bind(sock, (struct sockaddr *)&sin, sizeof(sin)) < 0) {
+		if (bind(sock, SAFE_SOCKADDR(&sin), sizeof(sin)) < 0) {
 			close(sock);
 			return 0;
 		}
 		break;
 	case AF_INET6:
-		if (bind(sock, (struct sockaddr *)&sin6, sizeof(sin6)) < 0) {
+		if (bind(sock, SAFE_SOCKADDR(&sin6), sizeof(sin6)) < 0) {
 			close(sock);
 			return 0;
 		}

[PATCH 13/15] mount.nfs: mountproto does not support RDMA

[Chuck Lever]

Clean up.  Our client does not support the MNT protocol on RDMA.

nfs_mount_protocol() isn't invoked for RDMA mounts (they are shunted
off before nfs_options2pmap() is invoked).  But in case it ever is,
it should return the expected response.

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

 utils/mount/network.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 62973cf..1ebaced 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1521,7 +1521,11 @@ nfs_mount_protocol(struct mount_options *options, unsigned long *protocol)
 	 * set @protocol to zero.  The pmap protocol value will
 	 * be filled in later by an rpcbind query in this case.
 	 */
-	return nfs_nfs_protocol(options, protocol);
+	if (!nfs_nfs_protocol(options, protocol))
+		return 0;
+	if (*protocol == NFSPROTO_RDMA)
+		*protocol = IPPROTO_TCP;
+	return 1;
 }
 
 /*

[PATCH 14/15] umount.nfs: Distinguish between nfs4 and nfs mounts

[Chuck Lever]

Neil Brown reports that umount.nfs is still confused by "-t nfs -o
vers=4" mounts.

/etc/mtab can be confused.  /proc/mounts is authoritative on the
fstype of a mount.  Have umount.nfs consult it to determine which
mechanism to use for unmounting.  The code to read /proc/mounts was
lifted from the nfsstat command.

The code introduced by this patch may look like belt-n-suspenders, but
we have two use cases to consider:

  1.  Old kernels don't support the "vers=4" mount option, so
      umount.nfs must look for the "nfs4" fstype
  2.  Upcoming kernels may eliminate support the "nfs4" fstype, so
      umount.nfs must look for the "vers=4" mount option

Thus this logic checks for "nfs4" first then looks for the NFS version
setting.

Note that we could handle unmounting entirely in the kernel, but that
won't help older kernels that have this issue.

See:
  https://bugzilla.linux-nfs.org/show_bug.cgi?id=189

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

 utils/mount/nfsumount.c |  111 ++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index b1927de..02d40ff 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -38,6 +38,9 @@
 #include "parse_opt.h"
 #include "parse_dev.h"
 
+#define MOUNTSFILE	"/proc/mounts"
+#define LINELEN		(4096)
+
 #if !defined(MNT_FORCE)
 /* dare not try to include <linux/mount.h> -- lots of errors */
 #define MNT_FORCE 1
@@ -242,6 +245,91 @@ static int nfs_umount23(const char *devname, char *string)
 	return result;
 }
 
+/*
+ * Detect NFSv4 mounts.
+ *
+ * Consult /proc/mounts to determine if the mount point
+ * is an NFSv4 mount.  The kernel is authoritative about
+ * what type of mount this is.
+ *
+ * Returns 1 if "mc" is an NFSv4 mount, zero if not, and
+ * -1 if some error occurred.
+ */
+static int nfs_umount_is_vers4(const struct mntentchn *mc)
+{
+	char buffer[LINELEN], *next;
+	int retval;
+	FILE *f;
+
+	if ((f = fopen(MOUNTSFILE, "r")) == NULL) {
+		fprintf(stderr, "%s: %s\n",
+			MOUNTSFILE, strerror(errno));
+		return -1;
+	}
+
+	retval = -1;
+	while (fgets(buffer, sizeof(buffer), f) != NULL) {
+		char *device, *mntdir, *type, *flags;
+		struct mount_options *options;
+		char *line = buffer;
+
+		next = strchr(line, '\n');
+		if (next != NULL)
+			*next = '\0';
+
+		device = strtok(line, " \t");
+		if (device == NULL)
+			continue;
+		mntdir = strtok(NULL, " \t");
+		if (mntdir == NULL)
+			continue;
+		if (strcmp(device, mc->m.mnt_fsname) != 0 &&
+		    strcmp(mntdir, mc->m.mnt_dir) != 0)
+			continue;
+
+		type = strtok(NULL, " \t");
+		if (type == NULL)
+			continue;
+		if (strcmp(type, "nfs4") == 0)
+			goto out_nfs4;
+
+		flags = strtok(NULL, " \t");
+		if (flags == NULL)
+			continue;
+		options = po_split(flags);
+		if (options != NULL) {
+			unsigned long version;
+			int rc;
+
+			rc = nfs_nfs_version(options, &version);
+			po_destroy(options);
+			if (rc && version == 4)
+				goto out_nfs4;
+		}
+
+		goto out_nfs;
+	}
+	if (retval == -1)
+		fprintf(stderr, "%s was not found in %s\n",
+			mc->m.mnt_dir, MOUNTSFILE);
+
+out:
+	fclose(f);
+	return retval;
+
+out_nfs4:
+	if (verbose)
+		fprintf(stderr, "NFSv4 mount point detected\n");
+	retval = 1;
+	goto out;
+
+out_nfs:
+	if (verbose)
+		fprintf(stderr, "Legacy NFS mount point detected\n");
+	retval = 0;
+	goto out;
+}
+
 static struct option umount_longopts[] =
 {
   { "force", 0, 0, 'f' },
@@ -365,13 +453,22 @@ int nfsumount(int argc, char *argv[])
 
 	ret = EX_SUCCESS;
 	if (mc) {
-		if (!lazy && strcmp(mc->m.mnt_type, "nfs4") != 0)
-			/* We ignore the error from nfs_umount23.
-			 * If the actual umount succeeds (in del_mtab),
-			 * we don't want to signal an error, as that
-			 * could cause /sbin/mount to retry!
-			 */
-			nfs_umount23(mc->m.mnt_fsname, mc->m.mnt_opts);
+		if (!lazy) {
+			switch (nfs_umount_is_vers4(mc)) {
+			case 0:
+				/* We ignore the error from nfs_umount23.
+				 * If the actual umount succeeds (in del_mtab),
+				 * we don't want to signal an error, as that
+				 * could cause /sbin/mount to retry!
+				 */
+				nfs_umount23(mc->m.mnt_fsname, mc->m.mnt_opts);
+				break;
+			case 1:
+				break;
+			default:
+				return EX_FAIL;
+			}
+		}
 		ret = del_mtab(mc->m.mnt_fsname, mc->m.mnt_dir);
 	} else if (*spec != '/') {
 		if (!lazy)

[PATCH 15/15] mount.nfs: don't show "remount" flag in /etc/mtab

[Chuck Lever]

Don't add the 'remount' option to /etc/mtab.  This is the same
behavior as file systems that use the monolithic /sbin/mount command.
See the MS_NOMTAB macro in utils-linux-ng/mount/mount.c.

Note that mount(8) has MS_USERS and MS_USER in the "nomtab" category
as well, but mount.nfs needs to record those values so that unmounting
a user-mounted NFS file system can work.

While we're here, fix some white space damage in fix_opts_string().

This is a partial fix for:

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=188

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

 utils/mount/mount.c           |    4 ++--
 utils/mount/mount_constants.h |    4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index 2909595..b4da21f 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -209,7 +209,7 @@ static char *fix_opts_string(int flags, const char *extra_opts)
 	}
 	if (flags & MS_USERS)
 		new_opts = xstrconcat3(new_opts, ",users", "");
-	
+
 	for (om = opt_map; om->opt != NULL; om++) {
 		if (om->skip)
 			continue;
@@ -281,7 +281,7 @@ static int add_mtab(char *spec, char *mount_point, char *fstype,
 	ment.mnt_fsname = spec;
 	ment.mnt_dir = mount_point;
 	ment.mnt_type = fstype;
-	ment.mnt_opts = fix_opts_string(flags, opts);
+	ment.mnt_opts = fix_opts_string(flags & ~MS_NOMTAB, opts);
 	ment.mnt_freq = freq;
 	ment.mnt_passno = pass;
 
diff --git a/utils/mount/mount_constants.h b/utils/mount/mount_constants.h
index cbfb099..4d050d8 100644
--- a/utils/mount/mount_constants.h
+++ b/utils/mount/mount_constants.h
@@ -64,4 +64,8 @@ if we have a stack or plain mount - mount atop of it, forming a stack. */
 #define MS_MGC_MSK 0xffff0000	/* magic flag number mask */
 #endif
 
+/* Generic options that are prevented from appearing
+ * in the options field in /etc/mtab. */
+#define MS_NOMTAB	(MS_REMOUNT)
+
 #endif	/* _NFS_UTILS_MOUNT_CONSTANTS_H */

Re: [PATCH 02/15] libnfs.a: Allow multiple RPC listeners to share listener port number

[Jim Rees]

Chuck Lever wrote:

  Normally, when "-p" is not specified on the mountd command line, the
  TI-RPC library chooses random port numbers for each listener.  If a
  port number _is_ specified on the command line, all the listeners
  will get the same port number, so SO_REUSEADDR needs to be set on
  each socket.
  
  Thus we can't let TI-RPC create the listener sockets for us in this
  case; we must create them ourselves and then set SO_REUSEADDR (and
  other socket options) by hand.

It bothers me that there are two separate code paths in two separate
libraries for these two nearly identical cases.  Wouldn't it be better to
add this functionality to tirpc?

Re: [PATCH 00/15] Fixes for nfs-utils-1.2.4

[Neil Brown]

On Sun, 10 Oct 2010 20:04:01 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Steve-
> 
> Here are fifteen patches which address several mount and mountd bugs
> and correct other minor problems in nfs-utils-1.2.3.  Now that the
> bake-a-thon event has passed, folks should have some spare moments to
> review these.

Hi,
 here is another you might like to add to the list.

gcc complained:

client.c: In function 'init_netmask6':
client.c:181:1: warning: no return statement in function returning non-void

and Suse' build system complained

I: Program returns random data in a function
E: nfs-utils no-return-in-nonvoid-function client.c:181

when I built without --enable-ipv6


Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/support/export/client.c b/support/export/client.c
index dbfc2b1..ba2db8f 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -178,6 +178,7 @@ out_badprefix:
 static int
 init_netmask6(nfs_client *UNUSED(clp), const char *UNUSED(slash))
 {
+	return 0;
 }
 #endif	/* IPV6_SUPPORTED */
 

Re: [PATCH 02/15] libnfs.a: Allow multiple RPC listeners to share listener port number

[Steve Dickson]

On 10/10/2010 08:20 PM, Jim Rees wrote:
> Chuck Lever wrote:
> 
>   Normally, when "-p" is not specified on the mountd command line, the
>   TI-RPC library chooses random port numbers for each listener.  If a
>   port number _is_ specified on the command line, all the listeners
>   will get the same port number, so SO_REUSEADDR needs to be set on
>   each socket.
>   
>   Thus we can't let TI-RPC create the listener sockets for us in this
>   case; we must create them ourselves and then set SO_REUSEADDR (and
>   other socket options) by hand.
> 
> It bothers me that there are two separate code paths in two separate
> libraries for these two nearly identical cases.  Wouldn't it be better to
> add this functionality to tirpc?
I have to agree... Why can't we simply had the tirpc code a socket
that has the SO_REUSEADDR set on it? 

steved.

Re: [PATCH 04/15] mount.nfs: Eliminate compiler warnings

[Steve Dickson]

On 10/10/2010 08:04 PM, Chuck Lever wrote:
> Clean up.
> 
> fstab.c: In function ‘lock_mtab’:
> fstab.c:385: warning: declaration of ‘errsv’ shadows a previous local
> fstab.c:367: warning: shadowed declaration is here
> fstab.c:407: warning: declaration of ‘errsv’ shadows a previous local
> fstab.c:367: warning: shadowed declaration is here
> fstab.c:417: warning: declaration of ‘tries’ shadows a previous local
> fstab.c:325: warning: shadowed declaration is here
> fstab.c:422: warning: declaration of ‘errsv’ shadows a previous local
> fstab.c:367: warning: shadowed declaration is here
Could you please post the gcc flags you using to generate 
theses warnings... These warnings are not being generated with
the default gcc flags we are currently using... 

tia,

steved.

> These are probably harmless.  Reusing a variable name, however, is a
> little confusing to follow when reading the code.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mount/fstab.c |   29 ++++++++++++++++-------------
>  1 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
> index 051fa38..a742e64 100644
> --- a/utils/mount/fstab.c
> +++ b/utils/mount/fstab.c
> @@ -364,19 +364,22 @@ lock_mtab (void) {
>  	/* Repeat until it was us who made the link */
>  	while (!we_created_lockfile) {
>  		struct flock flock;
> -		int errsv, j;
> +		int j;
>  
>  		j = link(linktargetfile, MOUNTED_LOCK);
> -		errsv = errno;
>  
> -		if (j == 0)
> -			we_created_lockfile = 1;
> +		{
> +			int errsv = errno;
>  
> -		if (j < 0 && errsv != EEXIST) {
> -			(void) unlink(linktargetfile);
> -			die (EX_FILEIO, _("can't link lock file %s: %s "
> -			     "(use -n flag to override)"),
> -			     MOUNTED_LOCK, strerror (errsv));
> +			if (j == 0)
> +				we_created_lockfile = 1;
> +
> +			if (j < 0 && errsv != EEXIST) {
> +				(void) unlink(linktargetfile);
> +				die (EX_FILEIO, _("can't link lock file %s: %s "
> +				     "(use -n flag to override)"),
> +				     MOUNTED_LOCK, strerror (errsv));
> +			}
>  		}
>  
>  		lockfile_fd = open (MOUNTED_LOCK, O_WRONLY);
> @@ -414,7 +417,7 @@ lock_mtab (void) {
>  			}
>  			(void) unlink(linktargetfile);
>  		} else {
> -			static int tries = 0;
> +			static int retries = 0;
>  
>  			/* Someone else made the link. Wait. */
>  			alarm(LOCK_TIMEOUT);
> @@ -428,10 +431,10 @@ lock_mtab (void) {
>  			alarm(0);
>  			/* Limit the number of iterations - maybe there
>  			   still is some old /etc/mtab~ */
> -			++tries;
> -			if (tries % 200 == 0)
> +			++retries;
> +			if (retries % 200 == 0)
>  			   usleep(30);
> -			if (tries > 100000) {
> +			if (retries > 100000) {
>  				(void) unlink(linktargetfile);
>  				close(lockfile_fd);
>  				die (EX_FILEIO, _("Cannot create link %s\n"
> 

Re: [PATCH 00/15] Fixes for nfs-utils-1.2.4

[Chuck Lever]

On Oct 11, 2010, at 12:16 AM, Neil Brown wrote:

> On Sun, 10 Oct 2010 20:04:01 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> Steve-
>> 
>> Here are fifteen patches which address several mount and mountd bugs
>> and correct other minor problems in nfs-utils-1.2.3.  Now that the
>> bake-a-thon event has passed, folks should have some spare moments to
>> review these.
> 
> Hi,
> here is another you might like to add to the list.

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

> 
> gcc complained:
> 
> client.c: In function 'init_netmask6':
> client.c:181:1: warning: no return statement in function returning non-void
> 
> and Suse' build system complained
> 
> I: Program returns random data in a function
> E: nfs-utils no-return-in-nonvoid-function client.c:181
> 
> when I built without --enable-ipv6
> 
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/support/export/client.c b/support/export/client.c
> index dbfc2b1..ba2db8f 100644
> --- a/support/export/client.c
> +++ b/support/export/client.c
> @@ -178,6 +178,7 @@ out_badprefix:
> static int
> init_netmask6(nfs_client *UNUSED(clp), const char *UNUSED(slash))
> {
> +	return 0;
> }
> #endif	/* IPV6_SUPPORTED */
> 

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

Re: [PATCH 02/15] libnfs.a: Allow multiple RPC listeners to share listener port number

[Chuck Lever]

On Oct 11, 2010, at 9:22 AM, Steve Dickson wrote:

> 
> 
> On 10/10/2010 08:20 PM, Jim Rees wrote:
>> Chuck Lever wrote:
>> 
>>  Normally, when "-p" is not specified on the mountd command line, the
>>  TI-RPC library chooses random port numbers for each listener.  If a
>>  port number _is_ specified on the command line, all the listeners
>>  will get the same port number, so SO_REUSEADDR needs to be set on
>>  each socket.
>> 
>>  Thus we can't let TI-RPC create the listener sockets for us in this
>>  case; we must create them ourselves and then set SO_REUSEADDR (and
>>  other socket options) by hand.
>> 
>> It bothers me that there are two separate code paths in two separate
>> libraries for these two nearly identical cases.

Are there?  Where's the other one?

The two implementations I'm aware of are both in libnfs.a.  One is for legacy RPC, and the other is for TI-RPC, and they both reside in support/nfs/ (and thus in libnfs.a, which is shared by the daemons in nfs-utils).  What am I missing?

>>  Wouldn't it be better to
>> add this functionality to tirpc?
> I have to agree... Why can't we simply had the tirpc code a socket
> that has the SO_REUSEADDR set on it?

I'm not saying absolutely not, but ATM I don't see the utility of doing what you suggest.

TI-RPC has a more-or-less standard API across O/S implementations.  I would think we should avoid making Linux-specific changes to the API, as that would limit the portability of RPC applications on Linux.  That's why I think this kind of thing belongs in nfs-utils, and not in TI-RPC.  If we put this code into TI-RPC, would we also need to do the same for the legacy glibc RPC implementation?

Secondly, SO_REUSEADDR is only half the fix.  The other half is xprt caching.  Simply changing the library to set SO_REUSEADDR is going to help only the case where the application wants just one version (eg, just MNT v1).

Creating a socket and passing it to svc_tli_create(3t) is exactly how the library was intended to be used in this case.  This is how rpcbind works, for instance, which suggests to me that anyone familiar with RPC servers will recognize this code idiom for what it is.

The Sun ONC+ documentation also recommends the use of svc_tli_create(3t) when an application needs to pass in a socket or specify a bind address.  This is referred to as the "expert" level server-side interface, which suggests this is going to be used rarely and only in special situations.

Can you suggest an API change to TI-RPC that would allow server applications to request an internally created socket with SO_REUSEADDR set?

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

Re: [PATCH 04/15] mount.nfs: Eliminate compiler warnings

[Chuck Lever]

On Oct 11, 2010, at 9:32 AM, Steve Dickson wrote:

> 
> 
> On 10/10/2010 08:04 PM, Chuck Lever wrote:
>> Clean up.
>> 
>> fstab.c: In function ‘lock_mtab’:
>> fstab.c:385: warning: declaration of ‘errsv’ shadows a previous local
>> fstab.c:367: warning: shadowed declaration is here
>> fstab.c:407: warning: declaration of ‘errsv’ shadows a previous local
>> fstab.c:367: warning: shadowed declaration is here
>> fstab.c:417: warning: declaration of ‘tries’ shadows a previous local
>> fstab.c:325: warning: shadowed declaration is here
>> fstab.c:422: warning: declaration of ‘errsv’ shadows a previous local
>> fstab.c:367: warning: shadowed declaration is here
> Could you please post the gcc flags you using to generate 
> theses warnings... These warnings are not being generated with
> the default gcc flags we are currently using... 

Since I've already privately sent you the compiler flags I use, would you like me to repost all of these compiler warning patches with the compiler command line options listed in the patch descriptions?

You can probably see the warnings fixed in this particular patch with "-Wshadow"

The exact warnings issued vary with compiler version and host hardware platform, so I can include that information as well, if desired.

gcc (GCC) 4.4.4 20100630 (Red Hat 4.4.4-10)

2.6.34.7-56.fc13.i686.PAE i386

Let me know if you need more.

> tia,
> 
> steved.
> 
>> These are probably harmless.  Reusing a variable name, however, is a
>> little confusing to follow when reading the code.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> utils/mount/fstab.c |   29 ++++++++++++++++-------------
>> 1 files changed, 16 insertions(+), 13 deletions(-)
>> 
>> diff --git a/utils/mount/fstab.c b/utils/mount/fstab.c
>> index 051fa38..a742e64 100644
>> --- a/utils/mount/fstab.c
>> +++ b/utils/mount/fstab.c
>> @@ -364,19 +364,22 @@ lock_mtab (void) {
>> 	/* Repeat until it was us who made the link */
>> 	while (!we_created_lockfile) {
>> 		struct flock flock;
>> -		int errsv, j;
>> +		int j;
>> 
>> 		j = link(linktargetfile, MOUNTED_LOCK);
>> -		errsv = errno;
>> 
>> -		if (j == 0)
>> -			we_created_lockfile = 1;
>> +		{
>> +			int errsv = errno;
>> 
>> -		if (j < 0 && errsv != EEXIST) {
>> -			(void) unlink(linktargetfile);
>> -			die (EX_FILEIO, _("can't link lock file %s: %s "
>> -			     "(use -n flag to override)"),
>> -			     MOUNTED_LOCK, strerror (errsv));
>> +			if (j == 0)
>> +				we_created_lockfile = 1;
>> +
>> +			if (j < 0 && errsv != EEXIST) {
>> +				(void) unlink(linktargetfile);
>> +				die (EX_FILEIO, _("can't link lock file %s: %s "
>> +				     "(use -n flag to override)"),
>> +				     MOUNTED_LOCK, strerror (errsv));
>> +			}
>> 		}
>> 
>> 		lockfile_fd = open (MOUNTED_LOCK, O_WRONLY);
>> @@ -414,7 +417,7 @@ lock_mtab (void) {
>> 			}
>> 			(void) unlink(linktargetfile);
>> 		} else {
>> -			static int tries = 0;
>> +			static int retries = 0;
>> 
>> 			/* Someone else made the link. Wait. */
>> 			alarm(LOCK_TIMEOUT);
>> @@ -428,10 +431,10 @@ lock_mtab (void) {
>> 			alarm(0);
>> 			/* Limit the number of iterations - maybe there
>> 			   still is some old /etc/mtab~ */
>> -			++tries;
>> -			if (tries % 200 == 0)
>> +			++retries;
>> +			if (retries % 200 == 0)
>> 			   usleep(30);
>> -			if (tries > 100000) {
>> +			if (retries > 100000) {
>> 				(void) unlink(linktargetfile);
>> 				close(lockfile_fd);
>> 				die (EX_FILEIO, _("Cannot create link %s\n"
>> 

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

Re: [PATCH 04/15] mount.nfs: Eliminate compiler warnings

[Steve Dickson]

On 10/11/2010 12:18 PM, Chuck Lever wrote:
>=20
> On Oct 11, 2010, at 9:32 AM, Steve Dickson wrote:
>=20
>>
>>
>> On 10/10/2010 08:04 PM, Chuck Lever wrote:
>>> Clean up.
>>>
>>> fstab.c: In function =91lock_mtab=92:
>>> fstab.c:385: warning: declaration of =91errsv=92 shadows a previous=
 local
>>> fstab.c:367: warning: shadowed declaration is here
>>> fstab.c:407: warning: declaration of =91errsv=92 shadows a previous=
 local
>>> fstab.c:367: warning: shadowed declaration is here
>>> fstab.c:417: warning: declaration of =91tries=92 shadows a previous=
 local
>>> fstab.c:325: warning: shadowed declaration is here
>>> fstab.c:422: warning: declaration of =91errsv=92 shadows a previous=
 local
>>> fstab.c:367: warning: shadowed declaration is here
>> Could you please post the gcc flags you using to generate=20
>> theses warnings... These warnings are not being generated with
>> the default gcc flags we are currently using...=20
>=20
> Since I've already privately sent you the compiler flags I use,=20
> would you like me to repost all of these compiler warning patches=20
> with the compiler command line options listed in the patch descriptio=
ns?
No... I'll just dig out those flags you sent me..

>=20
> You can probably see the warnings fixed in this particular patch with=
 "-Wshadow"
Thanks... I'll give that a try...

steved.

Re: [PATCH 02/15] libnfs.a: Allow multiple RPC listeners to share listener port number

[Jim Rees]

Chuck Lever wrote:

  >> It bothers me that there are two separate code paths in two separate
  >> libraries for these two nearly identical cases.
  
  Are there?  Where's the other one?
  
  The two implementations I'm aware of are both in libnfs.a.  One is for
  legacy RPC, and the other is for TI-RPC, and they both reside in
  support/nfs/ (and thus in libnfs.a, which is shared by the daemons in
  nfs-utils).  What am I missing?

Your svc_create_sock() duplicates code in svc_tli_create(), including the
calls to socket(), bind(), and listen().

I'll buy your argument that we shouldn't change the tirpc api and that
REUSEADDR is only half the fix.  But it still bothers me.

Re: [PATCH 02/15] libnfs.a: Allow multiple RPC listeners to share listener port number

[Steve Dickson]

On 10/11/2010 04:00 PM, Jim Rees wrote:
> Chuck Lever wrote:
> 
>   >> It bothers me that there are two separate code paths in two separate
>   >> libraries for these two nearly identical cases.
>   
>   Are there?  Where's the other one?
>   
>   The two implementations I'm aware of are both in libnfs.a.  One is for
>   legacy RPC, and the other is for TI-RPC, and they both reside in
>   support/nfs/ (and thus in libnfs.a, which is shared by the daemons in
>   nfs-utils).  What am I missing?
> 
> Your svc_create_sock() duplicates code in svc_tli_create(), including the
> calls to socket(), bind(), and listen().
> 
> I'll buy your argument that we shouldn't change the tirpc api and that
> REUSEADDR is only half the fix.  But it still bothers me.
I agree that adding the use of REUSEADDR to the libtirpc API is
probably not way to go... So I'll take the patch as is..

steved.

Re: [PATCH 01/15] mountd: Clear mountd registrations at start up

[Steve Dickson]

On 10/10/2010 08:04 PM, Chuck Lever wrote:
> Clear stale MNT registrations before mountd tries to create fresh
> listeners, to ensure that mountd starts.  This is also what statd
> does.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  utils/mountd/mountd.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
> index d309950..7e0cf6a 100644
> --- a/utils/mountd/mountd.c
> +++ b/utils/mountd/mountd.c
> @@ -840,6 +840,7 @@ main(int argc, char **argv)
>  	if (new_cache)
>  		cache_open();
>  
> +	unregister_services();
>  	if (version2()) {
>  		listeners += nfs_svc_create("mountd", MOUNTPROG,
>  					MOUNTVERS, mount_dispatch, port);
> 
Question, since unregister_services() only unregisters version
that are currently requested, won't it miss unregistering 
version that are not currently requested, ones that are left over
from a previous instant of mountd? 

The point being all versions need to be unregistered at his point, 
not just the ones currently being requested. 

steved.

Re: [PATCH 01/15] mountd: Clear mountd registrations at start up

[Steve Dickson]

On 10/13/2010 10:55 AM, Steve Dickson wrote:
> 
> 
> On 10/10/2010 08:04 PM, Chuck Lever wrote:
>> Clear stale MNT registrations before mountd tries to create fresh
>> listeners, to ensure that mountd starts.  This is also what statd
>> does.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  utils/mountd/mountd.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
>> index d309950..7e0cf6a 100644
>> --- a/utils/mountd/mountd.c
>> +++ b/utils/mountd/mountd.c
>> @@ -840,6 +840,7 @@ main(int argc, char **argv)
>>  	if (new_cache)
>>  		cache_open();
>>  
>> +	unregister_services();
>>  	if (version2()) {
>>  		listeners += nfs_svc_create("mountd", MOUNTPROG,
>>  					MOUNTVERS, mount_dispatch, port);
>>
> Question, since unregister_services() only unregisters version
> that are currently requested, won't it miss unregistering 
> version that are not currently requested, ones that are left over
> from a previous instant of mountd? 
> 
> The point being all versions need to be unregistered at his point, 
> not just the ones currently being requested. 
Something like:

mountd: Clear mountd registrations at start up

Clear stale MNT registrations before mountd tries to create fresh
listeners, to ensure that mountd starts.  This is also what statd
does.

Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index d309950..c36c471 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -97,13 +97,13 @@ static int version_any(void)
 }
 
 static void
-unregister_services (void)
+unregister_services (int all)
 {
-	if (version2()) {
+	if (all || version2()) {
 		nfs_svc_unregister(MOUNTPROG, MOUNTVERS);
 		nfs_svc_unregister(MOUNTPROG, MOUNTVERS_POSIX);
 	}
-	if (version3())
+	if (all || version3())
 		nfs_svc_unregister(MOUNTPROG, MOUNTVERS_NFSV3);
 }
 
@@ -180,7 +180,7 @@ fork_workers(void)
 
 	/* in parent */
 	wait_for_workers();
-	unregister_services();
+	unregister_services(0);
 	cleanup_lockfiles();
 	xlog(L_NOTICE, "mountd: no more workers, exiting\n");
 	exit(0);
@@ -192,7 +192,7 @@ fork_workers(void)
 static void 
 killer (int sig)
 {
-	unregister_services();
+	unregister_services(0);
 	if (num_threads > 1) {
 		/* play Kronos and eat our children */
 		kill(0, SIGTERM);
@@ -840,6 +840,7 @@ main(int argc, char **argv)
 	if (new_cache)
 		cache_open();
 
+	unregister_services(1);
 	if (version2()) {
 		listeners += nfs_svc_create("mountd", MOUNTPROG,
 					MOUNTVERS, mount_dispatch, port);
@@ -895,7 +896,7 @@ main(int argc, char **argv)
 	my_svc_run();
 
 	xlog(L_ERROR, "RPC service loop terminated unexpectedly. Exiting...\n");
-	unregister_services();
+	unregister_services(0);
 	exit(1);
 }
 

Re: [PATCH 01/15] mountd: Clear mountd registrations at start up

[Chuck Lever]

On Oct 13, 2010, at 11:12 AM, Steve Dickson wrote:

> 
> 
> On 10/13/2010 10:55 AM, Steve Dickson wrote:
>> 
>> 
>> On 10/10/2010 08:04 PM, Chuck Lever wrote:
>>> Clear stale MNT registrations before mountd tries to create fresh
>>> listeners, to ensure that mountd starts.  This is also what statd
>>> does.
>>> 
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> 
>>> utils/mountd/mountd.c |    1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
>>> index d309950..7e0cf6a 100644
>>> --- a/utils/mountd/mountd.c
>>> +++ b/utils/mountd/mountd.c
>>> @@ -840,6 +840,7 @@ main(int argc, char **argv)
>>> 	if (new_cache)
>>> 		cache_open();
>>> 
>>> +	unregister_services();
>>> 	if (version2()) {
>>> 		listeners += nfs_svc_create("mountd", MOUNTPROG,
>>> 					MOUNTVERS, mount_dispatch, port);
>>> 
>> Question, since unregister_services() only unregisters version
>> that are currently requested, won't it miss unregistering 
>> version that are not currently requested, ones that are left over
>> from a previous instant of mountd? 
>> 
>> The point being all versions need to be unregistered at his point, 
>> not just the ones currently being requested. 

I actually don't see a case where you would want to unregister just specific versions.  unregister_services() is currently too clever, by half.

The best thing to do is have unregister_services unregister everything, unconditionally.  So, add the new unregister_services() call site as my patch does, but change unregister_services() itself to do something like:

	nfs_svc_unregister(MOUNTPROG, MOUNTVERS);
	nfs_svc_unregister(MOUNTPROG, MOUNTVERS_POSIX);
	nfs_svc_unregister(MOUNTPROG, NFSV3);

What do you think?

> Something like:
> 
> mountd: Clear mountd registrations at start up
> 
> Clear stale MNT registrations before mountd tries to create fresh
> listeners, to ensure that mountd starts.  This is also what statd
> does.
> 
> Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
> index d309950..c36c471 100644
> --- a/utils/mountd/mountd.c
> +++ b/utils/mountd/mountd.c
> @@ -97,13 +97,13 @@ static int version_any(void)
> }
> 
> static void
> -unregister_services (void)
> +unregister_services (int all)
> {
> -	if (version2()) {
> +	if (all || version2()) {
> 		nfs_svc_unregister(MOUNTPROG, MOUNTVERS);
> 		nfs_svc_unregister(MOUNTPROG, MOUNTVERS_POSIX);
> 	}
> -	if (version3())
> +	if (all || version3())
> 		nfs_svc_unregister(MOUNTPROG, MOUNTVERS_NFSV3);
> }
> 
> @@ -180,7 +180,7 @@ fork_workers(void)
> 
> 	/* in parent */
> 	wait_for_workers();
> -	unregister_services();
> +	unregister_services(0);
> 	cleanup_lockfiles();
> 	xlog(L_NOTICE, "mountd: no more workers, exiting\n");
> 	exit(0);
> @@ -192,7 +192,7 @@ fork_workers(void)
> static void 
> killer (int sig)
> {
> -	unregister_services();
> +	unregister_services(0);
> 	if (num_threads > 1) {
> 		/* play Kronos and eat our children */
> 		kill(0, SIGTERM);
> @@ -840,6 +840,7 @@ main(int argc, char **argv)
> 	if (new_cache)
> 		cache_open();
> 
> +	unregister_services(1);
> 	if (version2()) {
> 		listeners += nfs_svc_create("mountd", MOUNTPROG,
> 					MOUNTVERS, mount_dispatch, port);
> @@ -895,7 +896,7 @@ main(int argc, char **argv)
> 	my_svc_run();
> 
> 	xlog(L_ERROR, "RPC service loop terminated unexpectedly. Exiting...\n");
> -	unregister_services();
> +	unregister_services(0);
> 	exit(1);
> }
> 
> 

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

Re: [PATCH 01/15] mountd: Clear mountd registrations at start up

[Steve Dickson]

On 10/13/2010 04:12 PM, Chuck Lever wrote:
> 
> On Oct 13, 2010, at 11:12 AM, Steve Dickson wrote:
> 
>>
>>
>> On 10/13/2010 10:55 AM, Steve Dickson wrote:
>>>
>>>
>>> On 10/10/2010 08:04 PM, Chuck Lever wrote:
>>>> Clear stale MNT registrations before mountd tries to create fresh
>>>> listeners, to ensure that mountd starts.  This is also what statd
>>>> does.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> utils/mountd/mountd.c |    1 +
>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
>>>> index d309950..7e0cf6a 100644
>>>> --- a/utils/mountd/mountd.c
>>>> +++ b/utils/mountd/mountd.c
>>>> @@ -840,6 +840,7 @@ main(int argc, char **argv)
>>>> 	if (new_cache)
>>>> 		cache_open();
>>>>
>>>> +	unregister_services();
>>>> 	if (version2()) {
>>>> 		listeners += nfs_svc_create("mountd", MOUNTPROG,
>>>> 					MOUNTVERS, mount_dispatch, port);
>>>>
>>> Question, since unregister_services() only unregisters version
>>> that are currently requested, won't it miss unregistering 
>>> version that are not currently requested, ones that are left over
>>> from a previous instant of mountd? 
>>>
>>> The point being all versions need to be unregistered at his point, 
>>> not just the ones currently being requested. 
> 
> I actually don't see a case where you would want to unregister just specific versions.  unregister_services() is currently too clever, by half.
> 
> The best thing to do is have unregister_services unregister everything, unconditionally.  So, add the new unregister_services() call site as my patch does, but change unregister_services() itself to do something like:
> 
> 	nfs_svc_unregister(MOUNTPROG, MOUNTVERS);
> 	nfs_svc_unregister(MOUNTPROG, MOUNTVERS_POSIX);
> 	nfs_svc_unregister(MOUNTPROG, NFSV3);
> 
> What do you think?
Well I thinking we only time we should unconditionally unregister 
everything is during startup... since the rest of the time we
know which versions have been started... but either way if fine
by me... As long as rpcbind does not fill the system log with 
messages complaining about unregistering services that have
not be registered... but I find that out in my testing... 

steved.

Re: [PATCH 01/15] mountd: Clear mountd registrations at start up

[Steve Dickson]

On 10/13/2010 04:12 PM, Chuck Lever wrote:
> 
> On Oct 13, 2010, at 11:12 AM, Steve Dickson wrote:
> 
>>
>>
>> On 10/13/2010 10:55 AM, Steve Dickson wrote:
>>>
>>>
>>> On 10/10/2010 08:04 PM, Chuck Lever wrote:
>>>> Clear stale MNT registrations before mountd tries to create fresh
>>>> listeners, to ensure that mountd starts.  This is also what statd
>>>> does.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> utils/mountd/mountd.c |    1 +
>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
>>>> index d309950..7e0cf6a 100644
>>>> --- a/utils/mountd/mountd.c
>>>> +++ b/utils/mountd/mountd.c
>>>> @@ -840,6 +840,7 @@ main(int argc, char **argv)
>>>> 	if (new_cache)
>>>> 		cache_open();
>>>>
>>>> +	unregister_services();
>>>> 	if (version2()) {
>>>> 		listeners += nfs_svc_create("mountd", MOUNTPROG,
>>>> 					MOUNTVERS, mount_dispatch, port);
>>>>
>>> Question, since unregister_services() only unregisters version
>>> that are currently requested, won't it miss unregistering 
>>> version that are not currently requested, ones that are left over
>>> from a previous instant of mountd? 
>>>
>>> The point being all versions need to be unregistered at his point, 
>>> not just the ones currently being requested. 
> 
> I actually don't see a case where you would want to unregister just specific versions.  unregister_services() is currently too clever, by half.
> 
> The best thing to do is have unregister_services unregister everything, unconditionally.  So, add the new unregister_services() call site as my patch does, but change unregister_services() itself to do something like:
> 
> 	nfs_svc_unregister(MOUNTPROG, MOUNTVERS);
> 	nfs_svc_unregister(MOUNTPROG, MOUNTVERS_POSIX);
> 	nfs_svc_unregister(MOUNTPROG, NFSV3);
> 
> What do you think?
This seems to work.. I'll just make the change can do the commit...

steved.

Re: [PATCH 00/15] Fixes for nfs-utils-1.2.4

[Steve Dickson]

On 10/10/2010 08:04 PM, Chuck Lever wrote:
> Steve-
> 
> Here are fifteen patches which address several mount and mountd bugs
> and correct other minor problems in nfs-utils-1.2.3.  Now that the
> bake-a-thon event has passed, folks should have some spare moments to
> review these.
> 
> This series includes the final version of the patch that addresses the
> "mountd -p <port> does not work" issue.
> 
> The series does not include a patch to address the "remount erases a
> mount point's version setting" issue.  The remount problem is not a
> 1.2.3 regression, it's been around for a very long time.  I'm still
> exploring solutions.
> 
> These patches are also available in my nfs-utils git repo on
> git.linux-nfs.org.
All 16 patches (including Neil's patch) have been committed.

steved.