[PATCH 0/5] More mountd patches

[PATCH 0/5] More mountd patches

[Chuck Lever]

Hi Steve-

Here are some clean-up and bug fix patches I'd like you to apply
before I start sending the IPv6 series.

The first two fix a bug reported by Neil.  I posted these to you a
few weeks ago, but I don't see them in your repo yet.

The next bug is the statd DNS bug that Ben Hutchings reported months
ago.  We just got confirmation that the patch does indeed fix the
problem.

The last two patches are two clean-ups from my IPv6 series.  The first
is uncontroversial.  The second converts exportfs to use xlog() more
idiomatically, allowing debugging messages in libexport.a to be
enabled from the exportfs command line.

---

Chuck Lever (5):
      exportfs: Use xlog() for error reporting
      exportfs: exportfs.c no longer needs #include "xmalloc.h"
      statd: statd fails to monitor if no reverse mapping of mon_name exists
      libexport: Fix IP address check in check_netgroup()
      libexport: Add a common exit label to check_netgroup()


 support/export/client.c   |   44 ++++++++++++++++++++++++--------
 utils/exportfs/exportfs.c |   62 ++++++++++++++++++++++++---------------------
 utils/statd/hostname.c    |    4 ++-
 3 files changed, 69 insertions(+), 41 deletions(-)

-- 
Chuck Lever

[PATCH 1/5] libexport: Add a common exit label to check_netgroup()

[Chuck Lever]

I'm about to change check_netgroup() to free dynamically allocated
resources before it returns, so a common exit point is needed.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Reviewed-by: Neil Brown <neilb@suse.de>
---

 support/export/client.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index dc01067..b1a3a09 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -496,37 +496,46 @@ check_netgroup(const nfs_client *clp, const struct addrinfo *ai)
 	int i, match;
 	char *dot;
 
+	match = 0;
+
 	/* First, try to match the hostname without
 	 * splitting off the domain */
-	if (innetgr(netgroup, hname, NULL, NULL))
-		return 1;
+	if (innetgr(netgroup, hname, NULL, NULL)) {
+		match = 1;
+		goto out;
+	}
 
 	/* See if hname aliases listed in /etc/hosts or nis[+]
 	 * match the requested netgroup */
 	hp = gethostbyname(hname);
 	if (hp != NULL) {
 		for (i = 0; hp->h_aliases[i]; i++)
-			if (innetgr(netgroup, hp->h_aliases[i], NULL, NULL))
-				return 1;
+			if (innetgr(netgroup, hp->h_aliases[i], NULL, NULL)) {
+				match = 1;
+				goto out;
+			}
 	}
 
 	/* If hname is ip address convert to FQDN */
 	tmp = host_pton(hname);
 	if (tmp != NULL) {
 		freeaddrinfo(tmp);
-		if (innetgr(netgroup, hname, NULL, NULL))
-			return 1;
+		if (innetgr(netgroup, hname, NULL, NULL)) {
+			match = 1;
+			goto out;
+		}
 	}
 
 	/* Okay, strip off the domain (if we have one) */
 	dot = strchr(hname, '.');
 	if (dot == NULL)
-		return 0;
+		goto out;
 
 	*dot = '\0';
 	match = innetgr(netgroup, hname, NULL, NULL);
 	*dot = '.';
 
+out:
 	return match;
 }
 #else	/* !HAVE_INNETGR */

[PATCH 2/5] libexport: Fix IP address check in check_netgroup()

[Chuck Lever]

Neil Brown reports that recent changes to replace
gethostby{addr,name}(3) with get{addr,info}name(3) may have
inadvertently broken netgroup support.

There used to be a gethostbyaddr(3) call in the third paragraph in
check_netgroup().  The reason for that gethostbyaddr(3) call was that
the first innetgr(3) call has already confirmed that hname is not a
member of the netgroup.  We also need to confirm that, if hname
happens to be an IP address, the hostname bound to that IP address is
not a member of the netgroup, either.

Fix this by restoring appropriate address to hostname mapping of hname
before retrying the innetgr(3) call.

