[PATCH 0/8] Fixes for nfs-utils-1.2.next

[PATCH 0/8] Fixes for nfs-utils-1.2.next

[Chuck Lever]

For review: eight patches that address some minor issues in and around
sm-notify.

---

Chuck Lever (8):
      sm-notify: sm-notify leaves monitor records in sm.bak
      sm-notify: Avoid extra rpcbind queries
      sm-notify: Use correct retransmit timeout when sending a fresh RPC
      sm-notify: Refactor insert_host() and recv_rpcbind_reply()
      nfsumount: Squelch compiler warning
      configure.ac: Add --with-statd-extension configure option
      configure.ac: Clean up help string for --enable-mount
      configure.ac: Fix help string for --with-statedir= option


 configure.ac            |   13 +++++++-
 support/nsm/file.c      |    9 +----
 utils/mount/nfsumount.c |    3 +-
 utils/statd/sm-notify.c |   77 +++++++++++++++++++++++++++++------------------
 4 files changed, 62 insertions(+), 40 deletions(-)

-- 
Chuck Lever

[PATCH 1/8] configure.ac: Fix help string for --with-statedir= option

[Chuck Lever]

The help string for --with-statedir attempts to show "/var/lib/nfs" in
square brackets, but they don't appear on my system (Fedora 13).  Use
the AC_HELP_STRING macro to display the help string properly, like all
the other "with" and "enable" options specified in our configure.ac.

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

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

diff --git a/configure.ac b/configure.ac
index c9fb27b..9e55a89 100644
--- a/configure.ac
+++ b/configure.ac
@@ -18,7 +18,8 @@ AC_ARG_WITH(release,
 	RELEASE=1)
 	AC_SUBST(RELEASE)
 AC_ARG_WITH(statedir,
-	[  --with-statedir=/foo    use state dir /foo [/var/lib/nfs]],
+	[AC_HELP_STRING([--with-statedir=/foo],
+			[use state dir /foo @<:@default=/var/lib/nfs@:>@])],
 	statedir=$withval,
 	statedir=/var/lib/nfs)
 	AC_SUBST(statedir)

[PATCH 2/8] configure.ac: Clean up help string for --enable-mount

[Chuck Lever]

Clean up a nit: The vim colorizer stumbles on the single quote in
"don't", so replace it with "do not".

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

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

diff --git a/configure.ac b/configure.ac
index 9e55a89..461a96a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -129,7 +129,7 @@ AC_ARG_ENABLE(uuid,
 	choose_blkid=default)
 AC_ARG_ENABLE(mount,
 	[AC_HELP_STRING([--enable-mount],
-			[Create mount.nfs and don't use the util-linux mount(8) functionality. @<:@default=yes@:>@])],
+			[Create mount.nfs and do not use the util-linux mount(8) functionality. @<:@default=yes@:>@])],
 	enable_mount=$enableval,
 	enable_mount=yes)
 	AM_CONDITIONAL(CONFIG_MOUNT, [test "$enable_mount" = "yes"])

[PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Chuck Lever]

Currently some distributions patch nfs-utils to put NSM state in a
subdirectory of /var/lib/nfs.  Make this a configure option instead.

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

 configure.ac       |    8 ++++++++
 support/nsm/file.c |    9 +--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index 461a96a..ba704e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
 	statedir=$withval,
 	statedir=/var/lib/nfs)
 	AC_SUBST(statedir)
