[PATCH 0/5] Bug fixes and NFS junction support

[PATCH 0/5] Bug fixes and NFS junction support

[Chuck Lever]

Four minor bug fix patches, for what they're worth, then...

The fifth patch adds support in mountd for loading a plug-in to
resolve NFS junctions.  The plug-in will be provided by fedfs-utils.
Source code will be posted very soon for review.

We'd like to see this in Fedora 17, if convenient.  Meantime, let's
review what I've got.

---

Chuck Lever (5):
      mountd: Support junction management plug-ins
      mountd: remove newline from xlog() format specifier strings
      mountd: Plug v4root memory leak
      configure.ac: Don't check for AI_ADDRCONFIG
      configure.ac: Clean up help string for --with-statdpath


 aclocal/ipv6.m4          |   11 --
 configure.ac             |   10 +-
 utils/mountd/Makefile.am |    2 
 utils/mountd/cache.c     |  225 ++++++++++++++++++++++++++++++++++++++++++++++
 utils/mountd/fsloc.c     |   10 +-
 utils/mountd/v4root.c    |    2 
 6 files changed, 237 insertions(+), 23 deletions(-)

-- 
Chuck Lever

[PATCH 1/5] configure.ac: Clean up help string for --with-statdpath

[Chuck Lever]

Neither m4 nor the vim colorizer like unbalanced single quotes.
Similar change as commit 34ee5730.

Also, the default value of the command line option is conventionally
listed in the right-hand side argument of AC_HELP_STRING.