See http://marc.info/?l=linux-nfs&m=128084830214653&w=2 .

Introduced by commit 0509d3428f523776ddd9d6e9fa318587d3ec7d84.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Reviewed-by: Neil Brown <neilb@suse.de>
---

 support/export/client.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/support/export/client.c b/support/export/client.c
index b1a3a09..21001ce 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -490,14 +490,19 @@ static int
 check_netgroup(const nfs_client *clp, const struct addrinfo *ai)
 {
 	const char *netgroup = clp->m_hostname + 1;
-	const char *hname = ai->ai_canonname;
 	struct addrinfo *tmp = NULL;
 	struct hostent *hp;
+	char *dot, *hname;
 	int i, match;
-	char *dot;
 
 	match = 0;
 
+	hname = strdup(ai->ai_canonname);
+	if (hname == NULL) {
+		xlog(D_GENERAL, "%s: no memory for strdup", __func__);
+		goto out;
+	}
+
 	/* First, try to match the hostname without
 	 * splitting off the domain */
 	if (innetgr(netgroup, hname, NULL, NULL)) {
@@ -516,13 +521,21 @@ check_netgroup(const nfs_client *clp, const struct addrinfo *ai)
 			}
 	}
 
-	/* If hname is ip address convert to FQDN */
+	/* If hname happens to be an IP address, convert it
+	 * to a the canonical DNS name bound to this address. */
 	tmp = host_pton(hname);
 	if (tmp != NULL) {
+		char *cname = host_canonname(tmp->ai_addr);
 		freeaddrinfo(tmp);
-		if (innetgr(netgroup, hname, NULL, NULL)) {
-			match = 1;
-			goto out;
+
+		/* The resulting FQDN may be in our netgroup. */
+		if (cname != NULL) {
+			free(hname);
+			hname = cname;
+			if (innetgr(netgroup, hname, NULL, NULL)) {
+				match = 1;
+				goto out;
+			}
 		}
 	}
 
@@ -533,9 +546,9 @@ check_netgroup(const nfs_client *clp, const struct addrinfo *ai)
 
 	*dot = '\0';
 	match = innetgr(netgroup, hname, NULL, NULL);
-	*dot = '.';
 
 out:
+	free(hname);
 	return match;
 }
 #else	/* !HAVE_INNETGR */

[PATCH 3/5] statd: statd fails to monitor if no reverse mapping of mon_name exists

[Chuck Lever]

Commit 8ce130c4 switched in the new statd_canonical_name() function
that constructs a "unique" name statd can use to uniquely identify a
monitor record.

The legacy statd would monitor a client that sent an IP address with
no reverse map as its caller_name.  To remain bug-for-bug compatible,
allow this case in the new statd.

This shouldn't be a problem: statd_canonical_name() needs to create
a unique name for the monitored host so it can keep track of monitor
requests from the same remote.  The IP address itself should work as
well as the host's canonical name, in case there is no reverse
mapping.

We still enforce the requirement that a mon_name that is a DNS name
must have a forward map to an IP address.

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

 utils/statd/hostname.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/utils/statd/hostname.c b/utils/statd/hostname.c
index 7d704cc..38f2265 100644
--- a/utils/statd/hostname.c
+++ b/utils/statd/hostname.c
@@ -212,7 +212,9 @@ statd_canonical_name(const char *hostname)
 					buf, (socklen_t)sizeof(buf));
 		freeaddrinfo(ai);
 		if (!result)
-			return NULL;
+			/* OK to use presentation address,
+			 * if no reverse map exists */
+			return strdup(hostname);
 		return strdup(buf);
 	}
 

[PATCH 4/5] exportfs: exportfs.c no longer needs #include "xmalloc.h"

[Chuck Lever]

Clean up:  No calls to xmalloc() or xstrdup() here.  No need for the
double #include of xmalloc.h.

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

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

diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index edc1625..93bad32 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -12,6 +12,8 @@
 #include <config.h>
 #endif
 
+#include <sys/types.h>
+#include <sys/stat.h>
 #include <sys/vfs.h>
 #include <sys/stat.h>
 #include <unistd.h>