+AC_ARG_WITH(statd-extension,
+	[AC_HELP_STRING([--with-statd-extension=foo],
+                        [Put NSM state in subdir foo of statedir])],
+	statdext=$withval,
+	statdext="")
+	AC_SUBST(statdext)
+	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
+			   [This defines the statedir subdirectory containing NSM state files.])
 AC_ARG_WITH(statduser,
 	[AC_HELP_STRING([--with-statduser=rpcuser],
                         [statd to run under @<:@rpcuser or nobody@:>@]
diff --git a/support/nsm/file.c b/support/nsm/file.c
index a12c753..b4a5af1 100644
--- a/support/nsm/file.c
+++ b/support/nsm/file.c
@@ -93,14 +93,7 @@
 #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
 
 #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
-
-/*
- * Some distributions place statd's files in a subdirectory
- */
-#define NSM_PATH_EXTENSION
-/* #define NSM_PATH_EXTENSION	"/statd" */
-
-#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
+#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
 
 static char nsm_base_dirname[PATH_MAX] = NSM_DEFAULT_STATEDIR;
 

[PATCH 4/8] nfsumount: Squelch compiler warning

[Chuck Lever]

nfsumount.c: In function ‘nfs_umount_is_vers4’:
nfsumount.c:164: warning: conversion to ‘int’ from ‘size_t’ may alter its value
nfsumount.c:173: warning: conversion to ‘size_t’ from ‘int’ may change the sign of the result

Introduced by commit 3564ebbf.

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

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

diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index b846564..3538d88 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -161,7 +161,8 @@ static int nfs_umount_is_vers4(const struct mntentchn *mc)
 		goto not_found;
 
 	do {
-		int nlen = strlen(pmc->m.mnt_fsname);
+		size_t nlen = strlen(pmc->m.mnt_fsname);
+
 		/*
 		 * It's possible the mount location string in /proc/mounts
 		 * ends with a '/'. In this case, if the entry came from

[PATCH 5/8] sm-notify: Refactor insert_host() and recv_rpcbind_reply()

[Chuck Lever]

Clean up: refactor the logic in recv_rpcbind_reply() that re-schedules
an nsm_host into a separate helper function

Adjust debugging messages so it's always apparent when an nsm_host is
rescheduled.

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

 utils/statd/sm-notify.c |   42 ++++++++++++++++++++++++++----------------
 1 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 2421f8b..46447e8 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -252,7 +252,6 @@ smn_get_host(const char *hostname,
 		return 0;
 
 	insert_host(host);
-	xlog(D_GENERAL, "Added host %s to notify list", hostname);
 	return 1;
 }
 
@@ -688,6 +687,25 @@ notify_host(int sock, struct nsm_host *host)
 	return 0;
 }
 
+static void
+smn_defer(struct nsm_host *host)
+{
+	host->xid = 0;
+	host->send_next = time(NULL) + NSM_MAX_TIMEOUT;
+	host->timeout = NSM_MAX_TIMEOUT;
+	insert_host(host);
+}
+
+static void
+smn_schedule(struct nsm_host *host)
+{
+	host->xid = 0;
+	host->send_next = time(NULL);
+	if (host->timeout >= NSM_MAX_TIMEOUT / 4)
+		host->timeout = NSM_MAX_TIMEOUT / 4;
+	insert_host(host);
+}
+
 /*
  * Extract the returned port number and set up the SM_NOTIFY call.
  */
@@ -696,21 +714,16 @@ recv_rpcbind_reply(struct sockaddr *sap, struct nsm_host *host, XDR *xdr)
 {
 	uint16_t port = nsm_recv_rpcbind(sap->sa_family, xdr);
 
-	host->send_next = time(NULL);
-	host->xid = 0;
-
 	if (port == 0) {
 		/* No binding for statd... */
 		xlog(D_GENERAL, "No statd on host %s", host->name);
-		host->timeout = NSM_MAX_TIMEOUT;
-		host->send_next += NSM_MAX_TIMEOUT;
+		smn_defer(host);
 	} else {
+		xlog(D_GENERAL, "Processing rpcbind reply for %s (port %u)",
+			host->name, port);
 		nfs_set_port(sap, port);
-		if (host->timeout >= NSM_MAX_TIMEOUT / 4)
-			host->timeout = NSM_MAX_TIMEOUT / 4;
+		smn_schedule(host);
 	}
-
-	insert_host(host);
 }
 
 /*
@@ -727,11 +740,7 @@ recv_notify_reply(struct nsm_host *host)
 
 	if (dot != NULL) {
 		*dot = '\0';
-		host->send_next = time(NULL);
-		host->xid = 0;
-		if (host->timeout >= NSM_MAX_TIMEOUT / 4)
-			host->timeout = NSM_MAX_TIMEOUT / 4;
-		insert_host(host);
+		smn_schedule(host);
 	} else {
 		xlog(D_GENERAL, "Host %s notified successfully", host->name);
 		smn_forget_host(host);
@@ -780,7 +789,7 @@ out:
 }
 
 /*
- * Insert host into sorted list
+ * Insert host into notification list, sorted by next send time
  */
 static void
 insert_host(struct nsm_host *host)
@@ -805,6 +814,7 @@ insert_host(struct nsm_host *host)
 
 	host->next = *where;
 	*where = host;
+	xlog(D_GENERAL, "Added host %s to notify list", host->name);
 }
 
 /*

[PATCH 6/8] sm-notify: Use correct retransmit timeout when sending a fresh RPC

[Chuck Lever]

An RPC retransmit timeout should start out the same for each new RPC
request.  Don't increase the retransmit timeout after receiving the
reply to the rpcbind query.

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

 utils/statd/sm-notify.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 46447e8..aa202d3 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -701,8 +701,7 @@ smn_schedule(struct nsm_host *host)
 {
 	host->xid = 0;
 	host->send_next = time(NULL);
-	if (host->timeout >= NSM_MAX_TIMEOUT / 4)
-		host->timeout = NSM_MAX_TIMEOUT / 4;
+	host->timeout = NSM_TIMEOUT;
 	insert_host(host);
 }
 

[PATCH 7/8] sm-notify: Avoid extra rpcbind queries

[Chuck Lever]

The logic in notify_host() watches the host->retries counter to see if
progress is not being made.  If progress stalls, notify_host() tries
another IP address.  This means sm-notify will generate a fresh
rpcbind query.

After an RPC succeeds, be sure to reset host->retries so sm-notify
doesn't start walking down the host's addrinfo list when we _are_
making progress.  In the common case, if the host responds, we avoid
extra rpcbind queries and send all requests for the host to the same
IP address.

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

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

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index aa202d3..690ec2a 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -699,6 +699,7 @@ smn_defer(struct nsm_host *host)
 static void
 smn_schedule(struct nsm_host *host)
 {
+	host->retries = 0;
 	host->xid = 0;
 	host->send_next = time(NULL);
 	host->timeout = NSM_TIMEOUT;

[PATCH 8/8] sm-notify: sm-notify leaves monitor records in sm.bak

[Chuck Lever]

sm-notify fails to remove monitor records from sm.bak when it has
finally notified a host.  This is because of a recent change to send
two SM_NOTIFY requests for each monitored peer: one with the local
host's FQDN, and one with an unqualified version of same.  This was
commit baa41b2c: "sm-notify: Send fully-qualified and unqualified
mon_names" (March 19, 2010).

Because of the March 2010 commit, sm-notify modifies the "my_name"
string during notification, but then uses this modified string to try
to find the monitor record to remove.  Of course the search for the
record fails.  So a persistent monitor record is left in sm.bak.

Aside from leaving trash around, this causes the same hosts to be
notified after every reboot, even if they successfully responded to
the previous SM_NOTIFY and they had no contact with us during the last
boot.

I also noticed that the trick of truncating the argument of SM_NOTIFY
doesn't work at all if a substitute "my_name" was specified via the "-v"
command line option.  This patch attempts to address that as well.

sm-notify should preserve the original my_name string so that
nsm_delete_host() can find the correct monitor record to delete.  Also
add some degree of protection to the mon_name and my_name strings in
each nsm_host record to prevent a future change from breaking this
dependency.

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

 utils/statd/sm-notify.c |   35 ++++++++++++++++++++++-------------
 1 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 690ec2a..a3290aa 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -45,8 +45,9 @@
 struct nsm_host {
 	struct nsm_host *	next;
 	char *			name;
-	char *			mon_name;
-	char *			my_name;
+	const char *		mon_name;
+	const char *		my_name;
+	char *			notify_arg;
 	struct addrinfo		*ai;
 	time_t			last_used;
 	time_t			send_next;
@@ -199,14 +200,23 @@ smn_alloc_host(const char *hostname, const char *mon_name,
 	if (host == NULL)
 		goto out_nomem;
 
+	/*
+	 * mon_name and my_name are preserved so sm-notify can
+	 * find the right monitor record to remove when it is
+	 * done processing this host.
+	 */
 	host->name = strdup(hostname);
-	host->mon_name = strdup(mon_name);
-	host->my_name = strdup(my_name);
+	host->mon_name = (const char *)strdup(mon_name);
+	host->my_name = (const char *)strdup(my_name);
+	host->notify_arg = strdup(opt_srcaddr != NULL ?
+					nsm_hostname : my_name);
 	if (host->name == NULL ||
 	    host->mon_name == NULL ||
-	    host->my_name == NULL) {
-		free(host->my_name);
-		free(host->mon_name);
+	    host->my_name == NULL ||
+	    host->notify_arg == NULL) {
+		free(host->notify_arg);
+		free((void *)host->my_name);
+		free((void *)host->mon_name);
 		free(host->name);
 		free(host);
 		goto out_nomem;
@@ -230,8 +240,9 @@ static void smn_forget_host(struct nsm_host *host)
 
 	nsm_delete_notified_host(host->name, host->mon_name, host->my_name);
 
-	free(host->my_name);
-	free(host->mon_name);
+	free(host->notify_arg);
+	free((void *)host->my_name);
+	free((void *)host->mon_name);
 	free(host->name);
 	if (host->ai)
 		freeaddrinfo(host->ai);
@@ -635,8 +646,6 @@ notify(const int sock)
 static int
 notify_host(int sock, struct nsm_host *host)
 {
-	const char *my_name = (opt_srcaddr != NULL ?
-					nsm_hostname : host->my_name);
 	struct sockaddr *sap;
 	socklen_t salen;
 
@@ -682,7 +691,7 @@ notify_host(int sock, struct nsm_host *host)
 		host->xid = nsm_xmit_rpcbind(sock, sap, SM_PROG, SM_VERS);
 	else
 		host->xid = nsm_xmit_notify(sock, sap, salen,
-					SM_PROG, my_name, nsm_state);
+					SM_PROG, host->notify_arg, nsm_state);
 
 	return 0;
 }
@@ -736,7 +745,7 @@ recv_rpcbind_reply(struct sockaddr *sap, struct nsm_host *host, XDR *xdr)
 static void
 recv_notify_reply(struct nsm_host *host)
 {
-	char *dot = strchr(host->my_name, '.');
+	char *dot = strchr(host->notify_arg, '.');
 
 	if (dot != NULL) {
 		*dot = '\0';

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Steve Dickson]

On 09/12/2011 06:06 PM, Chuck Lever wrote:
> Currently some distributions patch nfs-utils to put NSM state in a
> subdirectory of /var/lib/nfs.  Make this a configure option instead.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  configure.ac       |    8 ++++++++
>  support/nsm/file.c |    9 +--------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 461a96a..ba704e2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
>  	statedir=$withval,
>  	statedir=/var/lib/nfs)
>  	AC_SUBST(statedir)
> +AC_ARG_WITH(statd-extension,
> +	[AC_HELP_STRING([--with-statd-extension=foo],
> +                        [Put NSM state in subdir foo of statedir])],
> +	statdext=$withval,
> +	statdext="")
> +	AC_SUBST(statdext)
> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
> +			   [This defines the statedir subdirectory containing NSM state files.])
>  AC_ARG_WITH(statduser,
>  	[AC_HELP_STRING([--with-statduser=rpcuser],
>                          [statd to run under @<:@rpcuser or nobody@:>@]
> diff --git a/support/nsm/file.c b/support/nsm/file.c
> index a12c753..b4a5af1 100644
> --- a/support/nsm/file.c
> +++ b/support/nsm/file.c
> @@ -93,14 +93,7 @@
>  #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
>  
>  #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
> -
> -/*
> - * Some distributions place statd's files in a subdirectory
> - */
> -#define NSM_PATH_EXTENSION
> -/* #define NSM_PATH_EXTENSION	"/statd" */
> -
> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
Do we really need the NSM_PATH_EXTENSION define? Would it be more
straightforward to just have NFS_STATEDIR. Simplifying the code to:

#ifndef NFS_STATEDIR 
#define NFS_STATEDIR "/var/lib/nfs"
#endif

#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR

If there is no need for the extra NSM_PATH_EXTENSION define then 
we really don't want to create a configuration option for it.. IMHO..

steved.

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Jeff Layton]

On Sat, 17 Sep 2011 08:28:45 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 09/12/2011 06:06 PM, Chuck Lever wrote:
> > Currently some distributions patch nfs-utils to put NSM state in a
> > subdirectory of /var/lib/nfs.  Make this a configure option instead.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> > 
> >  configure.ac       |    8 ++++++++
> >  support/nsm/file.c |    9 +--------
> >  2 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 461a96a..ba704e2 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
> >  	statedir=$withval,
> >  	statedir=/var/lib/nfs)
> >  	AC_SUBST(statedir)
> > +AC_ARG_WITH(statd-extension,
> > +	[AC_HELP_STRING([--with-statd-extension=foo],
> > +                        [Put NSM state in subdir foo of statedir])],
> > +	statdext=$withval,
> > +	statdext="")
> > +	AC_SUBST(statdext)
> > +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
> > +			   [This defines the statedir subdirectory containing NSM state files.])
> >  AC_ARG_WITH(statduser,
> >  	[AC_HELP_STRING([--with-statduser=rpcuser],
> >                          [statd to run under @<:@rpcuser or nobody@:>@]
> > diff --git a/support/nsm/file.c b/support/nsm/file.c
> > index a12c753..b4a5af1 100644
> > --- a/support/nsm/file.c
> > +++ b/support/nsm/file.c
> > @@ -93,14 +93,7 @@
> >  #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
> >  
> >  #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
> > -
> > -/*
> > - * Some distributions place statd's files in a subdirectory
> > - */
> > -#define NSM_PATH_EXTENSION
> > -/* #define NSM_PATH_EXTENSION	"/statd" */
> > -
> > -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
> > +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
> Do we really need the NSM_PATH_EXTENSION define? Would it be more
> straightforward to just have NFS_STATEDIR. Simplifying the code to:
> 
> #ifndef NFS_STATEDIR 
> #define NFS_STATEDIR "/var/lib/nfs"
> #endif
> 
> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
> 
> If there is no need for the extra NSM_PATH_EXTENSION define then 
> we really don't want to create a configuration option for it.. IMHO..
> 


IIRC, the statd directory is not standard between distributions. Some
(like Fedora) put this dir in /var/lib/nfs/statd, and some just keep all
of that in /var/lib/nfs.

The main reason for not making the NSM statedir be /var/lib/nfs is
that statd defaults to running as the user that owns the statedir. We
don't really want /var/lib/nfs owned by rpcuser since it contains other
things that it shouldn't have access to if statd were compromised.

I think the idea here is to push this patch to mainline so we can stop
carrying nfs-utils-1.2.2-statdpath.patch in the Fedora repo and turn it
into an option that distros can use to put this in the location they
prefer.

If we do what you're suggesting above, we'll need a transition scheme
for Fedora and RHEL, and a way to deal with making statd run as the
proper user. I don't think we really want to go to that much effort for
statd...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Steve Dickson]

On 09/18/2011 07:49 AM, Jeff Layton wrote:
> On Sat, 17 Sep 2011 08:28:45 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 09/12/2011 06:06 PM, Chuck Lever wrote:
>>> Currently some distributions patch nfs-utils to put NSM state in a
>>> subdirectory of /var/lib/nfs.  Make this a configure option instead.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>>  configure.ac       |    8 ++++++++
>>>  support/nsm/file.c |    9 +--------
>>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 461a96a..ba704e2 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
>>>  	statedir=$withval,
>>>  	statedir=/var/lib/nfs)
>>>  	AC_SUBST(statedir)
>>> +AC_ARG_WITH(statd-extension,
>>> +	[AC_HELP_STRING([--with-statd-extension=foo],
>>> +                        [Put NSM state in subdir foo of statedir])],
>>> +	statdext=$withval,
>>> +	statdext="")
>>> +	AC_SUBST(statdext)
>>> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
>>> +			   [This defines the statedir subdirectory containing NSM state files.])
>>>  AC_ARG_WITH(statduser,
>>>  	[AC_HELP_STRING([--with-statduser=rpcuser],
>>>                          [statd to run under @<:@rpcuser or nobody@:>@]
>>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>>> index a12c753..b4a5af1 100644
>>> --- a/support/nsm/file.c
>>> +++ b/support/nsm/file.c
>>> @@ -93,14 +93,7 @@
>>>  #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
>>>  
>>>  #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>>> -
>>> -/*
>>> - * Some distributions place statd's files in a subdirectory
>>> - */
>>> -#define NSM_PATH_EXTENSION
>>> -/* #define NSM_PATH_EXTENSION	"/statd" */
>>> -
>>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
>>> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
>> Do we really need the NSM_PATH_EXTENSION define? Would it be more
>> straightforward to just have NFS_STATEDIR. Simplifying the code to:
>>
>> #ifndef NFS_STATEDIR 
>> #define NFS_STATEDIR "/var/lib/nfs"
>> #endif
>>
>> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
>>
>> If there is no need for the extra NSM_PATH_EXTENSION define then 
>> we really don't want to create a configuration option for it.. IMHO..
>>
> 
> 
> IIRC, the statd directory is not standard between distributions. Some
> (like Fedora) put this dir in /var/lib/nfs/statd, and some just keep all
> of that in /var/lib/nfs.
> 
> The main reason for not making the NSM statedir be /var/lib/nfs is
> that statd defaults to running as the user that owns the statedir. We
> don't really want /var/lib/nfs owned by rpcuser since it contains other
> things that it shouldn't have access to if statd were compromised.
> 
> I think the idea here is to push this patch to mainline so we can stop
> carrying nfs-utils-1.2.2-statdpath.patch in the Fedora repo and turn it
> into an option that distros can use to put this in the location they
> prefer.
Ah... I did forget about that statdpath.patch, which basically
replaces the NSM_PATH_EXTENSION with a new NSM_STATD_PATH.

NSM_PATH_EXTENSION has been defined as NULL since Jan
of 2010 so it not be used by anybody. S would it be possible
to use the statdpath.patch patch instead, which would
mean no disto would have to change? ;-)

steved.

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Steve Dickson]

On 09/18/2011 07:49 AM, Jeff Layton wrote:
> On Sat, 17 Sep 2011 08:28:45 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>>
>>
>> On 09/12/2011 06:06 PM, Chuck Lever wrote:
>>> Currently some distributions patch nfs-utils to put NSM state in a
>>> subdirectory of /var/lib/nfs.  Make this a configure option instead.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>>  configure.ac       |    8 ++++++++
>>>  support/nsm/file.c |    9 +--------
>>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 461a96a..ba704e2 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
>>>  	statedir=$withval,
>>>  	statedir=/var/lib/nfs)
>>>  	AC_SUBST(statedir)
>>> +AC_ARG_WITH(statd-extension,
>>> +	[AC_HELP_STRING([--with-statd-extension=foo],
>>> +                        [Put NSM state in subdir foo of statedir])],
>>> +	statdext=$withval,
>>> +	statdext="")
>>> +	AC_SUBST(statdext)
>>> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
>>> +			   [This defines the statedir subdirectory containing NSM state files.])
>>>  AC_ARG_WITH(statduser,
>>>  	[AC_HELP_STRING([--with-statduser=rpcuser],
>>>                          [statd to run under @<:@rpcuser or nobody@:>@]
>>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>>> index a12c753..b4a5af1 100644
>>> --- a/support/nsm/file.c
>>> +++ b/support/nsm/file.c
>>> @@ -93,14 +93,7 @@
>>>  #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
>>>  
>>>  #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>>> -
>>> -/*
>>> - * Some distributions place statd's files in a subdirectory
>>> - */
>>> -#define NSM_PATH_EXTENSION
>>> -/* #define NSM_PATH_EXTENSION	"/statd" */
>>> -
>>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
>>> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
>> Do we really need the NSM_PATH_EXTENSION define? Would it be more
>> straightforward to just have NFS_STATEDIR. Simplifying the code to:
>>
>> #ifndef NFS_STATEDIR 
>> #define NFS_STATEDIR "/var/lib/nfs"
>> #endif
>>
>> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
>>
>> If there is no need for the extra NSM_PATH_EXTENSION define then 
>> we really don't want to create a configuration option for it.. IMHO..
>>
> 
> 
> IIRC, the statd directory is not standard between distributions. Some
> (like Fedora) put this dir in /var/lib/nfs/statd, and some just keep all
> of that in /var/lib/nfs.
> 
> The main reason for not making the NSM statedir be /var/lib/nfs is
> that statd defaults to running as the user that owns the statedir. We
> don't really want /var/lib/nfs owned by rpcuser since it contains other
> things that it shouldn't have access to if statd were compromised.
> 
> I think the idea here is to push this patch to mainline so we can stop
> carrying nfs-utils-1.2.2-statdpath.patch in the Fedora repo and turn it
> into an option that distros can use to put this in the location they
> prefer.
After further review... I kinda like the idea of decoupling statd's
state directory from /var/lib/nfs which basically what the 
statdpath.patch 

> 
> If we do what you're suggesting above, we'll need a transition scheme
> for Fedora and RHEL, and a way to deal with making statd run as the
> proper user. I don't think we really want to go to that much effort for
> statd...
> 
Not if we take the following patch... ;-)


What do you guys think of something like:

commit da6eebe9b01ac132185364efe30b4ba54fc4134c
Author: Steve Dickson <steved@redhat.com>
Date:   Mon Sep 19 11:37:36 2011 -0400

    statd: Decouple statd's state directory from the NFS state directory
    
    To allow greater flexibly to where the state for
    statd is written, this patch introduces the NSM_STATD_PATH
    definition that can be defined at compile time with
    the --with-statdpath flag.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/configure.ac b/configure.ac
index 1a28f8a..6558672 100644
--- a/configure.ac
+++ b/configure.ac
@@ -22,6 +22,14 @@ AC_ARG_WITH(statedir,
 	statedir=$withval,
 	statedir=/var/lib/nfs)
 	AC_SUBST(statedir)
+AC_ARG_WITH(statdpath,
+	[AC_HELP_STRING([--with-statdpath=/foo],
+		[Causes statd put it's state file in /foo instead of statedir]
+	)],
+	statdpath=$withval,
+	statdpath=""
+	)
+	AC_SUBST(statdpath)
 AC_ARG_WITH(statduser,
 	[AC_HELP_STRING([--with-statduser=rpcuser],
                         [statd to run under @<:@rpcuser or nobody@:>@]
@@ -386,6 +394,9 @@ dnl *************************************************************
 dnl Export some path names to config.h
 dnl *************************************************************
 AC_DEFINE_UNQUOTED(NFS_STATEDIR, "$statedir", [This defines the location of the NFS state files. Warning: this must match definitions in config.mk!])
+if test "$statdpath" != ""; then
+		AC_DEFINE_UNQUOTED(NSM_STATD_PATH, "$statdpath", [Define this if you what statd file placed in somewhere other than NFS_STATEDIR])
+fi
 
 if test "x$cross_compiling" = "xno"; then
 	CFLAGS_FOR_BUILD=${CFLAGS_FOR_BUILD-"$CFLAGS"}
diff --git a/support/nsm/file.c b/support/nsm/file.c
index a12c753..2427a6c 100644
--- a/support/nsm/file.c
+++ b/support/nsm/file.c
@@ -95,12 +95,13 @@
 #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
 
 /*
- * Some distributions place statd's files in a subdirectory
+ * Allow different places for statd's files
  */
-#define NSM_PATH_EXTENSION
-/* #define NSM_PATH_EXTENSION	"/statd" */
-
-#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
+#ifdef NSM_STATD_PATH
+#define NSM_DEFAULT_STATEDIR		NSM_STATD_PATH
+#else
+#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR 
+#endif
 
 static char nsm_base_dirname[PATH_MAX] = NSM_DEFAULT_STATEDIR;
 

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Jeff Layton]

On Mon, 19 Sep 2011 13:56:17 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 09/18/2011 07:49 AM, Jeff Layton wrote:
> > On Sat, 17 Sep 2011 08:28:45 -0400
> > Steve Dickson <SteveD@redhat.com> wrote:
> > 
> >>
> >>
> >> On 09/12/2011 06:06 PM, Chuck Lever wrote:
> >>> Currently some distributions patch nfs-utils to put NSM state in a
> >>> subdirectory of /var/lib/nfs.  Make this a configure option instead.
> >>>
> >>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>> ---
> >>>
> >>>  configure.ac       |    8 ++++++++
> >>>  support/nsm/file.c |    9 +--------
> >>>  2 files changed, 9 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/configure.ac b/configure.ac
> >>> index 461a96a..ba704e2 100644
> >>> --- a/configure.ac
> >>> +++ b/configure.ac
> >>> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
> >>>  	statedir=$withval,
> >>>  	statedir=/var/lib/nfs)
> >>>  	AC_SUBST(statedir)
> >>> +AC_ARG_WITH(statd-extension,
> >>> +	[AC_HELP_STRING([--with-statd-extension=foo],
> >>> +                        [Put NSM state in subdir foo of statedir])],
> >>> +	statdext=$withval,
> >>> +	statdext="")
> >>> +	AC_SUBST(statdext)
> >>> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
> >>> +			   [This defines the statedir subdirectory containing NSM state files.])
> >>>  AC_ARG_WITH(statduser,
> >>>  	[AC_HELP_STRING([--with-statduser=rpcuser],
> >>>                          [statd to run under @<:@rpcuser or nobody@:>@]
> >>> diff --git a/support/nsm/file.c b/support/nsm/file.c
> >>> index a12c753..b4a5af1 100644
> >>> --- a/support/nsm/file.c
> >>> +++ b/support/nsm/file.c
> >>> @@ -93,14 +93,7 @@
> >>>  #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
> >>>  
> >>>  #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
> >>> -
> >>> -/*
> >>> - * Some distributions place statd's files in a subdirectory
> >>> - */
> >>> -#define NSM_PATH_EXTENSION
> >>> -/* #define NSM_PATH_EXTENSION	"/statd" */
> >>> -
> >>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
> >>> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
> >> Do we really need the NSM_PATH_EXTENSION define? Would it be more
> >> straightforward to just have NFS_STATEDIR. Simplifying the code to:
> >>
> >> #ifndef NFS_STATEDIR 
> >> #define NFS_STATEDIR "/var/lib/nfs"
> >> #endif
> >>
> >> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
> >>
> >> If there is no need for the extra NSM_PATH_EXTENSION define then 
> >> we really don't want to create a configuration option for it.. IMHO..
> >>
> > 
> > 
> > IIRC, the statd directory is not standard between distributions. Some
> > (like Fedora) put this dir in /var/lib/nfs/statd, and some just keep all
> > of that in /var/lib/nfs.
> > 
> > The main reason for not making the NSM statedir be /var/lib/nfs is
> > that statd defaults to running as the user that owns the statedir. We
> > don't really want /var/lib/nfs owned by rpcuser since it contains other
> > things that it shouldn't have access to if statd were compromised.
> > 
> > I think the idea here is to push this patch to mainline so we can stop
> > carrying nfs-utils-1.2.2-statdpath.patch in the Fedora repo and turn it
> > into an option that distros can use to put this in the location they
> > prefer.
> After further review... I kinda like the idea of decoupling statd's
> state directory from /var/lib/nfs which basically what the 
> statdpath.patch 
> 
> > 
> > If we do what you're suggesting above, we'll need a transition scheme
> > for Fedora and RHEL, and a way to deal with making statd run as the
> > proper user. I don't think we really want to go to that much effort for
> > statd...
> > 
> Not if we take the following patch... ;-)
> 
> 
> What do you guys think of something like:
> 
> commit da6eebe9b01ac132185364efe30b4ba54fc4134c
> Author: Steve Dickson <steved@redhat.com>
> Date:   Mon Sep 19 11:37:36 2011 -0400
> 
>     statd: Decouple statd's state directory from the NFS state directory
>     
>     To allow greater flexibly to where the state for
>     statd is written, this patch introduces the NSM_STATD_PATH
>     definition that can be defined at compile time with
>     the --with-statdpath flag.
>     
>     Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/configure.ac b/configure.ac
> index 1a28f8a..6558672 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -22,6 +22,14 @@ AC_ARG_WITH(statedir,
>  	statedir=$withval,
>  	statedir=/var/lib/nfs)
>  	AC_SUBST(statedir)
> +AC_ARG_WITH(statdpath,
> +	[AC_HELP_STRING([--with-statdpath=/foo],
> +		[Causes statd put it's state file in /foo instead of statedir]
> +	)],
> +	statdpath=$withval,
> +	statdpath=""
> +	)
> +	AC_SUBST(statdpath)
>  AC_ARG_WITH(statduser,
>  	[AC_HELP_STRING([--with-statduser=rpcuser],
>                          [statd to run under @<:@rpcuser or nobody@:>@]
> @@ -386,6 +394,9 @@ dnl *************************************************************
>  dnl Export some path names to config.h
>  dnl *************************************************************
>  AC_DEFINE_UNQUOTED(NFS_STATEDIR, "$statedir", [This defines the location of the NFS state files. Warning: this must match definitions in config.mk!])
> +if test "$statdpath" != ""; then
> +		AC_DEFINE_UNQUOTED(NSM_STATD_PATH, "$statdpath", [Define this if you what statd file placed in somewhere other than NFS_STATEDIR])
> +fi
>  
>  if test "x$cross_compiling" = "xno"; then
>  	CFLAGS_FOR_BUILD=${CFLAGS_FOR_BUILD-"$CFLAGS"}
> diff --git a/support/nsm/file.c b/support/nsm/file.c
> index a12c753..2427a6c 100644
> --- a/support/nsm/file.c
> +++ b/support/nsm/file.c
> @@ -95,12 +95,13 @@
>  #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>  
>  /*
> - * Some distributions place statd's files in a subdirectory
> + * Allow different places for statd's files
>   */
> -#define NSM_PATH_EXTENSION
> -/* #define NSM_PATH_EXTENSION	"/statd" */
> -
> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
> +#ifdef NSM_STATD_PATH
> +#define NSM_DEFAULT_STATEDIR		NSM_STATD_PATH
> +#else
> +#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR 
> +#endif
>  
>  static char nsm_base_dirname[PATH_MAX] = NSM_DEFAULT_STATEDIR;
>  

That works for me. I don't much care which approach goes in, but I do
think that the less distro-specific patches we need to carry, the
better.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Chuck Lever]

On Sep 19, 2011, at 1:56 PM, Steve Dickson wrote:

> 
> 
> On 09/18/2011 07:49 AM, Jeff Layton wrote:
>> On Sat, 17 Sep 2011 08:28:45 -0400
>> Steve Dickson <SteveD@redhat.com> wrote:
>> 
>>> 
>>> 
>>> On 09/12/2011 06:06 PM, Chuck Lever wrote:
>>>> Currently some distributions patch nfs-utils to put NSM state in a
>>>> subdirectory of /var/lib/nfs.  Make this a configure option instead.
>>>> 
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> 
>>>> configure.ac       |    8 ++++++++
>>>> support/nsm/file.c |    9 +--------
>>>> 2 files changed, 9 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/configure.ac b/configure.ac
>>>> index 461a96a..ba704e2 100644
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
>>>> 	statedir=$withval,
>>>> 	statedir=/var/lib/nfs)
>>>> 	AC_SUBST(statedir)
>>>> +AC_ARG_WITH(statd-extension,
>>>> +	[AC_HELP_STRING([--with-statd-extension=foo],
>>>> +                        [Put NSM state in subdir foo of statedir])],
>>>> +	statdext=$withval,
>>>> +	statdext="")
>>>> +	AC_SUBST(statdext)
>>>> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
>>>> +			   [This defines the statedir subdirectory containing NSM state files.])
>>>> AC_ARG_WITH(statduser,
>>>> 	[AC_HELP_STRING([--with-statduser=rpcuser],
>>>>                         [statd to run under @<:@rpcuser or nobody@:>@]
>>>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>>>> index a12c753..b4a5af1 100644
>>>> --- a/support/nsm/file.c
>>>> +++ b/support/nsm/file.c
>>>> @@ -93,14 +93,7 @@
>>>> #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
>>>> 
>>>> #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>>>> -
>>>> -/*
>>>> - * Some distributions place statd's files in a subdirectory
>>>> - */
>>>> -#define NSM_PATH_EXTENSION
>>>> -/* #define NSM_PATH_EXTENSION	"/statd" */
>>>> -
>>>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
>>>> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
>>> Do we really need the NSM_PATH_EXTENSION define? Would it be more
>>> straightforward to just have NFS_STATEDIR. Simplifying the code to:
>>> 
>>> #ifndef NFS_STATEDIR 
>>> #define NFS_STATEDIR "/var/lib/nfs"
>>> #endif
>>> 
>>> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
>>> 
>>> If there is no need for the extra NSM_PATH_EXTENSION define then 
>>> we really don't want to create a configuration option for it.. IMHO..
>>> 
>> 
>> 
>> IIRC, the statd directory is not standard between distributions. Some
>> (like Fedora) put this dir in /var/lib/nfs/statd, and some just keep all
>> of that in /var/lib/nfs.
>> 
>> The main reason for not making the NSM statedir be /var/lib/nfs is
>> that statd defaults to running as the user that owns the statedir. We
>> don't really want /var/lib/nfs owned by rpcuser since it contains other
>> things that it shouldn't have access to if statd were compromised.
>> 
>> I think the idea here is to push this patch to mainline so we can stop
>> carrying nfs-utils-1.2.2-statdpath.patch in the Fedora repo and turn it
>> into an option that distros can use to put this in the location they
>> prefer.
> After further review... I kinda like the idea of decoupling statd's
> state directory from /var/lib/nfs which basically what the 
> statdpath.patch 
> 
>> 
>> If we do what you're suggesting above, we'll need a transition scheme
>> for Fedora and RHEL, and a way to deal with making statd run as the
>> proper user. I don't think we really want to go to that much effort for
>> statd...
>> 
> Not if we take the following patch... ;-)
> 
> 
> What do you guys think of something like:
> 
> commit da6eebe9b01ac132185364efe30b4ba54fc4134c
> Author: Steve Dickson <steved@redhat.com>
> Date:   Mon Sep 19 11:37:36 2011 -0400
> 
>    statd: Decouple statd's state directory from the NFS state directory
> 
>    To allow greater flexibly to where the state for

  --> "flexibility"

>    statd is written, this patch introduces the NSM_STATD_PATH
>    definition that can be defined at compile time with
>    the --with-statdpath flag.

I'm not objecting, but why do you think this flexibility is better?  Enumerating a few use cases in the patch description would be helpful.  Is there, for example, a particular distribution that puts this directory outside of /var/lib/nfs?

Basically looks OK, see below for more specific comments.

>    Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/configure.ac b/configure.ac
> index 1a28f8a..6558672 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -22,6 +22,14 @@ AC_ARG_WITH(statedir,
> 	statedir=$withval,
> 	statedir=/var/lib/nfs)
> 	AC_SUBST(statedir)
> +AC_ARG_WITH(statdpath,
> +	[AC_HELP_STRING([--with-statdpath=/foo],
> +		[Causes statd put it's state file in /foo instead of statedir]

Corrected: add the word "to," make the word "file" plural, and remove incorrect apostrophe.

  "Causes statd to put its state files in /foo instead of in statedir."

> +	)],
> +	statdpath=$withval,
> +	statdpath=""
> +	)
> +	AC_SUBST(statdpath)
> AC_ARG_WITH(statduser,
> 	[AC_HELP_STRING([--with-statduser=rpcuser],
>                         [statd to run under @<:@rpcuser or nobody@:>@]
> @@ -386,6 +394,9 @@ dnl *************************************************************
> dnl Export some path names to config.h
> dnl *************************************************************
> AC_DEFINE_UNQUOTED(NFS_STATEDIR, "$statedir", [This defines the location of the NFS state files. Warning: this must match definitions in config.mk!])
> +if test "$statdpath" != ""; then
> +		AC_DEFINE_UNQUOTED(NSM_STATD_PATH, "$statdpath", [Define this if you what statd file placed in somewhere other than NFS_STATEDIR])

"what" --> "want"

though something else may be warranted if you agree with my suggestion below.

> +fi
> 
> if test "x$cross_compiling" = "xno"; then
> 	CFLAGS_FOR_BUILD=${CFLAGS_FOR_BUILD-"$CFLAGS"}
> diff --git a/support/nsm/file.c b/support/nsm/file.c
> index a12c753..2427a6c 100644
> --- a/support/nsm/file.c
> +++ b/support/nsm/file.c
> @@ -95,12 +95,13 @@
> #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
> 
> /*
> - * Some distributions place statd's files in a subdirectory
> + * Allow different places for statd's files
>  */
> -#define NSM_PATH_EXTENSION
> -/* #define NSM_PATH_EXTENSION	"/statd" */
> -
> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
> +#ifdef NSM_STATD_PATH
> +#define NSM_DEFAULT_STATEDIR		NSM_STATD_PATH
> +#else
> +#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR 
> +#endif

If we are now building the whole path in configure.ac, then it would be simpler and cleaner if this #if logic was in configure.ac, not here.  Just strip all this out, and define NSM_DEFAULT_STATEDIR (or whatever) in configure.ac.  In other words, NSM_DEFAULT_STATEDIR (or whatever) will always be defined, its value will always be listed in config.h, and, by default, it will just be a copy of NFS_STATEDIR.

Then there would be one single place (config.h) for other programs in nfs-utils, and for human beings, to look to see where statd is sticking its state files.  I for one would not be happy if, in order to fix a bug, I had to hunt through configure.ac, config.h, and support/nsm/file.c just to figure out exactly what pathname was generated.

> static char nsm_base_dirname[PATH_MAX] = NSM_DEFAULT_STATEDIR;

  ...

If we now have a global definition of the statd state directory pathname, utils/statd/Makefile should use an install hook to fix up the explicit "/var/lib/nfs" references in statd.man and sm-notify.man.  Jeff says he's got some sample code in cifs-utils.  That would finally allow completely removing all the hunks in statdpath.patch.

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

Re: [PATCH 0/8] Fixes for nfs-utils-1.2.next

[Steve Dickson]

On 09/12/2011 06:05 PM, Chuck Lever wrote:
> For review: eight patches that address some minor issues in and around
> sm-notify.
> 
> ---
> 
> Chuck Lever (8):
>       sm-notify: sm-notify leaves monitor records in sm.bak
>       sm-notify: Avoid extra rpcbind queries
>       sm-notify: Use correct retransmit timeout when sending a fresh RPC
>       sm-notify: Refactor insert_host() and recv_rpcbind_reply()
>       nfsumount: Squelch compiler warning
>       configure.ac: Add --with-statd-extension configure option
>       configure.ac: Clean up help string for --enable-mount
>       configure.ac: Fix help string for --with-statedir= option
> 
Committed all of these except for the one adding the 
statd-extension configure option..

steved.

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Steve Dickson]

Hey Chuck,

On 09/19/2011 03:36 PM, Chuck Lever wrote:
> 
> On Sep 19, 2011, at 1:56 PM, Steve Dickson wrote:
> 
>>
>>
>> On 09/18/2011 07:49 AM, Jeff Layton wrote:
>>> On Sat, 17 Sep 2011 08:28:45 -0400
>>> Steve Dickson <SteveD@redhat.com> wrote:
>>>
>>>>
>>>>
>>>> On 09/12/2011 06:06 PM, Chuck Lever wrote:
>>>>> Currently some distributions patch nfs-utils to put NSM state in a
>>>>> subdirectory of /var/lib/nfs.  Make this a configure option instead.
>>>>>
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>
>>>>> configure.ac       |    8 ++++++++
>>>>> support/nsm/file.c |    9 +--------
>>>>> 2 files changed, 9 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/configure.ac b/configure.ac
>>>>> index 461a96a..ba704e2 100644
>>>>> --- a/configure.ac
>>>>> +++ b/configure.ac
>>>>> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
>>>>> 	statedir=$withval,
>>>>> 	statedir=/var/lib/nfs)
>>>>> 	AC_SUBST(statedir)
>>>>> +AC_ARG_WITH(statd-extension,
>>>>> +	[AC_HELP_STRING([--with-statd-extension=foo],
>>>>> +                        [Put NSM state in subdir foo of statedir])],
>>>>> +	statdext=$withval,
>>>>> +	statdext="")
>>>>> +	AC_SUBST(statdext)
>>>>> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
>>>>> +			   [This defines the statedir subdirectory containing NSM state files.])
>>>>> AC_ARG_WITH(statduser,
>>>>> 	[AC_HELP_STRING([--with-statduser=rpcuser],
>>>>>                         [statd to run under @<:@rpcuser or nobody@:>@]
>>>>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>>>>> index a12c753..b4a5af1 100644
>>>>> --- a/support/nsm/file.c
>>>>> +++ b/support/nsm/file.c
>>>>> @@ -93,14 +93,7 @@
>>>>> #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
>>>>>
>>>>> #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>>>>> -
>>>>> -/*
>>>>> - * Some distributions place statd's files in a subdirectory
>>>>> - */
>>>>> -#define NSM_PATH_EXTENSION
>>>>> -/* #define NSM_PATH_EXTENSION	"/statd" */
>>>>> -
>>>>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
>>>>> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
>>>> Do we really need the NSM_PATH_EXTENSION define? Would it be more
>>>> straightforward to just have NFS_STATEDIR. Simplifying the code to:
>>>>
>>>> #ifndef NFS_STATEDIR 
>>>> #define NFS_STATEDIR "/var/lib/nfs"
>>>> #endif
>>>>
>>>> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
>>>>
>>>> If there is no need for the extra NSM_PATH_EXTENSION define then 
>>>> we really don't want to create a configuration option for it.. IMHO..
>>>>
>>>
>>>
>>> IIRC, the statd directory is not standard between distributions. Some
>>> (like Fedora) put this dir in /var/lib/nfs/statd, and some just keep all
>>> of that in /var/lib/nfs.
>>>
>>> The main reason for not making the NSM statedir be /var/lib/nfs is
>>> that statd defaults to running as the user that owns the statedir. We
>>> don't really want /var/lib/nfs owned by rpcuser since it contains other
>>> things that it shouldn't have access to if statd were compromised.
>>>
>>> I think the idea here is to push this patch to mainline so we can stop
>>> carrying nfs-utils-1.2.2-statdpath.patch in the Fedora repo and turn it
>>> into an option that distros can use to put this in the location they
>>> prefer.
>> After further review... I kinda like the idea of decoupling statd's
>> state directory from /var/lib/nfs which basically what the 
>> statdpath.patch 
>>
>>>
>>> If we do what you're suggesting above, we'll need a transition scheme
>>> for Fedora and RHEL, and a way to deal with making statd run as the
>>> proper user. I don't think we really want to go to that much effort for
>>> statd...
>>>
>> Not if we take the following patch... ;-)
>>
>>
>> What do you guys think of something like:
>>
>> commit da6eebe9b01ac132185364efe30b4ba54fc4134c
>> Author: Steve Dickson <steved@redhat.com>
>> Date:   Mon Sep 19 11:37:36 2011 -0400
>>
>>    statd: Decouple statd's state directory from the NFS state directory
>>
>>    To allow greater flexibly to where the state for
> 
>   --> "flexibility"
> 
>>    statd is written, this patch introduces the NSM_STATD_PATH
>>    definition that can be defined at compile time with
>>    the --with-statdpath flag.
> 
> I'm not objecting, but why do you think this flexibility is better?  Enumerating a few use cases in the patch description would be helpful.  Is there, for example, a particular distribution that puts this directory outside of /var/lib/nfs?
No example.. I just though it makes sense to decouple the state
directory from the /var/lib/nfs prefix... 

> 
> Basically looks OK, see below for more specific comments.
> 
>>    Signed-off-by: Steve Dickson <steved@redhat.com>
>>
>> diff --git a/configure.ac b/configure.ac
>> index 1a28f8a..6558672 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -22,6 +22,14 @@ AC_ARG_WITH(statedir,
>> 	statedir=$withval,
>> 	statedir=/var/lib/nfs)
>> 	AC_SUBST(statedir)
>> +AC_ARG_WITH(statdpath,
>> +	[AC_HELP_STRING([--with-statdpath=/foo],
>> +		[Causes statd put it's state file in /foo instead of statedir]
> 
> Corrected: add the word "to," make the word "file" plural, and remove incorrect apostrophe.
> 
>   "Causes statd to put its state files in /foo instead of in statedir."
> 
>> +	)],
>> +	statdpath=$withval,
>> +	statdpath=""
>> +	)
>> +	AC_SUBST(statdpath)
>> AC_ARG_WITH(statduser,
>> 	[AC_HELP_STRING([--with-statduser=rpcuser],
>>                         [statd to run under @<:@rpcuser or nobody@:>@]
>> @@ -386,6 +394,9 @@ dnl *************************************************************
>> dnl Export some path names to config.h
>> dnl *************************************************************
>> AC_DEFINE_UNQUOTED(NFS_STATEDIR, "$statedir", [This defines the location of the NFS state files. Warning: this must match definitions in config.mk!])
>> +if test "$statdpath" != ""; then
>> +		AC_DEFINE_UNQUOTED(NSM_STATD_PATH, "$statdpath", [Define this if you what statd file placed in somewhere other than NFS_STATEDIR])
> 
> "what" --> "want"
> 
> though something else may be warranted if you agree with my suggestion below.
Thanks... I guys the patch was not ready for prime time... 

> 
>> +fi
>>
>> if test "x$cross_compiling" = "xno"; then
>> 	CFLAGS_FOR_BUILD=${CFLAGS_FOR_BUILD-"$CFLAGS"}
>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>> index a12c753..2427a6c 100644
>> --- a/support/nsm/file.c
>> +++ b/support/nsm/file.c
>> @@ -95,12 +95,13 @@
>> #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>>
>> /*
>> - * Some distributions place statd's files in a subdirectory
>> + * Allow different places for statd's files
>>  */
>> -#define NSM_PATH_EXTENSION
>> -/* #define NSM_PATH_EXTENSION	"/statd" */
>> -
>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
>> +#ifdef NSM_STATD_PATH
>> +#define NSM_DEFAULT_STATEDIR		NSM_STATD_PATH
>> +#else
>> +#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR 
>> +#endif
> 
> If we are now building the whole path in configure.ac, then it would be simpler and cleaner if this #if logic was in configure.ac, not here.  Just strip all this out, and define NSM_DEFAULT_STATEDIR (or whatever) in configure.ac.  In other words, NSM_DEFAULT_STATEDIR (or whatever) will always be defined, its value will always be listed in config.h, and, by default, it will just be a copy of NFS_STATEDIR.
> 
> Then there would be one single place (config.h) for other programs in nfs-utils, and for human beings, to look to see where statd is sticking its state files.  I for one would not be happy if, in order to fix a bug, I had to hunt through configure.ac, config.h, and support/nsm/file.c just to figure out exactly what pathname was generated.
Good idea... 

> 
>> static char nsm_base_dirname[PATH_MAX] = NSM_DEFAULT_STATEDIR;
> 
>   ...
> 
> If we now have a global definition of the statd state directory pathname, utils/statd/Makefile should use an install hook to fix up the explicit "/var/lib/nfs" references in statd.man and sm-notify.man.  Jeff says he's got some sample code in cifs-utils.  That would finally allow completely removing all the hunks in statdpath.patch.
I'll take a look... 

Thanks for your time! 

steved.
 

Re: [PATCH 3/8] configure.ac: Add --with-statd-extension configure option

[Chuck Lever]

On Sep 17, 2011, at 8:28 AM, Steve Dickson wrote:

> 
> 
> On 09/12/2011 06:06 PM, Chuck Lever wrote:
>> Currently some distributions patch nfs-utils to put NSM state in a
>> subdirectory of /var/lib/nfs.  Make this a configure option instead.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> configure.ac       |    8 ++++++++
>> support/nsm/file.c |    9 +--------
>> 2 files changed, 9 insertions(+), 8 deletions(-)
>> 
>> diff --git a/configure.ac b/configure.ac
>> index 461a96a..ba704e2 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -23,6 +23,14 @@ AC_ARG_WITH(statedir,
>> 	statedir=$withval,
>> 	statedir=/var/lib/nfs)
>> 	AC_SUBST(statedir)
>> +AC_ARG_WITH(statd-extension,
>> +	[AC_HELP_STRING([--with-statd-extension=foo],
>> +                        [Put NSM state in subdir foo of statedir])],
>> +	statdext=$withval,
>> +	statdext="")
>> +	AC_SUBST(statdext)
>> +	AC_DEFINE_UNQUOTED(NSM_PATH_EXTENSION, "$statdext",
>> +			   [This defines the statedir subdirectory containing NSM state files.])
>> AC_ARG_WITH(statduser,
>> 	[AC_HELP_STRING([--with-statduser=rpcuser],
>>                         [statd to run under @<:@rpcuser or nobody@:>@]
>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>> index a12c753..b4a5af1 100644
>> --- a/support/nsm/file.c
>> +++ b/support/nsm/file.c
>> @@ -93,14 +93,7 @@
>> #define LINELEN		(RPCARGSLEN + SM_PRIV_SIZE * 2 + 1)
>> 
>> #define NSM_KERNEL_STATE_FILE	"/proc/sys/fs/nfs/nsm_local_state"
>> -
>> -/*
>> - * Some distributions place statd's files in a subdirectory
>> - */
>> -#define NSM_PATH_EXTENSION
>> -/* #define NSM_PATH_EXTENSION	"/statd" */
>> -
>> -#define NSM_DEFAULT_STATEDIR		NFS_STATEDIR NSM_PATH_EXTENSION
>> +#define NSM_DEFAULT_STATEDIR	NFS_STATEDIR "/" NSM_PATH_EXTENSION
> Do we really need the NSM_PATH_EXTENSION define? Would it be more
> straightforward to just have NFS_STATEDIR. Simplifying the code to:
> 
> #ifndef NFS_STATEDIR 
> #define NFS_STATEDIR "/var/lib/nfs"
> #endif
> 
> #define NSM_DEFAULT_STATEDIR		NFS_STATEDIR
> 
> If there is no need for the extra NSM_PATH_EXTENSION define then 
> we really don't want to create a configuration option for it.. IMHO..

RH likes to put statd files in /var/lib/nfs/statd, but everyone else puts them in /var/lib/nfs. So NSM_DEFAULT_STATEDIR is not the same as NFS_STATEDIR on RH distributions.

I'm caught by this every time I want to test upstream nfs-utils on my Fedora systems.  I have to maintain a separate patch, remember to apply it before testing, and then remember to remove it before pushing my patches to git.linux-nfs.org.

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