Introduced by commit 5c3e2665: "statd: Decouple statd's state
directory from the NFS state directory," (September 20, 2011).

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

 configure.ac |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index f101b86..d3656bf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -24,9 +24,8 @@ AC_ARG_WITH(statedir,
 	statedir=/var/lib/nfs)
 	AC_SUBST(statedir)
 AC_ARG_WITH(statdpath,
-	[AC_HELP_STRING([--with-statdpath=/foo @<:@default=/var/lib/nfs@:>@],
-		[define statd's state dir as /foo instead of the NFS statedir]
-	)],
+	[AC_HELP_STRING([--with-statdpath=/foo],
+			[define the statd state dir as /foo instead of the NFS statedir @<:@default=/var/lib/nfs@:>@])],
 	statdpath=$withval,
 	statdpath=$statedir
 	)

[PATCH 2/5] configure.ac: Don't check for AI_ADDRCONFIG

[Chuck Lever]

Commit 1ea2c3be: "nfs-utils: Remove all uses of AI_ADDRCONFIG,"
(October 28, 2010) removed the last use of AI_ADDRCONFIG.  Even so,
ipv6.m4 uses this check to ensure that the local getaddrinfo(3)
implementation is recent.  Maybe we shouldn't bother.

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

 aclocal/ipv6.m4 |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/aclocal/ipv6.m4 b/aclocal/ipv6.m4
index 5ee8fb6..4e39fbe 100644
--- a/aclocal/ipv6.m4
+++ b/aclocal/ipv6.m4
@@ -2,11 +2,6 @@ dnl Checks for IPv6 support
 dnl
 AC_DEFUN([AC_IPV6], [
 
-  AC_CHECK_DECL([AI_ADDRCONFIG],
-                [AC_DEFINE([HAVE_DECL_AI_ADDRCONFIG], 1,
-                           [Define this to 1 if AI_ADDRCONFIG macro is defined])], ,
-                [ #include <netdb.h> ])
-
   if test "$enable_ipv6" = yes; then
 
     dnl TI-RPC required for IPv6
@@ -18,12 +13,6 @@ AC_DEFUN([AC_IPV6], [
     AC_CHECK_FUNCS([getifaddrs getnameinfo bindresvport_sa], ,
                    [AC_MSG_ERROR([Missing library functions needed for IPv6.])])
 
-    dnl Need to detect presence of IPv6 networking at run time via
-    dnl getaddrinfo(3); old versions of glibc do not support ADDRCONFIG
-    AC_CHECK_DECL([AI_ADDRCONFIG], ,
-                  [AC_MSG_ERROR([full getaddrinfo(3) implementation needed for IPv6 support])],
-                  [ #include <netdb.h> ])
-
   fi
 
 ])dnl

[PATCH 3/5] mountd: Plug v4root memory leak

[Chuck Lever]

Valgrind reports that the memory allocated for eep's e_hostname field
was not being freed.  eep is not visible outside of v4root_create(),
so we don't need to strdup() that string.

Introduced by commit 3b777b0 "exports: NFSv4 pseudoroot support
routines" (Dec 1, 2009).

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

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

diff --git a/utils/mountd/v4root.c b/utils/mountd/v4root.c
index c33a5a9..81f813b 100644
--- a/utils/mountd/v4root.c
+++ b/utils/mountd/v4root.c
@@ -83,7 +83,7 @@ v4root_create(char *path, nfs_export *export)
 	struct exportent *curexp = &export->m_export;
 
 	dupexportent(&eep, &pseudo_root.m_export);
-	eep.e_hostname = strdup(curexp->e_hostname);
+	eep.e_hostname = curexp->e_hostname;
 	strncpy(eep.e_path, path, sizeof(eep.e_path));
 	if (strcmp(path, "/") != 0)
 		eep.e_flags &= ~NFSEXP_FSID;

[PATCH 4/5] mountd: remove newline from xlog() format specifier strings

[Chuck Lever]

Clean up: xlog() already adds a newline to the end of each line of
output.  Remove the superfluous newline from a number of xlog()
call sites in mountd.

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

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

diff --git a/utils/mountd/fsloc.c b/utils/mountd/fsloc.c
index e2add2d..704b7a0 100644
--- a/utils/mountd/fsloc.c
+++ b/utils/mountd/fsloc.c
@@ -40,12 +40,12 @@ static void replicas_print(struct servers *sp)
 {
 	int i;
 	if (!sp) {
-		xlog(L_NOTICE, "NULL replicas pointer\n");
+		xlog(L_NOTICE, "NULL replicas pointer");
 		return;
 	}
-	xlog(L_NOTICE, "replicas listsize=%i\n", sp->h_num);
+	xlog(L_NOTICE, "replicas listsize=%i", sp->h_num);
 	for (i=0; i<sp->h_num; i++) {
-		xlog(L_NOTICE, "    %s:%s\n",
+		xlog(L_NOTICE, "    %s:%s",
 		       sp->h_mp[i]->h_host, sp->h_mp[i]->h_path);
 	}
 }
@@ -125,13 +125,13 @@ static struct servers *method_list(char *data)
 	int i, listsize;
 	struct servers *rv=NULL;
 
-	xlog(L_NOTICE, "method_list(%s)\n", data);
+	xlog(L_NOTICE, "method_list(%s)", data);
 	for (ptr--, listsize=1; ptr; ptr=index(ptr, ':'), listsize++)
 		ptr++;
 	list = malloc(listsize * sizeof(char *));
 	copy = strdup(data);
 	if (copy)
-		xlog(L_NOTICE, "converted to %s\n", copy);
+		xlog(L_NOTICE, "converted to %s", copy);
 	if (list && copy) {
 		ptr = copy;
 		for (i=0; i<listsize; i++) {

[PATCH 5/5] mountd: Support junction management plug-ins

[Chuck Lever]

To support FedFS and NFS junctions without introducing additional
build-time or run-time dependencies on nfs-utils, the community has
chosen to use a dynamically loadable library to handle junction
resolution.

There is one plug-in library for mountd that will handle any NFS-
related junction type.  Currently there are two types:

  o nfs-basic	locally stored file set location data, and

  o nfs-fedfs	file set location data stored on an LDAP server

mountd's support for this library is enabled at build time by the
presence of the junction API definition header:

  /usr/include/nfs-plugin.h

If this header is not found on the build system, mountd will build
without junction support, and will operate as before.

Note that mountd does not cache junction resolution results.  NFSD
already caches these results in its exports cache.  Thus each time
NFSD calls up to mountd, it is, in essence, requesting a fresh
junction resolution operation, not a cached response.

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

 configure.ac             |    5 +
 utils/mountd/Makefile.am |    2 
 utils/mountd/cache.c     |  225 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index d3656bf..920e8da 100644
--- a/configure.ac
+++ b/configure.ac
@@ -248,6 +248,8 @@ AC_CHECK_FUNC([getservbyname], ,
 
 AC_CHECK_LIB([crypt], [crypt], [LIBCRYPT="-lcrypt"])
 
+AC_CHECK_LIB([dl], [dlclose], [LIBDL="-ldl"])
+
 if test "$enable_nfsv4" = yes; then
   dnl check for libevent libraries and headers
   AC_LIBEVENT
@@ -298,6 +300,7 @@ AC_SUBST(LIBSOCKET)
 AC_SUBST(LIBCRYPT)
 AC_SUBST(LIBBSD)
 AC_SUBST(LIBBLKID)
+AC_SUBST(LIBDL)
 
 if test "$enable_libmount" != no; then
    AC_CHECK_LIB(mount, mnt_context_do_mount, [LIBMOUNT="-lmount"], AC_MSG_ERROR([libmount needed]))
@@ -335,7 +338,7 @@ AC_CHECK_HEADERS([arpa/inet.h fcntl.h libintl.h limits.h \
                  stdlib.h string.h sys/file.h sys/ioctl.h sys/mount.h \
                  sys/param.h sys/socket.h sys/time.h sys/vfs.h \
                  syslog.h unistd.h com_err.h et/com_err.h \
-                 ifaddrs.h])
+                 ifaddrs.h nfs-plugin.h])
 
 dnl *************************************************************
 dnl Checks for typedefs, structures, and compiler characteristics
diff --git a/utils/mountd/Makefile.am b/utils/mountd/Makefile.am
index eba81fc..5a2d1b6 100644
--- a/utils/mountd/Makefile.am
+++ b/utils/mountd/Makefile.am
@@ -12,7 +12,7 @@ mountd_SOURCES = mountd.c mount_dispatch.c auth.c rmtab.c cache.c \
 mountd_LDADD = ../../support/export/libexport.a \
 	       ../../support/nfs/libnfs.a \
 	       ../../support/misc/libmisc.a \
-	       $(LIBBSD) $(LIBWRAP) $(LIBNSL) $(LIBBLKID)
+	       $(LIBBSD) $(LIBWRAP) $(LIBNSL) $(LIBBLKID) $(LIBDL)
 mountd_CPPFLAGS = $(AM_CPPFLAGS) $(CPPFLAGS) \
 		  -I$(top_builddir)/support/include \
 		  -I$(top_srcdir)/support/export
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d2ae456..d4f04ab 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -802,6 +802,229 @@ lookup_export(char *dom, char *path, struct addrinfo *ai)
 	return found;
 }
 
+#ifdef HAVE_NFS_PLUGIN_H
+#include <dlfcn.h>
+#include <nfs-plugin.h>
+
+/*
+ * Walk through a set of FS locations and build a set of export options.
+ * Returns true if all went to plan; otherwise, false.
+ */
+static _Bool
+locations_to_options(struct jp_ops *ops, nfs_fsloc_set_t locations,
+		char *options, size_t remaining, int *ttl)
+{
+	char *server, *last_path, *rootpath, *ptr;
+	_Bool seen = false;
+
+	last_path = NULL;
+	rootpath = NULL;
+	server = NULL;
+	ptr = options;
+	*ttl = 0;
+
+	for (;;) {
+		enum jp_status status;
+		int len;
+
+		status = ops->jp_get_next_location(locations, &server,
+							&rootpath, ttl);
+		if (status == JP_EMPTY)
+			break;
+		if (status != JP_OK) {
+			xlog(D_GENERAL, "%s: failed to parse location: %s",
+				__func__, ops->jp_error(status));
+			goto out_false;
+		}
+		xlog(D_GENERAL, "%s: Location: %s:%s",
+			__func__, server, rootpath);
+
+		if (last_path && strcmp(rootpath, last_path) == 0) {
+			len = snprintf(ptr, remaining, "+%s", server);
+			if (len < 0) {
+				xlog(D_GENERAL, "%s: snprintf: %m", __func__);
+				goto out_false;
+			}
+			if ((size_t)len > remaining) {
+				xlog(D_GENERAL, "%s: options buffer overflow", __func__);
+				goto out_false;
+			}
+			remaining -= (size_t)len;
+			ptr += len;
+		} else {
+			if (last_path == NULL)
+				len = snprintf(ptr, remaining, "refer=%s@%s",
+							rootpath, server);
+			else
+				len = snprintf(ptr, remaining, ":%s@%s",
+							rootpath, server);
+			if (len < 0) {
+				xlog(D_GENERAL, "%s: snprintf: %m", __func__);
+				goto out_false;
+			}
+			if ((size_t)len > remaining) {
+				xlog(D_GENERAL, "%s: options buffer overflow",
+					__func__);
+				goto out_false;
+			}
+			remaining -= (size_t)len;
+			ptr += len;
+			last_path = rootpath;
+		}
+
+		seen = true;
+		free(rootpath);
+		free(server);
+	}
+
+	xlog(D_CALL, "%s: options='%s', ttl=%d",
+		__func__, options, *ttl);
+	return seen;
+
+out_false:
+	free(rootpath);
+	free(server);
+	return false;
+}
+
+/*
+ * Walk through the set of FS locations and build an exportent.
+ * Returns pointer to an exportent if "junction" refers to a junction.
+ *
+ * Returned exportent points to static memory.
+ */
+static struct exportent *do_locations_to_export(struct jp_ops *ops,
+		nfs_fsloc_set_t locations, const char *junction,
+		char *options, size_t options_len)
+{
+	struct exportent *exp;
+	int ttl;
+
+	if (!locations_to_options(ops, locations, options, options_len, &ttl))
+		return NULL;
+
+	exp = mkexportent("*", (char *)junction, options);
+	if (exp == NULL) {
+		xlog(L_ERROR, "%s: Failed to construct exportent", __func__);
+		return NULL;
+	}
+
+	exp->e_uuid = NULL;
+	exp->e_ttl = ttl;
+	return exp;
+}
+
+/*
+ * Convert set of FS locations to an exportent.  Returns pointer to
+ * an exportent if "junction" refers to a junction.
+ *
+ * Returned exportent points to static memory.
+ */
+static struct exportent *locations_to_export(struct jp_ops *ops,
+		nfs_fsloc_set_t locations, const char *junction)
+{
+	struct exportent *exp;
+	char *options;
+
+	options = malloc(BUFSIZ);
+	if (options == NULL) {
+		xlog(D_GENERAL, "%s: failed to allocate options buffer",
+			__func__);
+		return NULL;
+	}
+	options[0] = '\0';
+
+	exp = do_locations_to_export(ops, locations, junction,
+						options, BUFSIZ);
+
+	free(options);
+	return exp;
+}
+
+/*
+ * Retrieve locations information in "junction" and dump it to the
+ * kernel.  Returns pointer to an exportent if "junction" refers
+ * to a junction.
+ *
+ * Returned exportent points to static memory.
+ */
+static struct exportent *invoke_junction_ops(void *handle,
+		const char *junction)
+{
+	nfs_fsloc_set_t locations;
+	struct exportent *exp;
+	enum jp_status status;
+	struct jp_ops *ops;
+	char *error;
+
+	ops = (struct jp_ops *)dlsym(handle, "nfs_junction_ops");
+	error = dlerror();
+	if (error != NULL) {
+		xlog(D_GENERAL, "%s: dlsym(jp_junction_ops): %s",
+			__func__, error);
+		return NULL;
+	}
+	if (ops->jp_api_version != JP_API_VERSION) {
+		xlog(D_GENERAL, "%s: unrecognized junction API version: %u",
+			__func__, ops->jp_api_version);
+		return NULL;
+	}
+
+	status = ops->jp_init(false);
+	if (status != JP_OK) {
+		xlog(D_GENERAL, "%s: failed to resolve %s: %s",
+			__func__, junction, ops->jp_error(status));
+		return NULL;
+	}
+
+	status = ops->jp_get_locations(junction, &locations);
+	if (status != JP_OK) {
+		xlog(D_GENERAL, "%s: failed to resolve %s: %s",
+			__func__, junction, ops->jp_error(status));
+		return NULL;
+	}
+
+	exp = locations_to_export(ops, locations, junction);
+
+	ops->jp_put_locations(locations);
+	ops->jp_done();
+	return exp;
+}
+
+/*
+ * Load the junction plug-in, then try to resolve "pathname".
+ * Returns pointer to an initialized exportent if "junction"
+ * refers to a junction, or NULL if not.
+ *
+ * Returned exportent points to static memory.
+ */
+static struct exportent *lookup_junction(const char *pathname)
+{
+	struct exportent *exp;
+	void *handle;
+
+	handle = dlopen("libnfsjunct.so", RTLD_NOW);
+	if (handle == NULL) {
+		xlog(D_GENERAL, "%s: dlopen: %s", __func__, dlerror());
+		return NULL;
+	}
+	(void)dlerror();	/* Clear any error */
+
+	exp = invoke_junction_ops(handle, pathname);
+
+	/* We could leave it loaded to make junction resolution
+	 * faster next time.  However, if we want to replace the
+	 * library, that would require restarting mountd. */
+	(void)dlclose(handle);
+	return exp;
+}
+#else	/* !HAVE_NFS_PLUGIN_H */
+static inline struct exportent *lookup_junction(const char *UNUSED(pathname))
+{
+	return NULL;
+}
+#endif	/* !HAVE_NFS_PLUGIN_H */
+
 static void nfsd_export(FILE *f)
 {
 	/* requests are:
@@ -854,7 +1077,7 @@ static void nfsd_export(FILE *f)
 			dump_to_cache(f, dom, path, NULL);
 		}
 	} else {
-		dump_to_cache(f, dom, path, NULL);
+		dump_to_cache(f, dom, path, lookup_junction(path));
 	}
  out:
 	xlog(D_CALL, "nfsd_export: found %p path %s", found, path ? path : NULL);

Re: [PATCH 5/5] mountd: Support junction management plug-ins

[Jim Meyering]

Chuck Lever wrote:
> To support FedFS and NFS junctions without introducing additional
> build-time or run-time dependencies on nfs-utils, the community has
> chosen to use a dynamically loadable library to handle junction
> resolution.
...

Hi Chuck,

You may just be following existing practice in nfs-utils (there
are numerous other examples of this off-by-one error[1]), but the
post-snprintf length check must fail when the returned length is
equal to the specified buffer length.

> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index d2ae456..d4f04ab 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -802,6 +802,229 @@ lookup_export(char *dom, char *path, struct addrinfo *ai)
>  	return found;
>  }
>
> +#ifdef HAVE_NFS_PLUGIN_H
> +#include <dlfcn.h>
> +#include <nfs-plugin.h>
> +
> +/*
> + * Walk through a set of FS locations and build a set of export options.
> + * Returns true if all went to plan; otherwise, false.
> + */
> +static _Bool
> +locations_to_options(struct jp_ops *ops, nfs_fsloc_set_t locations,
> +		char *options, size_t remaining, int *ttl)
> +{
> +	char *server, *last_path, *rootpath, *ptr;
> +	_Bool seen = false;
> +
> +	last_path = NULL;
> +	rootpath = NULL;
> +	server = NULL;
> +	ptr = options;
> +	*ttl = 0;
> +
> +	for (;;) {
> +		enum jp_status status;
> +		int len;
> +
> +		status = ops->jp_get_next_location(locations, &server,
> +							&rootpath, ttl);
> +		if (status == JP_EMPTY)
> +			break;
> +		if (status != JP_OK) {
> +			xlog(D_GENERAL, "%s: failed to parse location: %s",
> +				__func__, ops->jp_error(status));
> +			goto out_false;
> +		}
> +		xlog(D_GENERAL, "%s: Location: %s:%s",
> +			__func__, server, rootpath);
> +
> +		if (last_path && strcmp(rootpath, last_path) == 0) {
> +			len = snprintf(ptr, remaining, "+%s", server);
> +			if (len < 0) {
> +				xlog(D_GENERAL, "%s: snprintf: %m", __func__);
> +				goto out_false;
> +			}
> +			if ((size_t)len > remaining) {

s/>/>=/

snprintf returning the specified size indicates that the
result+NUL did not fit in the specified buffer.

> +				xlog(D_GENERAL, "%s: options buffer overflow", __func__);
> +				goto out_false;
> +			}
> +			remaining -= (size_t)len;
> +			ptr += len;
> +		} else {
> +			if (last_path == NULL)
> +				len = snprintf(ptr, remaining, "refer=%s@%s",
> +							rootpath, server);
> +			else
> +				len = snprintf(ptr, remaining, ":%s@%s",
> +							rootpath, server);
> +			if (len < 0) {
> +				xlog(D_GENERAL, "%s: snprintf: %m", __func__);
> +				goto out_false;
> +			}
> +			if ((size_t)len > remaining) {

Same here.

> +				xlog(D_GENERAL, "%s: options buffer overflow",
> +					__func__);
> +				goto out_false;
> +			}
> +			remaining -= (size_t)len;
> +			ptr += len;
> +			last_path = rootpath;
> +		}

[1] Here are other examples of that error:

  $ git grep -A13 snprintf|grep ' > '
  support/nfs/cacheio.c-  if (len > *lp)
  support/nfs/cacheio.c-  if (len > *lp)
  support/nfs/getport.c-  if (len < 0 || (size_t)len > count)
  utils/exportfs/exportfs.c-              if (fname_len > PATH_MAX) {
  utils/gssd/gssd_proc.c- if (nbytes > INFOBUFLEN)

Re: [PATCH 5/5] mountd: Support junction management plug-ins

[Chuck Lever]

On Jan 4, 2012, at 3:08 PM, Jim Meyering wrote:

> Chuck Lever wrote:
>> To support FedFS and NFS junctions without introducing additional
>> build-time or run-time dependencies on nfs-utils, the community has
>> chosen to use a dynamically loadable library to handle junction
>> resolution.
> ...
> 
> Hi Chuck,
> 
> You may just be following existing practice in nfs-utils (there
> are numerous other examples of this off-by-one error[1]), but the
> post-snprintf length check must fail when the returned length is
> equal to the specified buffer length.

I can fix and repost if there are more comments on this patch.  Otherwise, once Steve commits it you might consider fixing all of these at once.

> 
>> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
>> index d2ae456..d4f04ab 100644
>> --- a/utils/mountd/cache.c
>> +++ b/utils/mountd/cache.c
>> @@ -802,6 +802,229 @@ lookup_export(char *dom, char *path, struct addrinfo *ai)
>> 	return found;
>> }
>> 
>> +#ifdef HAVE_NFS_PLUGIN_H
>> +#include <dlfcn.h>
>> +#include <nfs-plugin.h>
>> +
>> +/*
>> + * Walk through a set of FS locations and build a set of export options.
>> + * Returns true if all went to plan; otherwise, false.
>> + */
>> +static _Bool
>> +locations_to_options(struct jp_ops *ops, nfs_fsloc_set_t locations,
>> +		char *options, size_t remaining, int *ttl)
>> +{
>> +	char *server, *last_path, *rootpath, *ptr;
>> +	_Bool seen = false;
>> +
>> +	last_path = NULL;
>> +	rootpath = NULL;
>> +	server = NULL;
>> +	ptr = options;
>> +	*ttl = 0;
>> +
>> +	for (;;) {
>> +		enum jp_status status;
>> +		int len;
>> +
>> +		status = ops->jp_get_next_location(locations, &server,
>> +							&rootpath, ttl);
>> +		if (status == JP_EMPTY)
>> +			break;
>> +		if (status != JP_OK) {
>> +			xlog(D_GENERAL, "%s: failed to parse location: %s",
>> +				__func__, ops->jp_error(status));
>> +			goto out_false;
>> +		}
>> +		xlog(D_GENERAL, "%s: Location: %s:%s",
>> +			__func__, server, rootpath);
>> +
>> +		if (last_path && strcmp(rootpath, last_path) == 0) {
>> +			len = snprintf(ptr, remaining, "+%s", server);
>> +			if (len < 0) {
>> +				xlog(D_GENERAL, "%s: snprintf: %m", __func__);
>> +				goto out_false;
>> +			}
>> +			if ((size_t)len > remaining) {
> 
> s/>/>=/
> 
> snprintf returning the specified size indicates that the
> result+NUL did not fit in the specified buffer.
> 
>> +				xlog(D_GENERAL, "%s: options buffer overflow", __func__);
>> +				goto out_false;
>> +			}
>> +			remaining -= (size_t)len;
>> +			ptr += len;
>> +		} else {
>> +			if (last_path == NULL)
>> +				len = snprintf(ptr, remaining, "refer=%s@%s",
>> +							rootpath, server);
>> +			else
>> +				len = snprintf(ptr, remaining, ":%s@%s",
>> +							rootpath, server);
>> +			if (len < 0) {
>> +				xlog(D_GENERAL, "%s: snprintf: %m", __func__);
>> +				goto out_false;
>> +			}
>> +			if ((size_t)len > remaining) {
> 
> Same here.
> 
>> +				xlog(D_GENERAL, "%s: options buffer overflow",
>> +					__func__);
>> +				goto out_false;
>> +			}
>> +			remaining -= (size_t)len;
>> +			ptr += len;
>> +			last_path = rootpath;
>> +		}
> 
> [1] Here are other examples of that error:
> 
>  $ git grep -A13 snprintf|grep ' > '
>  support/nfs/cacheio.c-  if (len > *lp)
>  support/nfs/cacheio.c-  if (len > *lp)
>  support/nfs/getport.c-  if (len < 0 || (size_t)len > count)
>  utils/exportfs/exportfs.c-              if (fname_len > PATH_MAX) {
>  utils/gssd/gssd_proc.c- if (nbytes > INFOBUFLEN)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 0/5] Bug fixes and NFS junction support

[Steve Dickson]

On 01/04/2012 02:56 PM, Chuck Lever wrote:
> Four minor bug fix patches, for what they're worth, then...
> 
> The fifth patch adds support in mountd for loading a plug-in to
> resolve NFS junctions.  The plug-in will be provided by fedfs-utils.
> Source code will be posted very soon for review.
> 
> We'd like to see this in Fedora 17, if convenient.  Meantime, let's
> review what I've got.
> 
> ---
> 
> Chuck Lever (5):
>       mountd: Support junction management plug-ins
>       mountd: remove newline from xlog() format specifier strings
>       mountd: Plug v4root memory leak
>       configure.ac: Don't check for AI_ADDRCONFIG
>       configure.ac: Clean up help string for --with-statdpath
All 5 patches have been committed...

I went ahead and fixed the snprintf length issue that was
brought up with the 5th patch... I neglected of fix the 
other issues that were pointed out... I will leave them
on my todo list but as always patches are welcome! 8-)

steved.

> 
> 
>  aclocal/ipv6.m4          |   11 --
>  configure.ac             |   10 +-
>  utils/mountd/Makefile.am |    2 
>  utils/mountd/cache.c     |  225 ++++++++++++++++++++++++++++++++++++++++++++++
>  utils/mountd/fsloc.c     |   10 +-
>  utils/mountd/v4root.c    |    2 
>  6 files changed, 237 insertions(+), 23 deletions(-)
>