@@ -20,13 +22,13 @@
 #include <string.h>
 #include <stdarg.h>
 #include <getopt.h>
+#include <fcntl.h>
 #include <netdb.h>
 #include <errno.h>
-#include "xmalloc.h"
+
 #include "misc.h"
 #include "nfslib.h"
 #include "exportfs.h"
-#include "xmalloc.h"
 #include "xlog.h"
 
 static void	export_all(int verbose);

[PATCH 5/5] exportfs: Use xlog() for error reporting

[Chuck Lever]

exportfs already invokes xlog_open() because libexport.a uses xlog()
exclusively for error reporting and debugging messages.  If we can
use xlog() throughout exportfs itself, that enables xlog debugging
messages everywhere in the code path.

In addition, use xlog() instead of fprintf(stderr) for reporting
errors in exportfs.c, to be consistent with libexport.a and other
components of nfs-utils.

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

 utils/exportfs/exportfs.c |   56 +++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index 93bad32..8496d82 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -37,7 +37,7 @@ static void	unexportfs(char *arg, int verbose);
 static void	exports_update(int verbose);
 static void	dump(int verbose);
 static void	error(nfs_export *exp, int err);
-static void	usage(void);
+static void	usage(const char *progname);
 static void	validate_export(nfs_export *exp);
 static int	matchhostname(const char *hostname1, const char *hostname2);
 
@@ -45,6 +45,7 @@ int
 main(int argc, char **argv)
 {
 	char	*options = NULL;
+	char	*progname = NULL;
 	int	f_export = 1;
 	int	f_all = 0;
 	int	f_verbose = 0;
@@ -54,7 +55,14 @@ main(int argc, char **argv)
 	int	new_cache = 0;
 	int	force_flush = 0;
 
-	xlog_open("exportfs");
+	if ((progname = strrchr(argv[0], '/')) != NULL)
+		progname++;
+	else
+		progname = argv[0];
+
+	xlog_open(progname);
+	xlog_stderr(1);
+	xlog_syslog(0);
 
 	export_errno = 0;
 
@@ -83,21 +91,21 @@ main(int argc, char **argv)
 			force_flush = 1;
 			break;
 		default:
-			usage();
+			usage(progname);
 			break;
 		}
 	}
 
 	if (optind != argc && f_all) {
-		fprintf(stderr,"exportfs: extra arguments are not permitted with -a or -r.\n");
+		xlog(L_ERROR, "extra arguments are not permitted with -a or -r");
 		return 1;
 	}
 	if (f_ignore && (f_all || ! f_export)) {
-		fprintf(stderr,"exportfs: -i not meaningful with -a, -r or -u.\n");
+		xlog(L_ERROR, "-i not meaningful with -a, -r or -u");
 		return 1;
 	}
 	if (f_reexport && ! f_export) {
-		fprintf(stderr, "exportfs: -r and -u are incompatible.\n");
+		xlog(L_ERROR, "-r and -u are incompatible");
 		return 1;
 	}
 	new_cache = check_new_cache();
@@ -106,8 +114,10 @@ main(int argc, char **argv)
 			if (new_cache)
 				cache_flush(1);
 			else {
-				fprintf(stderr, "exportfs: -f: only available with new cache controls: mount /proc/fs/nfsd first\n");
-				exit(1);
+				xlog(L_ERROR, "-f is available only "
+					"with new cache controls. "
+					"Mount /proc/fs/nfsd first");
+				return 1;
 			}
 			return 0;
 		} else {
@@ -245,7 +255,7 @@ exportfs(char *arg, char *options, int verbose)
 		*path++ = '\0';
 
 	if (!path || *path != '/') {
-		fprintf(stderr, "Invalid exporting option: %s\n", arg);
+		xlog(L_ERROR, "Invalid exporting option: %s", arg);
 		return;
 	}
 
@@ -291,8 +301,7 @@ unexportfs(char *arg, int verbose)
 		*path++ = '\0';
 
 	if (!path || *path != '/') {
-		fprintf(stderr, "Invalid unexporting option: %s\n",
-			arg);
+		xlog(L_ERROR, "Invalid unexporting option: %s", arg);
 		return;
 	}
 
@@ -387,14 +396,12 @@ validate_export(nfs_export *exp)
 	int fs_has_fsid = 0;
 
 	if (stat(path, &stb) < 0) {
-		fprintf(stderr, "exportfs: Warning: %s does not exist\n",
-			path);
+		xlog(L_ERROR, "Failed to stat %s: %m \n", path);
 		return;
 	}
 	if (!S_ISDIR(stb.st_mode) && !S_ISREG(stb.st_mode)) {
-		fprintf(stderr, "exportfs: Warning: %s is neither "
-			"a directory nor a file.\n"
-			"     remote access will fail\n", path);
+		xlog(L_ERROR, "%s is neither a directory nor a file. "
+			"Remote access will fail", path);
 		return;
 	}
 	if (!can_test())
@@ -407,19 +414,14 @@ validate_export(nfs_export *exp)
 	if ((exp->m_export.e_flags & NFSEXP_FSID) || exp->m_export.e_uuid ||
 	    fs_has_fsid) {
 		if ( !test_export(path, 1)) {
-			fprintf(stderr, "exportfs: Warning: %s does not "
-				"support NFS export.\n",
-				path);
+			xlog(L_ERROR, "%s does not support NFS export", path);
 			return;
 		}
 	} else if ( ! test_export(path, 0)) {
 		if (test_export(path, 1))
-			fprintf(stderr, "exportfs: Warning: %s requires fsid= "
-				"for NFS export\n", path);
+			xlog(L_ERROR, "%s requires fsid= for NFS export", path);
 		else
-			fprintf(stderr, "exportfs: Warning: %s does not "
-				"support NFS export.\n",
-				path);
+			xlog(L_ERROR, "%s does not support NFS export", path);
 		return;
 
 	}
@@ -602,13 +604,13 @@ dump(int verbose)
 static void
 error(nfs_export *exp, int err)
 {
-	fprintf(stderr, "%s:%s: %s\n", exp->m_client->m_hostname, 
+	xlog(L_ERROR, "%s:%s: %s\n", exp->m_client->m_hostname,
 		exp->m_export.e_path, strerror(err));
 }
 
 static void
-usage(void)
+usage(const char *progname)
 {
-	fprintf(stderr, "usage: exportfs [-aruv] [host:/path]\n");
+	fprintf(stderr, "usage: %s [-aruv] [host:/path]\n", progname);
 	exit(1);
 }

Re: [PATCH 0/5] More mountd patches

[Steve Dickson]

On 08/23/2010 01:10 PM, Chuck Lever wrote:
> Hi Steve-
> 
> Here are some clean-up and bug fix patches I'd like you to apply
> before I start sending the IPv6 series.
> 
> The first two fix a bug reported by Neil.  I posted these to you a
> few weeks ago, but I don't see them in your repo yet.
> 
> The next bug is the statd DNS bug that Ben Hutchings reported months
> ago.  We just got confirmation that the patch does indeed fix the
> problem.
> 
> The last two patches are two clean-ups from my IPv6 series.  The first
> is uncontroversial.  The second converts exportfs to use xlog() more
> idiomatically, allowing debugging messages in libexport.a to be
> enabled from the exportfs command line.
> 
> ---
> 
> Chuck Lever (5):
>       exportfs: Use xlog() for error reporting
>       exportfs: exportfs.c no longer needs #include "xmalloc.h"
>       statd: statd fails to monitor if no reverse mapping of mon_name exists
>       libexport: Fix IP address check in check_netgroup()
>       libexport: Add a common exit label to check_netgroup()
> 
> 
>  support/export/client.c   |   44 ++++++++++++++++++++++++--------
>  utils/exportfs/exportfs.c |   62 ++++++++++++++++++++++++---------------------
>  utils/statd/hostname.c    |    4 ++-
>  3 files changed, 69 insertions(+), 41 deletions(-)
> 
All five have been committed... Thanks!

steved.