[RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[NeilBrown]

This is an RFC series.  A little voice at the back of my head keeps
telling me that I'm over-engineering, but there isn't really that much
new code, and I think the result has a lot to recommend it.

But please tell me if I'm wrong.

- Various daemons (not all) are enhance to accept configuration
  information from /etc/nfs.conf
- the conffile reader is enhanced to support include files, and
  particularly to be able to include /etc/sysconf/X or /etc/defaults/X
  files usefully
- nfs-config.service is removed, because it isn't really needed with
  the above.
- documentation for all the above is provided, including a new
  nfs.systemd man page which gives the bigger picture.

Thanks,
NeilBrown


---

NeilBrown (15):
      Add man-page describing /etc/nfs.conf
      conffile: add bool support
      Add /etc/nfs.conf support to rpc.nfsd
      Add /etc/nfs.conf support for mountd.
      Add /etc/nfs.conf support for statd
      Add /etc/nfs.conf support for sm-notify
      conffile: free image of config file after parsing
      conffile: split loading of file into a separate function.
      conffile: add support for include files.
      conffile: strip "quotes" from values in conf file.
      conffile: ignore setting of empty string
      conffile: allow $name expansion of tag values.
      statd: allow --no-notify to be passed via environment variable.
      systemd: Remove the nfs-config.service
      Add nfs.systemd man page


 configure.ac                     |    6 -
 support/include/conffile.h       |    2 
 support/nfs/conffile.c           |  147 +++++++++++++++++++++---------
 systemd/Makefile.am              |    5 +
 systemd/README                   |   22 +++-
 systemd/nfs-blkmap.service       |    3 -
 systemd/nfs-config.service.in    |   13 ---
 systemd/nfs-idmapd.service       |    6 -
 systemd/nfs-mountd.service       |    6 -
 systemd/nfs-server.service       |    7 -
 systemd/nfs.conf.man             |  186 ++++++++++++++++++++++++++++++++++++++
 systemd/nfs.systemd.man          |  167 ++++++++++++++++++++++++++++++++++
 systemd/rpc-gssd.service.in      |    7 -
 systemd/rpc-statd-notify.service |    6 -
 systemd/rpc-statd.service        |    7 -
 systemd/rpc-svcgssd.service      |    6 -
 utils/mountd/mountd.c            |   36 +++++++
 utils/mountd/mountd.man          |   34 +++++++
 utils/nfsd/nfsd.c                |   36 +++++++
 utils/nfsd/nfsd.man              |   49 +++++++++-
 utils/statd/sm-notify.c          |   11 ++
 utils/statd/sm-notify.man        |   27 ++++++
 utils/statd/statd.c              |   25 +++++
 utils/statd/statd.man            |   40 ++++++++
 24 files changed, 737 insertions(+), 117 deletions(-)
 delete mode 100644 systemd/nfs-config.service.in
 create mode 100644 systemd/nfs.conf.man
 create mode 100644 systemd/nfs.systemd.man

--
Signature

[PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[NeilBrown]

I haven't added -H support, but everything else should be able to be
set through /etc/nfs.conf.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/nfs.conf.man |   24 ++++++++++++++++++++++++
 utils/nfsd/nfsd.c    |   36 ++++++++++++++++++++++++++++++++++++
 utils/nfsd/nfsd.man  |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 1f524d8fe74e..6ac6c65a81d9 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -76,8 +76,32 @@ file is the only way to configure this program.  See
 .BR nfsdcltrack (8)
 for details.
 
+.TP
+.B nfsd
+Recognized values:
+.BR threads ,
+.BR grace-time ,
+.BR lease-time ,
+.BR udp ,
+.BR tcp ,
+.BR vers2 ,
+.BR vers3 ,
+.BR vers4 ,
+.BR vers4.0 ,
+.BR vers4.1 ,
+.BR vers4.2 ,
+.BR rdma .
+
+Version and protocol values are Boolean values as described above.
+Threads and the two times are integers.
+.B rdma
+is a service name or number.  See
+.BR rpc.nfsd (8)
+for details.
+
 .SH FILES
 .I /etc/nfs.conf
 .SH SEE ALSO
 .BR nfsdcltrack (8),
+.BR rpc.nfsd (8),
 .BR nfsmount.conf (5).
diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
index 9a65877f30c3..62b2876948c3 100644
--- a/utils/nfsd/nfsd.c
+++ b/utils/nfsd/nfsd.c
@@ -24,6 +24,7 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 
+#include "conffile.h"
 #include "nfslib.h"
 #include "nfssvc.h"
 #include "xlog.h"
@@ -33,6 +34,8 @@
 #define NFSD_NPROC 8
 #endif
 
+char *conf_path = NFS_CONFFILE;
+
 static void	usage(const char *);
 
 static struct option longopts[] =
@@ -76,6 +79,39 @@ main(int argc, char **argv)
 	xlog_syslog(0);
 	xlog_stderr(1);
 
+	conf_init();
+	count = conf_get_num("nfsd", "threads", count);
+	grace = conf_get_num("nfsd", "grace-time", grace);
+	lease = conf_get_num("nfsd", "lease-time", lease);
+	rdma_port = conf_get_str("nfsd", "rdma");
+	if (conf_get_bool("nfsd", "udp", NFSCTL_UDPISSET(protobits)))
+		NFSCTL_UDPSET(protobits);
+	else
+		NFSCTL_UDPUNSET(protobits);
+	if (conf_get_bool("nfsd", "tcp", NFSCTL_TCPISSET(protobits)))
+		NFSCTL_TCPSET(protobits);
+	else
+		NFSCTL_TCPUNSET(protobits);
+	for (i = 2; i <= 4; i++) {
+		char tag[10];
+		sprintf(tag, "vers%d", i);
+		if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(versbits, i)))
+			NFSCTL_VERSET(versbits, i);
+		else
+			NFSCTL_VERUNSET(versbits, i);
+	}
+	/* We assume the kernel will default all minor versions to 'on',
+	 * and allow the config file to disable some.
+	 */
+	for (i = 0; i <= NFS4_MAXMINOR; i++) {
+		char tag[20];
+		sprintf(tag, "vers4.%d", i);
+		if (!conf_get_bool("nfsd", tag, 1)) {
+			NFSCTL_VERSET(minorversset, i);
+			NFSCTL_VERUNSET(minorversset, i);
+		}
+	}
+
 	while ((c = getopt_long(argc, argv, "dH:hN:V:p:P:sTUrG:L:", longopts, NULL)) != EOF) {
 		switch(c) {
 		case 'd':
diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
index 3ba847e2938f..7b9fbf21a947 100644
--- a/utils/nfsd/nfsd.man
+++ b/utils/nfsd/nfsd.man
@@ -95,11 +95,11 @@ New file open requests (NFSv4) and new file locks (NLM) will not be
 allowed until after this time has passed to allow clients to recover state.
 .TP
 .I nproc
-specify the number of NFS server threads. By default, just one
-thread is started. However, for optimum performance several threads
+specify the number of NFS server threads. By default, eight
+threads are started. However, for optimum performance several threads
 should be used. The actual figure depends on the number of and the work
 load created by the NFS clients, but a useful starting point is
-8 threads. Effects of modifying that number can be checked using
+eight threads. Effects of modifying that number can be checked using
 the
 .BR nfsstat (8)
 program.
@@ -114,6 +114,48 @@ In particular
 .B rpc.nfsd 0
 will stop all threads and thus close any open connections.
 
+.SH CONFIGURATION FILE
+Many of the options that can be set on the command line can also be
+controlled through values set in the
+.B [nfsd]
+section of the
+.I /etc/nfs.conf
+configuration file.  Values recognized include:
+.TP
+.B threads
+The number of threads to start.
+.TP
+.B grace-time
+The grace time, for both NFSv4 and NLM, in seconds.
+.TP
+.B lease-time
+The lease time for NFSv4, in seconds.
+.TP
+.B rdma
+Set RDMA port.  Use "rdma=nfsrdma" to enable standard port.
+.TP
+.B UDP
+Enable (with "on" or "yes" etc) or disable ("off", "no") UDP support.
+.TP
+.B TCP
+Enable or disable TCP support.
+.TP
+.B vers2
+.TP
+.B vers3
+.TP
+.B vers4
+Enable or disable a major NFS version.  3 and 4 are normally enabled
+by default.
+.TP
+.B vers4.1
+.TP
+.B vers4.2
+.TP
+.B vers4.3
+Setting these to "off" or similar will disable the selected minor
+versions.  All are enabled by default.
+
 .SH NOTES
 If the program is built with TI-RPC support, it will enable any protocol and
 address family combinations that are marked visible in the
@@ -125,6 +167,7 @@ database.
 .BR rpc.mountd (8),
 .BR exports (5),
 .BR exportfs (8),
+.BR nfs.conf (5),
 .BR rpc.rquotad (8),
 .BR nfsstat (8),
 .BR netconfig(5).

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[J. Bruce Fields]

On Fri, Dec 02, 2016 at 02:58:28PM +1100, NeilBrown wrote:
> I haven't added -H support, but everything else should be able to be
> set through /etc/nfs.conf.

I'm unclear--is this just a temporary omission for the purposes of this
RFC, or is there some reason you think -H shouldn't be set in nfs.conf?

--b.

> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  systemd/nfs.conf.man |   24 ++++++++++++++++++++++++
>  utils/nfsd/nfsd.c    |   36 ++++++++++++++++++++++++++++++++++++
>  utils/nfsd/nfsd.man  |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 106 insertions(+), 3 deletions(-)
> 
> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
> index 1f524d8fe74e..6ac6c65a81d9 100644
> --- a/systemd/nfs.conf.man
> +++ b/systemd/nfs.conf.man
> @@ -76,8 +76,32 @@ file is the only way to configure this program.  See
>  .BR nfsdcltrack (8)
>  for details.
>  
> +.TP
> +.B nfsd
> +Recognized values:
> +.BR threads ,
> +.BR grace-time ,
> +.BR lease-time ,
> +.BR udp ,
> +.BR tcp ,
> +.BR vers2 ,
> +.BR vers3 ,
> +.BR vers4 ,
> +.BR vers4.0 ,
> +.BR vers4.1 ,
> +.BR vers4.2 ,
> +.BR rdma .
> +
> +Version and protocol values are Boolean values as described above.
> +Threads and the two times are integers.
> +.B rdma
> +is a service name or number.  See
> +.BR rpc.nfsd (8)
> +for details.
> +
>  .SH FILES
>  .I /etc/nfs.conf
>  .SH SEE ALSO
>  .BR nfsdcltrack (8),
> +.BR rpc.nfsd (8),
>  .BR nfsmount.conf (5).
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 9a65877f30c3..62b2876948c3 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -24,6 +24,7 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  
> +#include "conffile.h"
>  #include "nfslib.h"
>  #include "nfssvc.h"
>  #include "xlog.h"
> @@ -33,6 +34,8 @@
>  #define NFSD_NPROC 8
>  #endif
>  
> +char *conf_path = NFS_CONFFILE;
> +
>  static void	usage(const char *);
>  
>  static struct option longopts[] =
> @@ -76,6 +79,39 @@ main(int argc, char **argv)
>  	xlog_syslog(0);
>  	xlog_stderr(1);
>  
> +	conf_init();
> +	count = conf_get_num("nfsd", "threads", count);
> +	grace = conf_get_num("nfsd", "grace-time", grace);
> +	lease = conf_get_num("nfsd", "lease-time", lease);
> +	rdma_port = conf_get_str("nfsd", "rdma");
> +	if (conf_get_bool("nfsd", "udp", NFSCTL_UDPISSET(protobits)))
> +		NFSCTL_UDPSET(protobits);
> +	else
> +		NFSCTL_UDPUNSET(protobits);
> +	if (conf_get_bool("nfsd", "tcp", NFSCTL_TCPISSET(protobits)))
> +		NFSCTL_TCPSET(protobits);
> +	else
> +		NFSCTL_TCPUNSET(protobits);
> +	for (i = 2; i <= 4; i++) {
> +		char tag[10];
> +		sprintf(tag, "vers%d", i);
> +		if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(versbits, i)))
> +			NFSCTL_VERSET(versbits, i);
> +		else
> +			NFSCTL_VERUNSET(versbits, i);
> +	}
> +	/* We assume the kernel will default all minor versions to 'on',
> +	 * and allow the config file to disable some.
> +	 */
> +	for (i = 0; i <= NFS4_MAXMINOR; i++) {
> +		char tag[20];
> +		sprintf(tag, "vers4.%d", i);
> +		if (!conf_get_bool("nfsd", tag, 1)) {
> +			NFSCTL_VERSET(minorversset, i);
> +			NFSCTL_VERUNSET(minorversset, i);
> +		}
> +	}
> +
>  	while ((c = getopt_long(argc, argv, "dH:hN:V:p:P:sTUrG:L:", longopts, NULL)) != EOF) {
>  		switch(c) {
>  		case 'd':
> diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
> index 3ba847e2938f..7b9fbf21a947 100644
> --- a/utils/nfsd/nfsd.man
> +++ b/utils/nfsd/nfsd.man
> @@ -95,11 +95,11 @@ New file open requests (NFSv4) and new file locks (NLM) will not be
>  allowed until after this time has passed to allow clients to recover state.
>  .TP
>  .I nproc
> -specify the number of NFS server threads. By default, just one
> -thread is started. However, for optimum performance several threads
> +specify the number of NFS server threads. By default, eight
> +threads are started. However, for optimum performance several threads
>  should be used. The actual figure depends on the number of and the work
>  load created by the NFS clients, but a useful starting point is
> -8 threads. Effects of modifying that number can be checked using
> +eight threads. Effects of modifying that number can be checked using
>  the
>  .BR nfsstat (8)
>  program.
> @@ -114,6 +114,48 @@ In particular
>  .B rpc.nfsd 0
>  will stop all threads and thus close any open connections.
>  
> +.SH CONFIGURATION FILE
> +Many of the options that can be set on the command line can also be
> +controlled through values set in the
> +.B [nfsd]
> +section of the
> +.I /etc/nfs.conf
> +configuration file.  Values recognized include:
> +.TP
> +.B threads
> +The number of threads to start.
> +.TP
> +.B grace-time
> +The grace time, for both NFSv4 and NLM, in seconds.
> +.TP
> +.B lease-time
> +The lease time for NFSv4, in seconds.
> +.TP
> +.B rdma
> +Set RDMA port.  Use "rdma=nfsrdma" to enable standard port.
> +.TP
> +.B UDP
> +Enable (with "on" or "yes" etc) or disable ("off", "no") UDP support.
> +.TP
> +.B TCP
> +Enable or disable TCP support.
> +.TP
> +.B vers2
> +.TP
> +.B vers3
> +.TP
> +.B vers4
> +Enable or disable a major NFS version.  3 and 4 are normally enabled
> +by default.
> +.TP
> +.B vers4.1
> +.TP
> +.B vers4.2
> +.TP
> +.B vers4.3
> +Setting these to "off" or similar will disable the selected minor
> +versions.  All are enabled by default.
> +
>  .SH NOTES
>  If the program is built with TI-RPC support, it will enable any protocol and
>  address family combinations that are marked visible in the
> @@ -125,6 +167,7 @@ database.
>  .BR rpc.mountd (8),
>  .BR exports (5),
>  .BR exportfs (8),
> +.BR nfs.conf (5),
>  .BR rpc.rquotad (8),
>  .BR nfsstat (8),
>  .BR netconfig(5).
> 

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

On Tue, Dec 06 2016, J. Bruce Fields wrote:

> On Fri, Dec 02, 2016 at 02:58:28PM +1100, NeilBrown wrote:
>> I haven't added -H support, but everything else should be able to be
>> set through /etc/nfs.conf.
>
> I'm unclear--is this just a temporary omission for the purposes of this
> RFC, or is there some reason you think -H shouldn't be set in nfs.conf?
>

Temporary omission.
I would need to decide how to store a list in nfs.conf.
1/
  [nfsd]
  host = foo
  host = bar

2/
  [nfsd]
    host = foo, bar

3/ ??

and I suspect that any -H on the command line would over-ride all host=
in nfs.conf, but I'm not certain.
I'm sure there is a good, defensible solution here, but I didn't want to
spend time on it for the RFC.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[Steve Dickson]

On 12/01/2016 10:58 PM, NeilBrown wrote:
> I haven't added -H support, but everything else should be able to be
> set through /etc/nfs.conf.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  systemd/nfs.conf.man |   24 ++++++++++++++++++++++++
>  utils/nfsd/nfsd.c    |   36 ++++++++++++++++++++++++++++++++++++
>  utils/nfsd/nfsd.man  |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 106 insertions(+), 3 deletions(-)
> 
> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
> index 1f524d8fe74e..6ac6c65a81d9 100644
> --- a/systemd/nfs.conf.man
> +++ b/systemd/nfs.conf.man
> @@ -76,8 +76,32 @@ file is the only way to configure this program.  See
>  .BR nfsdcltrack (8)
>  for details.
>  
> +.TP
> +.B nfsd
> +Recognized values:
> +.BR threads ,
> +.BR grace-time ,
> +.BR lease-time ,
> +.BR udp ,
> +.BR tcp ,
> +.BR vers2 ,
> +.BR vers3 ,
> +.BR vers4 ,
> +.BR vers4.0 ,
> +.BR vers4.1 ,
> +.BR vers4.2 ,
> +.BR rdma .
I'm curious as to what the criteria was as to
which options were defined. I would think 'debug'
and 'port' would have made the list.

steved.

> +
> +Version and protocol values are Boolean values as described above.
> +Threads and the two times are integers.
> +.B rdma
> +is a service name or number.  See
> +.BR rpc.nfsd (8)
> +for details.
> +
>  .SH FILES
>  .I /etc/nfs.conf
>  .SH SEE ALSO
>  .BR nfsdcltrack (8),
> +.BR rpc.nfsd (8),
>  .BR nfsmount.conf (5).
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 9a65877f30c3..62b2876948c3 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -24,6 +24,7 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  
> +#include "conffile.h"
>  #include "nfslib.h"
>  #include "nfssvc.h"
>  #include "xlog.h"
> @@ -33,6 +34,8 @@
>  #define NFSD_NPROC 8
>  #endif
>  
> +char *conf_path = NFS_CONFFILE;
> +
>  static void	usage(const char *);
>  
>  static struct option longopts[] =
> @@ -76,6 +79,39 @@ main(int argc, char **argv)
>  	xlog_syslog(0);
>  	xlog_stderr(1);
>  
> +	conf_init();
> +	count = conf_get_num("nfsd", "threads", count);
> +	grace = conf_get_num("nfsd", "grace-time", grace);
> +	lease = conf_get_num("nfsd", "lease-time", lease);
> +	rdma_port = conf_get_str("nfsd", "rdma");
> +	if (conf_get_bool("nfsd", "udp", NFSCTL_UDPISSET(protobits)))
> +		NFSCTL_UDPSET(protobits);
> +	else
> +		NFSCTL_UDPUNSET(protobits);
> +	if (conf_get_bool("nfsd", "tcp", NFSCTL_TCPISSET(protobits)))
> +		NFSCTL_TCPSET(protobits);
> +	else
> +		NFSCTL_TCPUNSET(protobits);
> +	for (i = 2; i <= 4; i++) {
> +		char tag[10];
> +		sprintf(tag, "vers%d", i);
> +		if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(versbits, i)))
> +			NFSCTL_VERSET(versbits, i);
> +		else
> +			NFSCTL_VERUNSET(versbits, i);
> +	}
> +	/* We assume the kernel will default all minor versions to 'on',
> +	 * and allow the config file to disable some.
> +	 */
> +	for (i = 0; i <= NFS4_MAXMINOR; i++) {
> +		char tag[20];
> +		sprintf(tag, "vers4.%d", i);
> +		if (!conf_get_bool("nfsd", tag, 1)) {
> +			NFSCTL_VERSET(minorversset, i);
> +			NFSCTL_VERUNSET(minorversset, i);
> +		}
> +	}
> +
>  	while ((c = getopt_long(argc, argv, "dH:hN:V:p:P:sTUrG:L:", longopts, NULL)) != EOF) {
>  		switch(c) {
>  		case 'd':
> diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
> index 3ba847e2938f..7b9fbf21a947 100644
> --- a/utils/nfsd/nfsd.man
> +++ b/utils/nfsd/nfsd.man
> @@ -95,11 +95,11 @@ New file open requests (NFSv4) and new file locks (NLM) will not be
>  allowed until after this time has passed to allow clients to recover state.
>  .TP
>  .I nproc
> -specify the number of NFS server threads. By default, just one
> -thread is started. However, for optimum performance several threads
> +specify the number of NFS server threads. By default, eight
> +threads are started. However, for optimum performance several threads
>  should be used. The actual figure depends on the number of and the work
>  load created by the NFS clients, but a useful starting point is
> -8 threads. Effects of modifying that number can be checked using
> +eight threads. Effects of modifying that number can be checked using
>  the
>  .BR nfsstat (8)
>  program.
> @@ -114,6 +114,48 @@ In particular
>  .B rpc.nfsd 0
>  will stop all threads and thus close any open connections.
>  
> +.SH CONFIGURATION FILE
> +Many of the options that can be set on the command line can also be
> +controlled through values set in the
> +.B [nfsd]
> +section of the
> +.I /etc/nfs.conf
> +configuration file.  Values recognized include:
> +.TP
> +.B threads
> +The number of threads to start.
> +.TP
> +.B grace-time
> +The grace time, for both NFSv4 and NLM, in seconds.
> +.TP
> +.B lease-time
> +The lease time for NFSv4, in seconds.
> +.TP
> +.B rdma
> +Set RDMA port.  Use "rdma=nfsrdma" to enable standard port.
> +.TP
> +.B UDP
> +Enable (with "on" or "yes" etc) or disable ("off", "no") UDP support.
> +.TP
> +.B TCP
> +Enable or disable TCP support.
> +.TP
> +.B vers2
> +.TP
> +.B vers3
> +.TP
> +.B vers4
> +Enable or disable a major NFS version.  3 and 4 are normally enabled
> +by default.
> +.TP
> +.B vers4.1
> +.TP
> +.B vers4.2
> +.TP
> +.B vers4.3
> +Setting these to "off" or similar will disable the selected minor
> +versions.  All are enabled by default.
> +
>  .SH NOTES
>  If the program is built with TI-RPC support, it will enable any protocol and
>  address family combinations that are marked visible in the
> @@ -125,6 +167,7 @@ database.
>  .BR rpc.mountd (8),
>  .BR exports (5),
>  .BR exportfs (8),
> +.BR nfs.conf (5),
>  .BR rpc.rquotad (8),
>  .BR nfsstat (8),
>  .BR netconfig(5).
> 
> 

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

On Wed, Dec 07 2016, Steve Dickson wrote:

> On 12/01/2016 10:58 PM, NeilBrown wrote:
>> I haven't added -H support, but everything else should be able to be
>> set through /etc/nfs.conf.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  systemd/nfs.conf.man |   24 ++++++++++++++++++++++++
>>  utils/nfsd/nfsd.c    |   36 ++++++++++++++++++++++++++++++++++++
>>  utils/nfsd/nfsd.man  |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 106 insertions(+), 3 deletions(-)
>> 
>> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
>> index 1f524d8fe74e..6ac6c65a81d9 100644
>> --- a/systemd/nfs.conf.man
>> +++ b/systemd/nfs.conf.man
>> @@ -76,8 +76,32 @@ file is the only way to configure this program.  See
>>  .BR nfsdcltrack (8)
>>  for details.
>>  
>> +.TP
>> +.B nfsd
>> +Recognized values:
>> +.BR threads ,
>> +.BR grace-time ,
>> +.BR lease-time ,
>> +.BR udp ,
>> +.BR tcp ,
>> +.BR vers2 ,
>> +.BR vers3 ,
>> +.BR vers4 ,
>> +.BR vers4.0 ,
>> +.BR vers4.1 ,
>> +.BR vers4.2 ,
>> +.BR rdma .
> I'm curious as to what the criteria was as to
> which options were defined. I would think 'debug'
> and 'port' would have made the list.

port should have made the list, it just .... didn't.

debug was more of a deliberate omission, or at least a deferral.
I didn't include debug for any daemons. mountd, nfsd, nfsdcltrack all
allow the option.  statd blends it with no-syslog. sm-notify has "-d".
mountd's --debug is different to the other, and takes word, as does
"exportfs -d".  The word is passed to xlog_sconfig can can select from
general, call, auth, parse, all.

Assuming that we want to be able to enable debug in the config file
(wouldn't you just run that command manually when you want to enable
debugging?) it would be good to standardize somehow.

So I guess I was treating "debug" like "host" - do it later.
I have no excuse for "port" :-(

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[Steve Dickson]

On 12/06/2016 05:30 PM, NeilBrown wrote:
>> I'm curious as to what the criteria was as to
>> which options were defined. I would think 'debug'
>> and 'port' would have made the list.
> port should have made the list, it just .... didn't.
Sure... I'll add it... 

> 
> debug was more of a deliberate omission, or at least a deferral.
> I didn't include debug for any daemons. mountd, nfsd, nfsdcltrack all
> allow the option.  statd blends it with no-syslog. sm-notify has "-d".
> mountd's --debug is different to the other, and takes word, as does
> "exportfs -d".  The word is passed to xlog_sconfig can can select from
> general, call, auth, parse, all.
For the ones that take a word, we could make debug=1 mean 
--debug all as the default. If other types of debug is 
need the command like can always be used. 

> 
> Assuming that we want to be able to enable debug in the config file
> (wouldn't you just run that command manually when you want to enable
> debugging?) it would be good to standardize somehow.
Hopefully through the nfs.conf file things can be standardized.
I'm not sure we want change command line arguments for
that reason though... 
 
> 
> So I guess I was treating "debug" like "host" - do it later.
> I have no excuse for "port" :-(
Sounds good... 

steved.

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[Steve Dickson]

On 12/01/2016 10:58 PM, NeilBrown wrote:
> I haven't added -H support, but everything else should be able to be
> set through /etc/nfs.conf.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  systemd/nfs.conf.man |   24 ++++++++++++++++++++++++
>  utils/nfsd/nfsd.c    |   36 ++++++++++++++++++++++++++++++++++++
>  utils/nfsd/nfsd.man  |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 106 insertions(+), 3 deletions(-)
> 
> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
> index 1f524d8fe74e..6ac6c65a81d9 100644
> --- a/systemd/nfs.conf.man
> +++ b/systemd/nfs.conf.man
> @@ -76,8 +76,32 @@ file is the only way to configure this program.  See
>  .BR nfsdcltrack (8)
>  for details.
>  
> +.TP
> +.B nfsd
> +Recognized values:
> +.BR threads ,
> +.BR grace-time ,
> +.BR lease-time ,
> +.BR udp ,
> +.BR tcp ,
> +.BR vers2 ,
> +.BR vers3 ,
> +.BR vers4 ,
> +.BR vers4.0 ,
Do we need both ver4 and ver4.0?

steved.
> +.BR vers4.1 ,
> +.BR vers4.2 ,
> +.BR rdma .
> +
> +Version and protocol values are Boolean values as described above.
> +Threads and the two times are integers.
> +.B rdma
> +is a service name or number.  See
> +.BR rpc.nfsd (8)
> +for details.
> +
>  .SH FILES
>  .I /etc/nfs.conf
>  .SH SEE ALSO
>  .BR nfsdcltrack (8),
> +.BR rpc.nfsd (8),
>  .BR nfsmount.conf (5).
> diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c
> index 9a65877f30c3..62b2876948c3 100644
> --- a/utils/nfsd/nfsd.c
> +++ b/utils/nfsd/nfsd.c
> @@ -24,6 +24,7 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  
> +#include "conffile.h"
>  #include "nfslib.h"
>  #include "nfssvc.h"
>  #include "xlog.h"
> @@ -33,6 +34,8 @@
>  #define NFSD_NPROC 8
>  #endif
>  
> +char *conf_path = NFS_CONFFILE;
> +
>  static void	usage(const char *);
>  
>  static struct option longopts[] =
> @@ -76,6 +79,39 @@ main(int argc, char **argv)
>  	xlog_syslog(0);
>  	xlog_stderr(1);
>  
> +	conf_init();
> +	count = conf_get_num("nfsd", "threads", count);
> +	grace = conf_get_num("nfsd", "grace-time", grace);
> +	lease = conf_get_num("nfsd", "lease-time", lease);
> +	rdma_port = conf_get_str("nfsd", "rdma");
> +	if (conf_get_bool("nfsd", "udp", NFSCTL_UDPISSET(protobits)))
> +		NFSCTL_UDPSET(protobits);
> +	else
> +		NFSCTL_UDPUNSET(protobits);
> +	if (conf_get_bool("nfsd", "tcp", NFSCTL_TCPISSET(protobits)))
> +		NFSCTL_TCPSET(protobits);
> +	else
> +		NFSCTL_TCPUNSET(protobits);
> +	for (i = 2; i <= 4; i++) {
> +		char tag[10];
> +		sprintf(tag, "vers%d", i);
> +		if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(versbits, i)))
> +			NFSCTL_VERSET(versbits, i);
> +		else
> +			NFSCTL_VERUNSET(versbits, i);
> +	}
> +	/* We assume the kernel will default all minor versions to 'on',
> +	 * and allow the config file to disable some.
> +	 */
> +	for (i = 0; i <= NFS4_MAXMINOR; i++) {
> +		char tag[20];
> +		sprintf(tag, "vers4.%d", i);
> +		if (!conf_get_bool("nfsd", tag, 1)) {
> +			NFSCTL_VERSET(minorversset, i);
> +			NFSCTL_VERUNSET(minorversset, i);
> +		}
> +	}
> +
>  	while ((c = getopt_long(argc, argv, "dH:hN:V:p:P:sTUrG:L:", longopts, NULL)) != EOF) {
>  		switch(c) {
>  		case 'd':
> diff --git a/utils/nfsd/nfsd.man b/utils/nfsd/nfsd.man
> index 3ba847e2938f..7b9fbf21a947 100644
> --- a/utils/nfsd/nfsd.man
> +++ b/utils/nfsd/nfsd.man
> @@ -95,11 +95,11 @@ New file open requests (NFSv4) and new file locks (NLM) will not be
>  allowed until after this time has passed to allow clients to recover state.
>  .TP
>  .I nproc
> -specify the number of NFS server threads. By default, just one
> -thread is started. However, for optimum performance several threads
> +specify the number of NFS server threads. By default, eight
> +threads are started. However, for optimum performance several threads
>  should be used. The actual figure depends on the number of and the work
>  load created by the NFS clients, but a useful starting point is
> -8 threads. Effects of modifying that number can be checked using
> +eight threads. Effects of modifying that number can be checked using
>  the
>  .BR nfsstat (8)
>  program.
> @@ -114,6 +114,48 @@ In particular
>  .B rpc.nfsd 0
>  will stop all threads and thus close any open connections.
>  
> +.SH CONFIGURATION FILE
> +Many of the options that can be set on the command line can also be
> +controlled through values set in the
> +.B [nfsd]
> +section of the
> +.I /etc/nfs.conf
> +configuration file.  Values recognized include:
> +.TP
> +.B threads
> +The number of threads to start.
> +.TP
> +.B grace-time
> +The grace time, for both NFSv4 and NLM, in seconds.
> +.TP
> +.B lease-time
> +The lease time for NFSv4, in seconds.
> +.TP
> +.B rdma
> +Set RDMA port.  Use "rdma=nfsrdma" to enable standard port.
> +.TP
> +.B UDP
> +Enable (with "on" or "yes" etc) or disable ("off", "no") UDP support.
> +.TP
> +.B TCP
> +Enable or disable TCP support.
> +.TP
> +.B vers2
> +.TP
> +.B vers3
> +.TP
> +.B vers4
> +Enable or disable a major NFS version.  3 and 4 are normally enabled
> +by default.
> +.TP
> +.B vers4.1
> +.TP
> +.B vers4.2
> +.TP
> +.B vers4.3
> +Setting these to "off" or similar will disable the selected minor
> +versions.  All are enabled by default.
> +
>  .SH NOTES
>  If the program is built with TI-RPC support, it will enable any protocol and
>  address family combinations that are marked visible in the
> @@ -125,6 +167,7 @@ database.
>  .BR rpc.mountd (8),
>  .BR exports (5),
>  .BR exportfs (8),
> +.BR nfs.conf (5),
>  .BR rpc.rquotad (8),
>  .BR nfsstat (8),
>  .BR netconfig(5).
> 
> 

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On Wed, Dec 07 2016, Steve Dickson wrote:

>> +.B nfsd
>> +Recognized values:
>> +.BR threads ,
>> +.BR grace-time ,
>> +.BR lease-time ,
>> +.BR udp ,
>> +.BR tcp ,
>> +.BR vers2 ,
>> +.BR vers3 ,
>> +.BR vers4 ,
>> +.BR vers4.0 ,
> Do we need both ver4 and ver4.0?

"vers4" allows you to enable or disable v4 has a whole.
The assumption is that if enabled, all minor versions that the kernel
supports will be enabled by default.
vers4.x allows individual minor versions to be disabled, so
 vers4=yes
 vers4.0=no
would disable v4.0, just like "-V4 -N4.0"

I do wonder if this is ever valid though.  Why do we allow minor
versions to be enabled/disabled?
Does it make any sense to enable a non-contiguous set of minor versions?
Should we just have a maximum NFSv4 minor version?

I was trying to duplicate the current functionality as closely as
convenient.  That might not be best in this case.

Thanks,
NeilBrown


>
> steved.
>> +.BR vers4.1 ,
>> +.BR vers4.2 ,
>> +.BR rdma .

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[Steve Dickson]

On 12/06/2016 05:36 PM, NeilBrown wrote:
> On Wed, Dec 07 2016, Steve Dickson wrote:
> 
>>> +.B nfsd
>>> +Recognized values:
>>> +.BR threads ,
>>> +.BR grace-time ,
>>> +.BR lease-time ,
>>> +.BR udp ,
>>> +.BR tcp ,
>>> +.BR vers2 ,
>>> +.BR vers3 ,
>>> +.BR vers4 ,
>>> +.BR vers4.0 ,
>> Do we need both ver4 and ver4.0?
> 
> "vers4" allows you to enable or disable v4 has a whole.
> The assumption is that if enabled, all minor versions that the kernel
> supports will be enabled by default.
> vers4.x allows individual minor versions to be disabled, so
>  vers4=yes
>  vers4.0=no
> would disable v4.0, just like "-V4 -N4.0"
I see...

> 
> I do wonder if this is ever valid though.  Why do we allow minor
> versions to be enabled/disabled?
IDK... I think Trond did this... you know...
when in doubt... blame Trond! 8-) 
 
> Does it make any sense to enable a non-contiguous set of minor versions?
I don't think so... Talk about handing people rope! ;-)

> Should we just have a maximum NFSv4 minor version?
Maybe.. 

> 
> I was trying to duplicate the current functionality as closely as
> convenient.  That might not be best in this case.
You did a good job... this is definitely a nit.

steved.

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[J. Bruce Fields]

On Wed, Dec 07, 2016 at 09:44:25AM -0500, Steve Dickson wrote:
> 
> 
> On 12/06/2016 05:36 PM, NeilBrown wrote:
> > On Wed, Dec 07 2016, Steve Dickson wrote:
> > 
> >>> +.B nfsd
> >>> +Recognized values:
> >>> +.BR threads ,
> >>> +.BR grace-time ,
> >>> +.BR lease-time ,
> >>> +.BR udp ,
> >>> +.BR tcp ,
> >>> +.BR vers2 ,
> >>> +.BR vers3 ,
> >>> +.BR vers4 ,
> >>> +.BR vers4.0 ,
> >> Do we need both ver4 and ver4.0?
> > 
> > "vers4" allows you to enable or disable v4 has a whole.
> > The assumption is that if enabled, all minor versions that the kernel
> > supports will be enabled by default.
> > vers4.x allows individual minor versions to be disabled, so
> >  vers4=yes
> >  vers4.0=no
> > would disable v4.0, just like "-V4 -N4.0"
> I see...
> 
> > 
> > I do wonder if this is ever valid though.  Why do we allow minor
> > versions to be enabled/disabled?
> IDK... I think Trond did this... you know...
> when in doubt... blame Trond! 8-) 

Or Benny, 8daf220a6a83 "nfsd41: control nfsv4.1 svc via
/proc/fs/nfsd/versions".

> > Does it make any sense to enable a non-contiguous set of minor versions?
> I don't think so... Talk about handing people rope! ;-)

I can't think of a reason either.

> > Should we just have a maximum NFSv4 minor version?
> Maybe.. 

If you do that then I'd allow a minimum too.

--b.

> > I was trying to duplicate the current functionality as closely as
> > convenient.  That might not be best in this case.
> You did a good job... this is definitely a nit.
> 
> steved.
> 

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]

On Thu, Dec 08 2016, "J. Bruce Fields" <bfields@redhat.com> wrote:

> On Wed, Dec 07, 2016 at 09:44:25AM -0500, Steve Dickson wrote:
>> > 
>> > I do wonder if this is ever valid though.  Why do we allow minor
>> > versions to be enabled/disabled?
>> IDK... I think Trond did this... you know...
>> when in doubt... blame Trond! 8-) 
>
> Or Benny, 8daf220a6a83 "nfsd41: control nfsv4.1 svc via
> /proc/fs/nfsd/versions".
>
>> > Does it make any sense to enable a non-contiguous set of minor versions?
>> I don't think so... Talk about handing people rope! ;-)
>
> I can't think of a reason either.
>
>> > Should we just have a maximum NFSv4 minor version?
>> Maybe.. 
>
> If you do that then I'd allow a minimum too.

Why?  I'm trying to understand why you would ever want to turn disable a
particular minor version.  I could equally well ask myself "why specify
a maximum"...

Benny's patch only suggests that it removes the need for
CONFIG_NFSD_V4_1, but why was that needed?
Maybe it is just to disable buggy code if some version is found to have
an inconvenient bug?  The client, by default, tries 4.2, then 4.1, then
4.0.  Older clients might start elsewhere.  So just disabling unreliable
versions individually does seem to make sense.

I note that Benny's original patch disabled v4.1 by default, and
required it to be explicitly enabled.  The current kernel enables all by
default, and requires them be explicitly disabled is required.
My current nfs.conf code will disabled unwanted minor versions, but not
enable any that are already disabled ... I guess I could fix that.

.... it is a shame that we can only enable/disable minor versions
when there are no nfsd threads running.  The justification for
failing if nfsd is active is fairly lame, and only applies
to major versions.  If minor versions could be changed at any time,
then it could be a simple function of rpc.nfsd, and the config
file would never need to enable things, only disable them.

Is there any other reason to disable minor versions, other than to avoid
buggy code (either in server or client)??


Actually.... I have a related question that I keep meaning to ask, but
haven't.

What are the circumstances where NFSv3 should be preferred over NFSv4
(assuming up-to-date kernels on server and client)?

I can think of:
 - NFSv3 is safer if you might need to support loop-back mounts
   as I don't think NFSv4 state management can reliably make forward
   progress against memory pressure.  In particular, creating the
   state-manager thread can deadlock waiting for an NFS write.

 - If you want fast-failover from one server to another,
   NFSv3 is probably faster as it only imposes the grace-time on
   lock requests.  NFSv4 imposes it on OPEN and so READ/WRITE
   as well.

 - NFSv3 is preferred if (for some weird reason) you need
   to use UDP.

Are there any others?  Are there any similar reasons to prefer a smaller
NFSv4 minor version over a larger one?

Thanks,
NeilBrown

>
> --b.
>
>> > I was trying to duplicate the current functionality as closely as
>> > convenient.  That might not be best in this case.
>> You did a good job... this is definitely a nit.
>> 
>> steved.
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[Steve Dickson]

On 12/07/2016 06:14 PM, NeilBrown wrote:
>> If you do that then I'd allow a minimum too.
> Why?  I'm trying to understand why you would ever want to turn disable a
> particular minor version.  I could equally well ask myself "why specify
> a maximum"...
I think I agree with this... Why put limits on what version
people can use.

> 
> Benny's patch only suggests that it removes the need for
> CONFIG_NFSD_V4_1, but why was that needed?
> Maybe it is just to disable buggy code if some version is found to have
> an inconvenient bug?  The client, by default, tries 4.2, then 4.1, then
> 4.0.  Older clients might start elsewhere.  So just disabling unreliable
> versions individually does seem to make sense.
> 
> I note that Benny's original patch disabled v4.1 by default, and
> required it to be explicitly enabled.  The current kernel enables all by
> default, and requires them be explicitly disabled is required.
I think it was to protect distros from enabling code 
they were not going to test. I remember it was a while until
RHEL6 enable the v4.1 code because we were not willing to
support it. 

> My current nfs.conf code will disabled unwanted minor versions, but not
> enable any that are already disabled ... I guess I could fix that.
I think disabling minor version for nfs.conf is perfect! 

But the only issue I see... how does one distribute that
configuration over thousands of servers/vms... that is
something we have never been good at... IMHO.
 
> 
> .... it is a shame that we can only enable/disable minor versions
> when there are no nfsd threads running.  The justification for
> failing if nfsd is active is fairly lame, and only applies
> to major versions.  If minor versions could be changed at any time,
> then it could be a simple function of rpc.nfsd, and the config
> file would never need to enable things, only disable them.
As you know rpc.nfsd is a hit and run command... It 
sets things up and then exits. I just don't see why there
would be a need to change the support of minor versions
on the fly? What about existing mounts?? Or maybe I'm
misunderstanding... 
 
> 
> Is there any other reason to disable minor versions, other than to avoid
> buggy code (either in server or client)??
There seems to be a history in Linux to be able to configure
exactly what you what in or out... Which is not a bad thing, IMHO. 
 
> 
> 
> Actually.... I have a related question that I keep meaning to ask, but
> haven't.
> 
> What are the circumstances where NFSv3 should be preferred over NFSv4
> (assuming up-to-date kernels on server and client)?
Good Question... This is worthy of a another thread! : -) 

> 
> I can think of:
>  - NFSv3 is safer if you might need to support loop-back mounts
>    as I don't think NFSv4 state management can reliably make forward
>    progress against memory pressure.  In particular, creating the
>    state-manager thread can deadlock waiting for an NFS write.
Agreed... v3 loopback make more sense that v4 mounts but 
what is the sense of doing NFS lookback mount in the
first place? Why not just do bind mount and be done with it! 

> 
>  - If you want fast-failover from one server to another,
>    NFSv3 is probably faster as it only imposes the grace-time on
>    lock requests.  NFSv4 imposes it on OPEN and so READ/WRITE
>    as well.
v3 will always be faster... IMHO... The less data to move will
always win the race. 

> 
>  - NFSv3 is preferred if (for some weird reason) you need
>    to use UDP.
Do you known anybody that is this position? As you know I'm
beating the path to turn off UDP support for all NFS daemons.

> 
> Are there any others?  Are there any similar reasons to prefer a smaller
> NFSv4 minor version over a larger one?
I'm thinking no... to the last question... v4.0 was the beginning,
v4.1 was better (recent server only support 4.1), and 4.2 has
features that will seriously improve performance...  
In theory :-) 

steved.  

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[J. Bruce Fields]

On Thu, Dec 08, 2016 at 10:14:54AM +1100, NeilBrown wrote:
> On Thu, Dec 08 2016, "J. Bruce Fields" <bfields@redhat.com> wrote:
> 
> > On Wed, Dec 07, 2016 at 09:44:25AM -0500, Steve Dickson wrote:
> >> > 
> >> > I do wonder if this is ever valid though.  Why do we allow minor
> >> > versions to be enabled/disabled?
> >> IDK... I think Trond did this... you know...
> >> when in doubt... blame Trond! 8-) 
> >
> > Or Benny, 8daf220a6a83 "nfsd41: control nfsv4.1 svc via
> > /proc/fs/nfsd/versions".
> >
> >> > Does it make any sense to enable a non-contiguous set of minor versions?
> >> I don't think so... Talk about handing people rope! ;-)
> >
> > I can't think of a reason either.
> >
> >> > Should we just have a maximum NFSv4 minor version?
> >> Maybe.. 
> >
> > If you do that then I'd allow a minimum too.
> 
> Why?  I'm trying to understand why you would ever want to turn disable a
> particular minor version.  I could equally well ask myself "why specify
> a maximum"...

In the past setting a default maximum that's lower than the maximum
supported by the code has been a way to make new features available for
testers and early adopters while keeping other users safe.

For a minimum--I think "1" would be the most likely minimum, just
because there were some major changes made between 4.0 and 4.1, and
people might eventually want to enforce a policy of supporting only
versions >=4.1 in their environements.

And eventually, I don't know, maybe you really don't want clients
mounting unless they support labeled NFS, or something?  Though you
can't really force them to use new features.  I haven't really thought
it through.

> Benny's patch only suggests that it removes the need for
> CONFIG_NFSD_V4_1, but why was that needed?
> Maybe it is just to disable buggy code if some version is found to have
> an inconvenient bug?  The client, by default, tries 4.2, then 4.1, then
> 4.0.  Older clients might start elsewhere.  So just disabling unreliable
> versions individually does seem to make sense.
> 
> I note that Benny's original patch disabled v4.1 by default, and
> required it to be explicitly enabled.  The current kernel enables all by
> default, and requires them be explicitly disabled is required.
> My current nfs.conf code will disabled unwanted minor versions, but not
> enable any that are already disabled ... I guess I could fix that.

I think what that means is that the defaults are determined by the
kernel version you're running as opposed to by userspace.

The main reason for disabling minor versions has been immature code in
higher versions, and in that case I think it makes sense for the kernel
to provide the default: if you don't have a particular preference, I
think the choice of whether to run 4.1 or 4.0 probably depends on how
new your kernel is.

--b.

> .... it is a shame that we can only enable/disable minor versions
> when there are no nfsd threads running.  The justification for
> failing if nfsd is active is fairly lame, and only applies
> to major versions.  If minor versions could be changed at any time,
> then it could be a simple function of rpc.nfsd, and the config
> file would never need to enable things, only disable them.
> 
> Is there any other reason to disable minor versions, other than to avoid
> buggy code (either in server or client)??
> 
> 
> Actually.... I have a related question that I keep meaning to ask, but
> haven't.
> 
> What are the circumstances where NFSv3 should be preferred over NFSv4
> (assuming up-to-date kernels on server and client)?
> 
> I can think of:
>  - NFSv3 is safer if you might need to support loop-back mounts
>    as I don't think NFSv4 state management can reliably make forward
>    progress against memory pressure.  In particular, creating the
>    state-manager thread can deadlock waiting for an NFS write.
> 
>  - If you want fast-failover from one server to another,
>    NFSv3 is probably faster as it only imposes the grace-time on
>    lock requests.  NFSv4 imposes it on OPEN and so READ/WRITE
>    as well.
> 
>  - NFSv3 is preferred if (for some weird reason) you need
>    to use UDP.
> 
> Are there any others?  Are there any similar reasons to prefer a smaller
> NFSv4 minor version over a larger one?
> 
> Thanks,
> NeilBrown
> 
> >
> > --b.
> >
> >> > I was trying to duplicate the current functionality as closely as
> >> > convenient.  That might not be best in this case.
> >> You did a good job... this is definitely a nit.
> >> 
> >> steved.
> >> 

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On Wed, Dec 07 2016, NeilBrown wrote:

> [ Unknown signature status ]
> On Wed, Dec 07 2016, Steve Dickson wrote:
>
>>> +.B nfsd
>>> +Recognized values:
>>> +.BR threads ,
>>> +.BR grace-time ,
>>> +.BR lease-time ,
>>> +.BR udp ,
>>> +.BR tcp ,
>>> +.BR vers2 ,
>>> +.BR vers3 ,
>>> +.BR vers4 ,
>>> +.BR vers4.0 ,
>> Do we need both ver4 and ver4.0?
>
> "vers4" allows you to enable or disable v4 has a whole.
> The assumption is that if enabled, all minor versions that the kernel
> supports will be enabled by default.
> vers4.x allows individual minor versions to be disabled, so
>  vers4=yes
>  vers4.0=no
> would disable v4.0, just like "-V4 -N4.0"

What I meant here, of course, is
   just like "-V4 -N4.0" *should*

not "just like what it *does*."

To my surprise, when you actually try "nfsd -N4.0",
it opens /proc/fs/nfsd/versions and writes "+4.32".

#define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1))) 

That "-1" makes it clear that version 4.0 isn't understood.

The kernel side seem to understand that '0' is an independent minor version:
static bool nfsd_supported_minorversions[NFSD_SUPPORTED_MINOR_VERSION + 1] = {
	[0] = 1,
	[1] = 1,
	[2] = 1,
};

but refuses to let it be disabled:
				if (num != 4)
					return -EINVAL;
				minor = simple_strtoul(minorp+1, NULL, 0);
				if (minor == 0)
					return -EINVAL;

so you can have 4.1 disabled while 4.0 and 4.2 are active, but you
cannot disable 4.0 as well.

Should that be fixed?  Do we care?

At least we should change
					int i = atoi(p+1);
					if (i > NFS4_MAXMINOR) {
						fprintf(stderr, "%s: unsupported minor version\n", optarg);
						exit(1);

in nfsd.c so the test is
  if (i < NFS4_MINMINOR || i > NFS4_MAXMINOR) {

I'll send a patch.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 03/15] Add /etc/nfs.conf support to rpc.nfsd

[J. Bruce Fields]

On Wed, Dec 21, 2016 at 10:22:24AM +1100, NeilBrown wrote:
> On Wed, Dec 07 2016, NeilBrown wrote:
> 
> > [ Unknown signature status ]
> > On Wed, Dec 07 2016, Steve Dickson wrote:
> >
> >>> +.B nfsd
> >>> +Recognized values:
> >>> +.BR threads ,
> >>> +.BR grace-time ,
> >>> +.BR lease-time ,
> >>> +.BR udp ,
> >>> +.BR tcp ,
> >>> +.BR vers2 ,
> >>> +.BR vers3 ,
> >>> +.BR vers4 ,
> >>> +.BR vers4.0 ,
> >> Do we need both ver4 and ver4.0?
> >
> > "vers4" allows you to enable or disable v4 has a whole.
> > The assumption is that if enabled, all minor versions that the kernel
> > supports will be enabled by default.
> > vers4.x allows individual minor versions to be disabled, so
> >  vers4=yes
> >  vers4.0=no
> > would disable v4.0, just like "-V4 -N4.0"
> 
> What I meant here, of course, is
>    just like "-V4 -N4.0" *should*
> 
> not "just like what it *does*."
> 
> To my surprise, when you actually try "nfsd -N4.0",
> it opens /proc/fs/nfsd/versions and writes "+4.32".
> 
> #define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1))) 
> 
> That "-1" makes it clear that version 4.0 isn't understood.
> 
> The kernel side seem to understand that '0' is an independent minor version:
> static bool nfsd_supported_minorversions[NFSD_SUPPORTED_MINOR_VERSION + 1] = {
> 	[0] = 1,
> 	[1] = 1,
> 	[2] = 1,
> };
> 
> but refuses to let it be disabled:
> 				if (num != 4)
> 					return -EINVAL;
> 				minor = simple_strtoul(minorp+1, NULL, 0);
> 				if (minor == 0)
> 					return -EINVAL;
> 
> so you can have 4.1 disabled while 4.0 and 4.2 are active, but you
> cannot disable 4.0 as well.
> 
> Should that be fixed?  Do we care?

It's not going to keep me up at night, but we should probably go ahead
and fix it, I can imagine somebody might care some day.

--b.

> 
> At least we should change
> 					int i = atoi(p+1);
> 					if (i > NFS4_MAXMINOR) {
> 						fprintf(stderr, "%s: unsupported minor version\n", optarg);
> 						exit(1);
> 
> in nfsd.c so the test is
>   if (i < NFS4_MINMINOR || i > NFS4_MAXMINOR) {
> 
> I'll send a patch.
> 
> Thanks,
> NeilBrown

[PATCH 05/15] Add /etc/nfs.conf support for statd

[NeilBrown]

Some options appear in the [lockd] section.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/nfs.conf.man  |   25 +++++++++++++++++++++++++
 utils/statd/statd.c   |   20 ++++++++++++++++++++
 utils/statd/statd.man |   35 +++++++++++++++++++++++++++++++++--
 3 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 9fe9d0eff1fc..a1121e46ef5e 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -118,6 +118,31 @@ section, are used to configure mountd.  See
 .BR rpc.mountd (8)
 for details.
 
+.TP
+.B statd
+Recognized values:
+.BR port ,
+.BR outgoing-port ,
+.BR name ,
+.BR state-directory-path ,
+.BR ha-callout .
+
+See
+.BR rpc.statd (8)
+for details.
+
+.TP
+.B lockd
+Recognized values:
+.B port
+and
+.BR udp-port .
+
+See
+.BR rpc.statd (8)
+for details.
+
+
 .SH FILES
 .I /etc/nfs.conf
 .SH SEE ALSO
diff --git a/utils/statd/statd.c b/utils/statd/statd.c
index 15f2b18d104d..5f4ad79e6bf3 100644
--- a/utils/statd/statd.c
+++ b/utils/statd/statd.c
@@ -26,6 +26,7 @@
 #include <sys/wait.h>
 #include <grp.h>
 
+#include "conffile.h"
 #include "statd.h"
 #include "nfslib.h"
 #include "nfsrpc.h"
@@ -36,6 +37,7 @@
 #include <sys/socket.h>
 
 int	run_mode = 0;		/* foreground logging mode */
+char	*conf_path = NFS_CONFFILE;
 
 /* LH - I had these local to main, but it seemed silly to have 
  * two copies of each - one in main(), one static in log.c... 
@@ -242,6 +244,7 @@ static void set_nlm_port(char *type, int port)
 int main (int argc, char **argv)
 {
 	extern char *optarg;
+	char *s;
 	int pid;
 	int arg;
 	int port = 0, out_port = 0;
@@ -266,6 +269,23 @@ int main (int argc, char **argv)
 	/* Set hostname */
 	MY_NAME = NULL;
 
+	conf_init();
+	out_port = conf_get_num("statd", "outgoing-port", out_port);
+	port = conf_get_num("statd", "port", port);
+	MY_NAME = conf_get_str("statd", "name");
+	if (MY_NAME)
+		run_mode |= STATIC_HOSTNAME;
+	s = conf_get_str("statd", "state-directory-path");
+	if (s && !nsm_setup_pathnames(argv[0], s))
+		exit(1);
+	s = conf_get_str("statd", "ha-callout");
+	if (s)
+		ha_callout_prog = s;
+
+	nlm_tcp = conf_get_num("lockd", "port", nlm_tcp);
+	/* udp defaults to the same as tcp ! */
+	nlm_udp = conf_get_num("lockd", "udp-port", nlm_tcp);
+
 	/* Process command line switches */
 	while ((arg = getopt_long(argc, argv, "h?vVFNH:dn:p:o:P:LT:U:", longopts, NULL)) != EOF) {
 		switch (arg) {
diff --git a/utils/statd/statd.man b/utils/statd/statd.man
index 1e5520c6dfea..91c260f1bf5e 100644
--- a/utils/statd/statd.man
+++ b/utils/statd/statd.man
@@ -8,7 +8,7 @@
 .\" Rewritten by Chuck Lever <chuck.lever@oracle.com>, 2009.
 .\" Copyright 2009 Oracle.  All rights reserved.
 .\"
-.TH RPC.STATD 8 "1 November 2009
+.TH RPC.STATD 8 "1 November 2009"
 .SH NAME
 rpc.statd \- NSM service daemon
 .SH SYNOPSIS
@@ -247,7 +247,7 @@ should listen on for
 .B NLM
 requests.
 .TP
-.BI "\-P, " "" \-\-state\-directory\-path " pathname
+.BI "\-P, " "" \-\-state\-directory\-path " pathname"
 Specifies the pathname of the parent directory
 where NSM state information resides.
 If this option is not specified,
@@ -267,6 +267,37 @@ Causes
 to display version information on
 .I stderr
 and then exit.
+.SH CONFIGURATION FILE
+Many of the options that can be set on the command line can also be
+controlled through values set in the
+.B [statd]
+or, in some cases, the
+.B [lockd]
+sections of the
+.I /etc/nfs.conf
+configuration file.
+Values recognized in the
+.B [statd]
+section include
+.BR port ,
+.BR outgoing-port ,
+.BR name ,
+.BR state-directory-path ", and"
+.B ha-callout
+which each have the same effect as the option with the same name.
+
+The values recognized in the
+.B [lockd]
+section include
+.B port
+and
+.B udp-port
+which have the same effect as the
+.B --nlm-port
+and
+.B --nlm-udp-port
+options, respectively.
+
 .SH SECURITY
 The
 .B rpc.statd

[PATCH 04/15] Add /etc/nfs.conf support for mountd.

[NeilBrown]

Some values are taken from the [nfsd] section
to ensure consistency.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/nfs.conf.man    |   22 +++++++++++++++++++++-
 utils/mountd/mountd.c   |   36 ++++++++++++++++++++++++++++++++++++
 utils/mountd/mountd.man |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 6ac6c65a81d9..9fe9d0eff1fc 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -92,16 +92,36 @@ Recognized values:
 .BR vers4.2 ,
 .BR rdma .
 
-Version and protocol values are Boolean values as described above.
+Version and protocol values are Boolean values as described above,
+and are also used by
+.BR rpc.mountd .
 Threads and the two times are integers.
 .B rdma
 is a service name or number.  See
 .BR rpc.nfsd (8)
 for details.
 
+.TP
+.B mountd
+Recognized values:
+.BR manage-gids ,
+.BR descriptors ,
+.BR port ,
+.BR threads ,
+.BR reverse-lookup ,
+.BR state-directory-path ,
+.BR ha-callout .
+
+These, together with the protocol and version values in the
+.B [nfsd]
+section, are used to configure mountd.  See
+.BR rpc.mountd (8)
+for details.
+
 .SH FILES
 .I /etc/nfs.conf
 .SH SEE ALSO
 .BR nfsdcltrack (8),
 .BR rpc.nfsd (8),
+.BR rpc.mountd (8),
 .BR nfsmount.conf (5).
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index d6cebbbd3920..2048fce92b8e 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -22,6 +22,8 @@
 #include <fcntl.h>
 #include <sys/resource.h>
 #include <sys/wait.h>
+
+#include "conffile.h"
 #include "xmalloc.h"
 #include "misc.h"
 #include "mountd.h"
@@ -38,6 +40,8 @@ int reverse_resolve = 0;
 int manage_gids;
 int use_ipaddr = -1;
 
+char *conf_path = NFS_CONFFILE;
+
 /* PRC: a high-availability callout program can be specified with -H
  * When this is done, the program will receive callouts whenever clients
  * send mount or unmount requests -- the callout is not needed for 2.6 kernel */
@@ -654,6 +658,7 @@ main(int argc, char **argv)
 {
 	char    *state_dir = NFS_STATEDIR;
 	char	*progname;
+	char	*s;
 	unsigned int listeners = 0;
 	int	foreground = 0;
 	int	port = 0;
@@ -669,6 +674,37 @@ main(int argc, char **argv)
 	else
 		progname = argv[0];
 
+	conf_init();
+	manage_gids = conf_get_bool("mountd", "manage-gids", manage_gids);
+	descriptors = conf_get_num("mountd", "descriptors", descriptors);
+	port = conf_get_num("mountd", "port", port);
+	num_threads = conf_get_num("mountd", "threads", num_threads);
+	reverse_resolve = conf_get_bool("mountd", "reverse-lookup", reverse_resolve);
+	ha_callout_prog = conf_get_str("mountd", "ha-callout");
+
+	s = conf_get_str("mountd", "state-directory-path");
+	if (s)
+		state_dir = s;
+
+	/* NOTE: following uses "nfsd" section of nfs.conf !!!! */
+	if (conf_get_bool("nfsd", "udp", NFSCTL_UDPISSET(_rpcprotobits)))
+		NFSCTL_UDPSET(_rpcprotobits);
+	else
+		NFSCTL_UDPUNSET(_rpcprotobits);
+	if (conf_get_bool("nfsd", "tcp", NFSCTL_TCPISSET(_rpcprotobits)))
+		NFSCTL_TCPSET(_rpcprotobits);
+	else
+		NFSCTL_TCPUNSET(_rpcprotobits);
+	for (vers = 2; vers <= 4; vers++) {
+		char tag[10];
+		sprintf(tag, "vers%d", vers);
+		if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(nfs_version, vers)))
+			NFSCTL_VERSET(nfs_version, vers);
+		else
+			NFSCTL_VERUNSET(nfs_version, vers);
+	}
+
+
 	/* Parse the command line options and arguments. */
 	opterr = 0;
 	while ((c = getopt_long(argc, argv, "o:nFd:p:P:hH:N:V:vurs:t:g", longopts, NULL)) != EOF)
diff --git a/utils/mountd/mountd.man b/utils/mountd/mountd.man
index e0d1a0acba3a..9f0a51f2016e 100644
--- a/utils/mountd/mountd.man
+++ b/utils/mountd/mountd.man
@@ -199,6 +199,39 @@ the server. Note that the 'primary' group id is not affected so a
 .B newgroup
 command on the client will still be effective.  This function requires
 a Linux Kernel with version at least 2.6.21.
+
+.SH CONFIGURATION FILE
+Many of the options that can be set on the command line can also be
+controlled through values set in the
+.B [mountd]
+or, in some cases, the
+.B [nfsd]
+sections of the
+.I /etc/nfs.conf
+configuration file.
+Values recognized in the
+.B [mountd]
+section include
+.BR manage-gids ,
+.BR descriptors ,
+.BR port ,
+.BR threads ,
+.BR reverse-lookup ", and"
+.BR state-directory-path ,
+.B ha-callout
+which each have the same effect as the option with the same name.
+
+The values recognized in the
+.B [nfsd]
+section include
+.BR TCP ,
+.BR UDP ,
+.BR vers2 ,
+.BR vers3 ", and"
+.B vers4
+which each have same same meaning as given by
+.BR rpc.nfsd (8).
+
 .SH TCP_WRAPPERS SUPPORT
 You can protect your
 .B rpc.mountd
@@ -253,6 +286,7 @@ table of clients accessing server's exports
 .BR rpc.nfsd (8),
 .BR rpc.rquotad (8),
 .BR nfs (5),
+.BR nfs.conf (5),
 .BR tcpd (8),
 .BR hosts_access (5),
 .BR iptables (8),

[PATCH 01/15] Add man-page describing /etc/nfs.conf

[NeilBrown]

It may seem a little odd placing this in the "systemd" directory,
but it is a conveninent place, and /etc/nfs.conf was added in
part to help with systemd integration.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/Makefile.am  |    4 ++-
 systemd/nfs.conf.man |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 systemd/nfs.conf.man

diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 49c9b8d3b459..b647ef00a0d1 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -36,7 +36,9 @@ unit_files += \
 endif
 endif
 
-EXTRA_DIST = $(unit_files)
+man5_MANS	= nfs.conf.man
+
+EXTRA_DIST = $(unit_files) $(man5_MANS)
 
 unit_dir = /usr/lib/systemd/system
 generator_dir = /usr/lib/systemd/system-generators
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
new file mode 100644
index 000000000000..3dd56f735de2
--- /dev/null
+++ b/systemd/nfs.conf.man
@@ -0,0 +1,67 @@
+.TH NFS.CONF 5
+.SH NAME
+nfs.conf \- general configuration for NFS daemons and tools
+.SH SYNOPSIS
+.I /etc/nfs.conf
+.SH DESCRIPTION
+.PP
+This file contains site-specific configuration for various NFS daemons
+and other processes.  Most configuration can also be passed to
+processes via command line arguments, but it can be more convenient to
+have a central file.  In particular, this encourages consistent
+configuration across different processes.
+.PP
+When command line options are provided, they override values set in
+this file.  When this file does not specify a particular parameter,
+and no command line option is provided, each tool provides its own
+default values.
+.PP
+The file format supports multiple sections, each of which can contain
+multiple value assignments.  A section is introduced by a line
+containing the section name enclosed in square brackets, so
+.RS
+.B [global]
+.RE
+would introduce a section called
+.BR global .
+A value assignment is a single line that has the name of the value, an
+equals sign, and a setting for the value, so
+.RS
+.B threads = 4
+.RE
+would set the value named
+.B threads
+in the current section to
+.BR 4 .
+Leading and trailing spaces and tab
+are ignored, as are spaces and tabs surrounding the equals sign.
+.PP
+Any line starting with
+.RB \*(lq # \*(rq
+or
+.RB \*(lq ; \*(rq
+is ignored, as is any blank line.
+.PP
+Lookup of section and value names is case-insensitive.
+
+.SH SECTIONS
+The following sections are known to various programs, and can contain
+the given named values.
+.TP
+.B nfsdcltrack
+Recognized values:
+.BR storagedir .
+
+The
+.B nfsdcltrack
+program is run directly by the Linux kernel and there is no
+opportunity to provide command line arguments, so the configuration
+file is the only way to configure this program.  See
+.BR nfsdcltrack (8)
+for details.
+
+.SH FILES
+.I /etc/nfs.conf
+.SH SEE ALSO
+.BR nfsdcltrack (8),
+.BR nfsmount.conf (5).

[PATCH 02/15] conffile: add bool support

[NeilBrown]

conf_get_bool() interprets various strings as 'true' or 'false'.
If no suitable value is found, the default is returned.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/include/conffile.h |    2 ++
 support/nfs/conffile.c     |   32 ++++++++++++++++++++++++++++++++
 systemd/nfs.conf.man       |   16 ++++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/support/include/conffile.h b/support/include/conffile.h
index a6d5846f6720..3fe3a78230f6 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -36,6 +36,7 @@
 #include <sys/queue.h>
 #include <ctype.h>
 #include <stdint.h>
+#include <stdbool.h>
 
 struct conf_list_node {
 	TAILQ_ENTRY(conf_list_node) link;
@@ -57,6 +58,7 @@ extern struct sockaddr *conf_get_address(char *, char *);
 extern struct conf_list *conf_get_list(char *, char *);
 extern struct conf_list *conf_get_tag_list(char *, char *);
 extern int      conf_get_num(char *, char *, int);
+extern _Bool    conf_get_bool(char *, char *, _Bool);
 extern char    *conf_get_str(char *, char *);
 extern char    *conf_get_section(char *, char *, char *);
 extern void     conf_init(void);
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 6b94ec0b5b53..609d78294f35 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -446,6 +446,38 @@ conf_get_num(char *section, char *tag, int def)
 	return def;
 }
 
+/*
+ * Return the Boolean value denoted by TAG in section SECTION, or DEF
+ * if that tags does not exist.
+ * FALSE is returned for case-insensitve comparisons with 0, f, false, n, no, off
+ * TRUE is returned for 1, t, true, y, yes, on
+ * A failure to match one of these results in DEF
+ */
+_Bool
+conf_get_bool(char *section, char *tag, _Bool def)
+{
+	char *value = conf_get_str(section, tag);
+
+	if (!value)
+		return def;
+	if (strcasecmp(value, "1") == 0 ||
+	    strcasecmp(value, "t") == 0 ||
+	    strcasecmp(value, "true") == 0 ||
+	    strcasecmp(value, "y") == 0 ||
+	    strcasecmp(value, "yes") == 0 ||
+	    strcasecmp(value, "on") == 0)
+		return true;
+
+	if (strcasecmp(value, "0") == 0 ||
+	    strcasecmp(value, "f") == 0 ||
+	    strcasecmp(value, "false") == 0 ||
+	    strcasecmp(value, "n") == 0 ||
+	    strcasecmp(value, "no") == 0 ||
+	    strcasecmp(value, "off") == 0)
+		return false;
+	return def;
+}
+
 /* Validate X according to the range denoted by TAG in section SECTION.  */
 int
 conf_match_num(char *section, char *tag, int x)
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 3dd56f735de2..1f524d8fe74e 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -44,6 +44,22 @@ is ignored, as is any blank line.
 .PP
 Lookup of section and value names is case-insensitive.
 
+Where a Boolean value is expected, any of
+.BR true ,
+.BR t ,
+.BR yes ,
+.BR y ,
+.BR on ", or"
+.B 1
+can be used for "true", while
+.BR false ,
+.BR f ,
+.BR no ,
+.BR n ,
+.BR off ", or"
+.B 0
+can be used for "false".  Comparisons are case-insensitive.
+
 .SH SECTIONS
 The following sections are known to various programs, and can contain
 the given named values.

[PATCH 09/15] conffile: add support for include files.

[NeilBrown]

A tag

  include = filename

will be replaced by the content of the file.
This must appear after a section heading, and any assignments
not in their own section will be included in the section that this
directive is in.
e.g

 [environment]
  include = /etc/sysconfig/nfs

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/nfs/conffile.c |    8 ++++++--
 systemd/nfs.conf.man   |   13 +++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 965726c74f6b..8de580bd6970 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -52,6 +52,7 @@
 #pragma GCC visibility push(hidden)
 
 static void conf_load_defaults(void);
+static int conf_load(int trans, char *path);
 static int conf_set(int , char *, char *, char *, 
 	char *, int , int );
 
@@ -308,8 +309,11 @@ conf_parse_line(int trans, char *line, size_t sz)
 					break;
 				}
 			}
-			/* XXX Perhaps should we not ignore errors?  */
-			conf_set(trans, section, arg, line, val, 0, 0);
+			if (strcasecmp(line, "include") == 0)
+				conf_load(trans, val);
+			else
+				/* XXX Perhaps should we not ignore errors?  */
+				conf_set(trans, section, arg, line, val, 0, 0);
 			return;
 		}
 	}
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 726f603544b7..13459ea85744 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -42,6 +42,19 @@ or
 .RB \*(lq ; \*(rq
 is ignored, as is any blank line.
 .PP
+The value name
+.B include
+is special.  If a section contains
+.RS
+.B include = /some/file/name
+.RE
+then the named file will be read, and any value assignments found
+there-in will be added to the current section.  If the file contains
+section headers, then new sections will be created just as if the
+included file appeared in place of the
+.B include
+line.
+.PP
 Lookup of section and value names is case-insensitive.
 
 Where a Boolean value is expected, any of

[PATCH 11/15] conffile: ignore setting of empty string

[NeilBrown]

A value setting like

   foo =

is now equivalent to not setting "foo" at all.
This is likely to be least confusing.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/nfs/conffile.c |    2 ++
 systemd/nfs.conf.man   |    3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 947bf9bad1e7..eaff5f5c35ea 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -748,6 +748,8 @@ conf_set(int transaction, char *section, char *arg,
 {
 	struct conf_trans *node;
 
+	if (!value || !*value)
+		return 0;
 	node = conf_trans_node(transaction, CONF_SET);
 	if (!node)
 		return 1;
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 8e2f034d0b9b..8cf55668b664 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -36,7 +36,8 @@ in the current section to
 Leading and trailing spaces and tab
 are ignored, as are spaces and tabs surrounding the equals sign.
 Single and double quotes surrounding the assigned value are also
-removed.
+removed.  If the resulting string is empty, the whole assignment
+is ignored.
 .PP
 Any line starting with
 .RB \*(lq # \*(rq

[PATCH 06/15] Add /etc/nfs.conf support for sm-notify

[NeilBrown]

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/nfs.conf.man      |   10 ++++++++++
 utils/statd/sm-notify.c   |   11 +++++++++++
 utils/statd/sm-notify.man |   27 +++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index a1121e46ef5e..726f603544b7 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -142,6 +142,16 @@ See
 .BR rpc.statd (8)
 for details.
 
+.TP
+.B sm-notify
+Recognized values:
+.BR retry-time ,
+.BR outgoing-port ", and"
+.BR outgoing-addr .
+
+See
+.BR sm-notify (8)
+for details.
 
 .SH FILES
 .I /etc/nfs.conf
diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 8afddd97da73..19f40afcb376 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -29,6 +29,7 @@
 #include <errno.h>
 #include <grp.h>
 
+#include "conffile.h"
 #include "sockaddr.h"
 #include "xlog.h"
 #include "nsm.h"
@@ -66,6 +67,7 @@ static _Bool		opt_update_state = true;
 static unsigned int	opt_max_retry = 15 * 60;
 static char *		opt_srcaddr = NULL;
 static char *		opt_srcport = NULL;
+char *			conf_path = NFS_CONFFILE;
 
 static void		notify(const int sock);
 static int		notify_host(int, struct nsm_host *);
@@ -479,6 +481,7 @@ main(int argc, char **argv)
 {
 	int	c, sock, force = 0;
 	char *	progname;
+	char *	s;
 
 	progname = strrchr(argv[0], '/');
 	if (progname != NULL)
@@ -486,6 +489,14 @@ main(int argc, char **argv)
 	else
 		progname = argv[0];
 
+	conf_init();
+	opt_max_retry = conf_get_num("sm-notify", "retry-time", opt_max_retry / 60) * 60;
+	opt_srcport = conf_get_str("sm-notify", "outgoing-port");
+	opt_srcaddr = conf_get_str("sm-notify", "outgoing-addr");
+	s = conf_get_str("statd", "state-directory-path");
+	if (s && !nsm_setup_pathnames(argv[0], s))
+		exit(1);
+
 	while ((c = getopt(argc, argv, "dm:np:v:P:f")) != -1) {
 		switch (c) {
 		case 'f':
diff --git a/utils/statd/sm-notify.man b/utils/statd/sm-notify.man
index 7a1cbfae998f..bb7f6e0a1420 100644
--- a/utils/statd/sm-notify.man
+++ b/utils/statd/sm-notify.man
@@ -219,6 +219,33 @@ argument when sending SM_NOTIFY requests.
 .IP
 This option can be useful in multi-homed configurations where
 the remote requires notification from a specific network address.
+.SH CONFIGURATION FILE
+Many of the options that can be set on the command line can also be
+controlled through values set in the
+.B [sm-notify]
+or, in one case, the
+.B [statd]
+section of the
+.I /etc/nfs.conf
+configuration file.
+
+Values recognized in the
+.B [sm-notify]
+section include:
+.BR retry-time ,
+.BR outgoing-port ", and"
+.BR outgoing-addr .
+These have the same effect as the command line options
+.BR m ,
+.BR p ", and"
+.B v
+respectively.
+
+The value recognized in the
+.B [statd]
+section is
+.BR state-directory-path .
+
 .SH SECURITY
 The
 .B sm-notify

[PATCH 10/15] conffile: strip "quotes" from values in conf file.

[NeilBrown]

When "include = " is used to read and "environment" file such as
/etc/sysconfig/nfs, there might be quotes around values.
Stripe those off, just like a 'shell' reading the file would.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/nfs/conffile.c |   25 ++++++++++++++++---------
 systemd/nfs.conf.man   |    2 ++
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 8de580bd6970..947bf9bad1e7 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -212,7 +212,7 @@ static void
 conf_parse_line(int trans, char *line, size_t sz)
 {
 	char *val, *ptr;
-	size_t i, valsize;
+	size_t i;
 	size_t j;
 	static char *section = 0;
 	static char *arg = 0;
@@ -299,16 +299,23 @@ conf_parse_line(int trans, char *line, size_t sz)
 			}
 			line[strcspn (line, " \t=")] = '\0';
 			val = line + i + 1 + strspn (line + i + 1, " \t");
-			valsize = 0;
-			while (val[valsize++]);
 
-			/* Skip trailing spaces and comments */
-			for (j = 0; j < valsize; j++) {
-				if (val[j] == '#' || val[j] == ';' || isspace(val[j])) {
-					val[j] = '\0';
-					break;
+			if (line[0] == '"') {
+				line ++;
+				j = strcspn(line, "\"");
+				line[j] = 0;
+			} else if (line[0] == '\'') {
+				line ++;
+				j = strcspn(line, "'");
+				line[j] = 0;
+			} else
+				/* Skip trailing spaces and comments */
+				for (j = 0; val[j]; j++) {
+					if (val[j] == '#' || val[j] == ';' || isspace(val[j])) {
+						val[j] = '\0';
+						break;
+					}
 				}
-			}
 			if (strcasecmp(line, "include") == 0)
 				conf_load(trans, val);
 			else
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 13459ea85744..8e2f034d0b9b 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -35,6 +35,8 @@ in the current section to
 .BR 4 .
 Leading and trailing spaces and tab
 are ignored, as are spaces and tabs surrounding the equals sign.
+Single and double quotes surrounding the assigned value are also
+removed.
 .PP
 Any line starting with
 .RB \*(lq # \*(rq

[PATCH 08/15] conffile: split loading of file into a separate function.

[NeilBrown]

This will make support of include files easier.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/nfs/conffile.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index e4597507b922..965726c74f6b 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -366,23 +366,18 @@ conf_init (void)
 	conf_reinit();
 }
 
-/* Open the config file and map it into our address space, then parse it.  */
-void
-conf_reinit(void)
+static int
+conf_load(int trans, char *path)
 {
-	struct conf_binding *cb = 0;
-	int fd, trans;
-	unsigned int i;
-	size_t sz;
-	char *new_conf_addr = 0;
 	struct stat sb;
+	if ((stat (path, &sb) == 0) || (errno != ENOENT)) {
+		char *new_conf_addr;
+		size_t sz = sb.st_size;
+		int fd = open (path, O_RDONLY, 0);
 
-	if ((stat (conf_path, &sb) == 0) || (errno != ENOENT)) {
-		sz = sb.st_size;
-		fd = open (conf_path, O_RDONLY, 0);
 		if (fd == -1) {
-			xlog_warn("conf_reinit: open (\"%s\", O_RDONLY) failed", conf_path);
-			return;
+			xlog_warn("conf_reinit: open (\"%s\", O_RDONLY) failed", path);
+			return -1;
 		}
 
 		new_conf_addr = malloc(sz);
@@ -399,13 +394,28 @@ conf_reinit(void)
 		}
 		close(fd);
 
-		trans = conf_begin();
 		/* XXX Should we not care about errors and rollback?  */
 		conf_parse(trans, new_conf_addr, sz);
 		free(new_conf_addr);
+		return 0;
+	fail:
+		close(fd);
+		free(new_conf_addr);
 	}
-	else
-		trans = conf_begin();
+	return -1;
+}
+
+/* Open the config file and map it into our address space, then parse it.  */
+void
+conf_reinit(void)
+{
+	struct conf_binding *cb = 0;
+	int trans;
+	unsigned int i;
+
+	trans = conf_begin();
+	if (conf_load(trans, conf_path) < 0)
+		return;
 
 	/* Load default configuration values.  */
 	conf_load_defaults();
@@ -419,11 +429,6 @@ conf_reinit(void)
 
 	conf_end(trans, 1);
 	return;
-
-fail:
-	if (new_conf_addr)
-		free(new_conf_addr);
-	close (fd);
 }
 
 /*

[PATCH 07/15] conffile: free image of config file after parsing

[NeilBrown]

conffile allocates memory and reads in the config file.
It then parses the file, using strdup() to take a copy of
any string that it uses, so after conf_parse() there are no
references in to the allocated file image.

conffile does not free this image.  It keeps a pointer, but never uses
it in an interesing way, and never frees it.  This is a little clumsy
and interfers with a future patch which will support the inclusion of
subordinate config files.

So free 'new_conf_addr' when finished with it, and discard the
'conf_addr' variable that stored it.
This has an insignificant performance consequence in that we node
always free everything in the hash table, even when we know it must
be empty.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/nfs/conffile.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 609d78294f35..e4597507b922 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -107,8 +107,6 @@ struct conf_binding {
 char *conf_path;
 LIST_HEAD (conf_bindings, conf_binding) conf_bindings[256];
 
-static char *conf_addr;
-
 static __inline__ uint8_t
 conf_hash(char *s)
 {
@@ -396,7 +394,7 @@ conf_reinit(void)
 		/* XXX I assume short reads won't happen here.  */
 		if (read (fd, new_conf_addr, sz) != (int)sz) {
 			xlog_warn("conf_reinit: read (%d, %p, %lu) failed",
-   				fd, new_conf_addr, (unsigned long)sz);
+				fd, new_conf_addr, (unsigned long)sz);
 			goto fail;
 		}
 		close(fd);
@@ -404,6 +402,7 @@ conf_reinit(void)
 		trans = conf_begin();
 		/* XXX Should we not care about errors and rollback?  */
 		conf_parse(trans, new_conf_addr, sz);
+		free(new_conf_addr);
 	}
 	else
 		trans = conf_begin();
@@ -412,17 +411,13 @@ conf_reinit(void)
 	conf_load_defaults();
 
 	/* Free potential existing configuration.  */
-	if (conf_addr) {
-		for (i = 0; i < sizeof conf_bindings / sizeof conf_bindings[0]; i++) {
-			cb = LIST_FIRST (&conf_bindings[i]);
-			for (; cb; cb = LIST_FIRST (&conf_bindings[i]))
-				conf_remove_now(cb->section, cb->tag);
-		}
-		free (conf_addr);
+	for (i = 0; i < sizeof conf_bindings / sizeof conf_bindings[0]; i++) {
+		cb = LIST_FIRST (&conf_bindings[i]);
+		for (; cb; cb = LIST_FIRST (&conf_bindings[i]))
+			conf_remove_now(cb->section, cb->tag);
 	}
 
 	conf_end(trans, 1);
-	conf_addr = new_conf_addr;
 	return;
 
 fail:

[PATCH 15/15] Add nfs.systemd man page

[NeilBrown]

This discusses some of the behaviors of the various
unit files, and how best to work with them to achieve
various results.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/Makefile.am     |    4 +
 systemd/nfs.systemd.man |  167 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+), 2 deletions(-)
 create mode 100644 systemd/nfs.systemd.man

diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index b5ab157d728b..0d15b9f25ff8 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -36,8 +36,8 @@ endif
 endif
 
 man5_MANS	= nfs.conf.man
-
-EXTRA_DIST = $(unit_files) $(man5_MANS)
+man7_MANS	= nfs.systemd.man
+EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
 
 unit_dir = /usr/lib/systemd/system
 generator_dir = /usr/lib/systemd/system-generators
diff --git a/systemd/nfs.systemd.man b/systemd/nfs.systemd.man
new file mode 100644
index 000000000000..01801ebb5f2c
--- /dev/null
+++ b/systemd/nfs.systemd.man
@@ -0,0 +1,167 @@
+.TH NFS.SYSTEMD 7
+.SH NAME
+nfs.systemd \- managing NFS services through systemd.
+.SH SYNOPSIS
+nfs-utils.service
+.br
+nfs-server.service
+.br
+nfs-client.target
+.br
+.I etc
+.SH DESCRIPTION
+The
+.I nfs-utils
+package provides a suite of
+.I systemd
+unit files which allow the various services to be started and
+managed.  These unit files ensure that the services are started in the
+correct order, and the prerequisites are active before dependant
+services start.  As there are quite  few unit files, it is not
+immediately obvious how best to achieve certain results.  The
+following subsections attempt to cover the issues that are most likely
+to come up.
+.SS Configuration
+The standard systemd unit files do not provide any easy way to pass
+any command line arguments to daemons so as to configure their
+behavior.  In many case such configuration can be performed by making
+changes to
+.I /etc/nfs.conf
+or other configuration files.  When that is not convenient, a
+distribution might provide systemd "drop-in" files which replace the
+.B ExecStart=
+setting to start the program with different arguments.  For example a
+drop-in file
+.B systemd/system/nfs-mountd.service.d/local.conf
+containing
+.RS
+.nf
+[Service]
+EnvironmentFile=/etc/sysconfig/nfs
+ExecStart=
+ExecStart= /usr/sbin/rpc.mountd $RPCMOUNTDOPTS
+.fi
+.RE
+would cause the
+.B nfs-mountd.service
+unit to run the
+.I rpc.mountd
+program using, for arguments, the value given for
+.B RPCMOUNTDOPTS
+in
+.IR /etc/sysconfig/nfs .
+This allows for seamless integration with existing configuration
+tools.
+.SS Enabling unit files
+There are three unit files which are designed to be manually enabled.
+All others are automatically run as required.  The three are:
+.TP
+.B nfs-client.target
+This should be enabled on any host which ever serves as an NFS client.
+There is little cost in transparently enabling it whenever NFS client
+software is installed.
+.TP
+.B nfs-server.service
+This must be enabled to provide NFS service to clients.  It starts and
+configures the required daemons in the required order.
+.TP
+.B nfs-blkmap.service
+The
+.B blkmapd
+daemon is only required on NFS clients which are using pNFS (parallel
+NFS), and particularly using the
+.B blocklayout
+layout protocol.  If you might use this particular extension to NFS,
+the
+.B nfs-blkmap.service
+unit should be enabled.
+.PP
+Several other units which might be considered to be optional, such as
+.I rpc-gssd.service
+are careful to only start if the required configuration file exists.
+.I rpc-gsdd.service
+will not start if the
+.I krb5.keytab
+file does not exist (typically in
+.IR /etc ).
+.SS Restarting NFS services
+Most NFS daemons can be restarted at any time.  They will reload any
+state that they need, and continue servicing requests.  This is rarely
+necessary though.
+.PP
+When configuration changesare make, it can be hard to know exactly
+which services need to be restarted to ensure that the configuration
+takes effect.  The simplest approach, which is often the best, is to
+restart everything.  To help with this, the
+.B nfs-utils.service
+unit is provided.  It declares appropriate dependencies with other
+unit files so that
+.RS
+.B systemctl restart nfs-utils
+.RE
+will restart all NFS daemons that are running.  This will cause all
+configuration changes to take effect
+.I except
+for changes to mount options lists in
+.I /etc/fstab
+or
+.IR /etc/nfsmount.conf .
+Mount options can only be changed by unmounting and remounting
+filesystem.  This can be a disruptive operation so it should only be
+done when the value justifies the cost.  The command
+.RS
+.B umount -a -t nfs; mount -a -t nfs
+.RE
+should unmount and remount all NFS filesystems.
+.SS Masking unwanted services
+Rarely there may be a desire to prohibit some services from running
+even though there are normally part of a working NFS system.  This may
+be needed to reduce system load to an absolute minimum, or to reduce
+attack surface by not running daemons that are not absolutely
+required.
+.PP
+Two particular services which this can apply to are
+.I rpcbind
+and
+.IR idmapd .
+.I rpcbind
+is not part of the
+.I nfs-utils
+package, but it used by several NFS services.  However it is
+.B not
+needed when only NFSv4 is in use.  If a site will never use NFSv3 (or
+NFSv2) and does not want
+.I rpcbind
+to be running, the correct approach is to run
+.RS
+.B systemctl mask rpcbind
+.RE
+This will disable
+.IR rpcbind ,
+and the various NFS services which depend on it (and are only needed
+for NFSv3) will refuse to start, without interfering with the
+operation of NFSv4 services.  In particular,
+.I rpc.statd
+will not run when
+.I rpcbind
+is masked.
+.PP
+.I idmapd
+is only needed for NFSv4, and even then is not needed when the client
+and server agree to use user-ids rather than user-names to identify the
+owners of files.  If
+.I idmapd
+is not needed and not wanted, it can be masked with
+.RS
+.B systemctl mask idmapd
+.RE
+.SH FILES
+/etc/nfs.conf
+.br
+/etc/nfsmount.conf
+.br
+/etc/idmapd.conf
+.SH SEE ALSO
+.BR systemd.unit (5),
+.BR nfs.conf (5),
+.BR nfsmount.conf (5).

[PATCH 12/15] conffile: allow $name expansion of tag values.

[NeilBrown]

If the value for a tag starts with '$', then the remainder
of the value is treated as an environment variable name.
It is looked up in the environment (getenv) and if not found,
it is looked for in the [environment] section of the config file.

This lookup is formed as access time e.g. by conf_get_str(), not at
parse time.

The expected usage is that the config file can contain something like

 [environment]
  include = /etc/sysconfig/nfs
 [other-section]
  tag = $NAME

and conf_get_str("other-section","tag") will report the value of "NAME"
in the given file.

As different distributions used different environment files, and different
variable names with-in them, a distro could provide a static config file
which maps from names in that environment file to config tags requires
by NFS daemons.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 support/nfs/conffile.c |   16 ++++++++++++++--
 systemd/nfs.conf.man   |    8 ++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index eaff5f5c35ea..e4f685c558fa 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -519,12 +519,24 @@ char *
 conf_get_str(char *section, char *tag)
 {
 	struct conf_binding *cb;
-
+retry:
 	cb = LIST_FIRST (&conf_bindings[conf_hash (section)]);
 	for (; cb; cb = LIST_NEXT (cb, link)) {
 		if (strcasecmp (section, cb->section) == 0
-				&& strcasecmp (tag, cb->tag) == 0)
+		    && strcasecmp (tag, cb->tag) == 0) {
+			if (cb->value[0] == '$') {
+				/* expand $name from [environment] section,
+				 * or from environment
+				 */
+				char *env = getenv(cb->value+1);
+				if (env)
+					return env;
+				section = "environment";
+				tag = cb->value + 1;
+				goto retry;
+			}
 			return cb->value;
+		}
 	}
 	return 0;
 }
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 8cf55668b664..03f1f2be534d 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -45,6 +45,14 @@ or
 .RB \*(lq ; \*(rq
 is ignored, as is any blank line.
 .PP
+If the assigned value started with a
+.RB \*(lq $ \*(rq
+then the remainder is treated as a name and looked for in the section
+.B [environment]
+or in the processes environment (see
+.BR environ (7)).
+The value found is used for this value.
+.PP
 The value name
 .B include
 is special.  If a section contains

[PATCH 14/15] systemd: Remove the nfs-config.service

[NeilBrown]

Now that we have /etc/nfs.conf, a lot of configuration can be
read directly.  So nfs-config isn't really needed any more.

Some distributions allow command-line arguments for various
daemons to be set in an environment file (/etc/sysconfig, /etc/defaults).
Passing these through /etc/nfs.conf is not possible.

Instead, a distro that needs this functionality can create drop-in
files which select the required value.  As no commands are given
default arguments by systemd unit files, the drop-in can just add
distro-specific args.
For example
  /lib/systemd/system/nfs-mountd.service.d/local.conf
   [Service]
   EnvironmentFile=/etc/sysconfig/nfs
   ExecStart=
   ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDOPTS

Note the need for the empty assignment to remove existing definitions
first.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 configure.ac                     |    6 ------
 systemd/Makefile.am              |    1 -
 systemd/README                   |   22 +++++++++++++++-------
 systemd/nfs-blkmap.service       |    3 +--
 systemd/nfs-config.service.in    |   13 -------------
 systemd/nfs-idmapd.service       |    6 +-----
 systemd/nfs-mountd.service       |    6 +-----
 systemd/nfs-server.service       |    7 +------
 systemd/rpc-gssd.service.in      |    7 +------
 systemd/rpc-statd-notify.service |    6 +-----
 systemd/rpc-statd.service        |    6 +-----
 systemd/rpc-svcgssd.service      |    6 +-----
 12 files changed, 23 insertions(+), 66 deletions(-)
 delete mode 100644 systemd/nfs-config.service.in

diff --git a/configure.ac b/configure.ac
index 8a5aa2e5203b..b3388a62fe30 100644
--- a/configure.ac
+++ b/configure.ac
@@ -518,11 +518,6 @@ AC_SUBST([AM_CFLAGS], ["$my_am_cflags"])
 # Make sure that $ACLOCAL_FLAGS are used during a rebuild
 AC_SUBST([ACLOCAL_AMFLAGS], ["-I $ac_macro_dir \$(ACLOCAL_FLAGS)"])
 
-# make libexecdir available for substituion in config files
-# 2 "evals" needed late to expand variable names.
-AC_SUBST([_libexecdir])
-AC_CONFIG_COMMANDS_PRE([eval eval _libexecdir=$libexecdir])
-
 # make _sysconfdir available for substituion in config files
 # 2 "evals" needed late to expand variable names.
 AC_SUBST([_sysconfdir])
@@ -530,7 +525,6 @@ AC_CONFIG_COMMANDS_PRE([eval eval _sysconfdir=$sysconfdir])
 
 AC_CONFIG_FILES([
 	Makefile
-	systemd/nfs-config.service
 	systemd/rpc-gssd.service
 	linux-nfs/Makefile
 	support/Makefile
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index b647ef00a0d1..b5ab157d728b 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
 unit_files =  \
     nfs-client.target \
     \
-    nfs-config.service \
     nfs-mountd.service \
     nfs-server.service \
     nfs-utils.service \
diff --git a/systemd/README b/systemd/README
index 7c43df88438a..da23d6f63a91 100644
--- a/systemd/README
+++ b/systemd/README
@@ -19,8 +19,8 @@ by a suitable 'preset' setting:
     can work (if no type is given, ".service" is assumed).
 
  nfs-client.target
-    If enabled, daemons needs for an nfs client are enabled.
-    This does *not* include rpc.statd.  the rpc-statd.service unit
+    If enabled, daemons needed for an nfs client are enabled.
+    This does *not* include rpc.statd.  The rpc-statd.service unit
     is started by /usr/sbin/start-statd which mount.nfs will run
     if statd is needed.
 
@@ -52,11 +52,19 @@ It cannot stop rpc.statd or rpc.gssd as they may be in use by the
 client and systemd cannot specify is two-pronged reverse dependency.
 (i.e. stop this unit if none of these units are running)
 
-Distro specific commandline configuration can be provided by
-installing a script /usr/libexec/nfs-utils/nfs-utils_env.sh
-This should write /run/sysconfig/nfs-utils based on configuration
-information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
-It is run once by nfs-config.service.
+Distro specific configuration can be included in /etc/nfs.conf, or
+by providing drop-in files which replace the ExecStart line for a given
+service, and possibly add an EnvironmentFile line.
+
+For example, if systemd/system/nfs-mountd.service.d/local.conf
+contained
+  [Service]
+  EnvironmentFile=/etc/sysconfig/nfs
+  ExecStart=
+  ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDOPTS
+
+then the setting of RPCMOUNTDOPTS in /etc/sysconfig/nfs would be
+passed to rpc.mountd.
 
 rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
 is present.
diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
index ddbf4e91da46..ddc324e70679 100644
--- a/systemd/nfs-blkmap.service
+++ b/systemd/nfs-blkmap.service
@@ -10,8 +10,7 @@ PartOf=nfs-utils.service
 [Service]
 Type=forking
 PIDFile=/var/run/blkmapd.pid
-EnvironmentFile=-/run/sysconfig/nfs-utils
-ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
+ExecStart=/usr/sbin/blkmapd
 
 [Install]
 WantedBy=nfs-client.target
diff --git a/systemd/nfs-config.service.in b/systemd/nfs-config.service.in
deleted file mode 100644
index e89dc54209aa..000000000000
--- a/systemd/nfs-config.service.in
+++ /dev/null
@@ -1,13 +0,0 @@
-[Unit]
-Description=Preprocess NFS configuration
-After=local-fs.target
-DefaultDependencies=no
-
-[Service]
-Type=oneshot
-# This service needs to run any time any nfs service
-# is started, so changes to local config files get
-# incorporated.  Having "RemainAfterExit=no" (the default)
-# ensures this happens.
-RemainAfterExit=no
-ExecStart=@_libexecdir@/nfs-utils/nfs-utils_env.sh
diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
index df3dd9d8a856..acca86b683fb 100644
--- a/systemd/nfs-idmapd.service
+++ b/systemd/nfs-idmapd.service
@@ -6,10 +6,6 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
 
 BindsTo=nfs-server.service
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
 Type=forking
-ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
+ExecStart=/usr/sbin/rpc.idmapd
diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
index 8a39f3e21184..15e828bc0d3c 100644
--- a/systemd/nfs-mountd.service
+++ b/systemd/nfs-mountd.service
@@ -6,10 +6,6 @@ After=proc-fs-nfsd.mount
 After=network.target local-fs.target
 BindsTo=nfs-server.service
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
 Type=forking
-ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
+ExecStart=/usr/sbin/rpc.mountd
diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
index 196c81849ff4..5be5de65cf8a 100644
--- a/systemd/nfs-server.service
+++ b/systemd/nfs-server.service
@@ -16,16 +16,11 @@ Before= rpc-statd-notify.service
 Wants=auth-rpcgss-module.service
 After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
-
 Type=oneshot
 RemainAfterExit=yes
 ExecStartPre=/usr/sbin/exportfs -r
-ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
+ExecStart=/usr/sbin/rpc.nfsd
 ExecStop=/usr/sbin/rpc.nfsd 0
 ExecStopPost=/usr/sbin/exportfs -au
 ExecStopPost=/usr/sbin/exportfs -f
diff --git a/systemd/rpc-gssd.service.in b/systemd/rpc-gssd.service.in
index 1a7911cb8f50..b35302740e37 100644
--- a/systemd/rpc-gssd.service.in
+++ b/systemd/rpc-gssd.service.in
@@ -9,11 +9,6 @@ ConditionPathExists=@_sysconfdir@/krb5.keytab
 
 PartOf=nfs-utils.service
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
-
 Type=forking
-ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
+ExecStart=/usr/sbin/rpc.gssd
diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
index 89ba36cf6d61..7bfc9b16d782 100644
--- a/systemd/rpc-statd-notify.service
+++ b/systemd/rpc-statd-notify.service
@@ -10,10 +10,6 @@ After=nfs-server.service
 
 PartOf=nfs-utils.service
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
 Type=forking
-ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
+ExecStart=-/usr/sbin/sm-notify
diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
index d5392b7cff4d..60d600f2fae6 100644
--- a/systemd/rpc-statd.service
+++ b/systemd/rpc-statd.service
@@ -7,12 +7,8 @@ After=network.target nss-lookup.target rpcbind.socket
 
 PartOf=nfs-utils.service
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
 Environment=RPC_STATD_NO_NOTIFY=1
-EnvironmentFile=-/run/sysconfig/nfs-utils
 Type=forking
 PIDFile=/var/run/rpc.statd.pid
-ExecStart=/usr/sbin/rpc.statd $STATDARGS
+ExecStart=/usr/sbin/rpc.statd
diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
index 41177b60d141..7187e3c35a41 100644
--- a/systemd/rpc-svcgssd.service
+++ b/systemd/rpc-svcgssd.service
@@ -11,10 +11,6 @@ ConditionPathExists=|!/run/gssproxy.pid
 ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
 ConditionPathExists=/etc/krb5.keytab
 
-Wants=nfs-config.service
-After=nfs-config.service
-
 [Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
 Type=forking
-ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
+ExecStart=/usr/sbin/rpc.svcgssd

[PATCH 13/15] statd: allow --no-notify to be passed via environment variable.

[NeilBrown]

The significant value of allowing this is that it means that
for default operation, systemd unit files do not need to pass any
options to any programs.  The purpose of this will become apparent in
the next patch.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/rpc-statd.service |    3 ++-
 utils/statd/statd.c       |    5 +++++
 utils/statd/statd.man     |    5 +++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
index a02f5c41a424..d5392b7cff4d 100644
--- a/systemd/rpc-statd.service
+++ b/systemd/rpc-statd.service
@@ -11,7 +11,8 @@ Wants=nfs-config.service
 After=nfs-config.service
 
 [Service]
+Environment=RPC_STATD_NO_NOTIFY=1
 EnvironmentFile=-/run/sysconfig/nfs-utils
 Type=forking
 PIDFile=/var/run/rpc.statd.pid
-ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
+ExecStart=/usr/sbin/rpc.statd $STATDARGS
diff --git a/utils/statd/statd.c b/utils/statd/statd.c
index 5f4ad79e6bf3..1c34c9ef02cb 100644
--- a/utils/statd/statd.c
+++ b/utils/statd/statd.c
@@ -251,10 +251,15 @@ int main (int argc, char **argv)
 	int nlm_udp = 0, nlm_tcp = 0;
 	struct rlimit rlim;
 	int notify_sockfd;
+	char *env;
 
 	/* Default: daemon mode, no other options */
 	run_mode = 0;
 
+	env = getenv("RPC_STATD_NO_NOTIFY");
+	if (env && atoi(env) > 0)
+		run_mode |= MODE_NO_NOTIFY;
+
 	/* Log to stderr if there's an error during startup */
 	xlog_stderr(1);
 	xlog_syslog(0);
diff --git a/utils/statd/statd.man b/utils/statd/statd.man
index 91c260f1bf5e..71d58461b5ea 100644
--- a/utils/statd/statd.man
+++ b/utils/statd/statd.man
@@ -418,6 +418,11 @@ it attempts to start listeners on network transports marked 'visible' in
 As long as at least one network transport listener starts successfully,
 .B rpc.statd
 will operate.
+.SH ENVIRONMENT
+.TP
+.B RPC_STATD_NO_NOTIFY=
+If set to a positive integer, has the same effect as
+.IR \-\-no\-notify .
 .SH FILES
 .TP 2.5i
 .I /var/lib/nfs/sm

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[J. Bruce Fields]

On Fri, Dec 02, 2016 at 02:58:27PM +1100, NeilBrown wrote:
> This is an RFC series.  A little voice at the back of my head keeps
> telling me that I'm over-engineering, but there isn't really that much
> new code, and I think the result has a lot to recommend it.
> 
> But please tell me if I'm wrong.
> 
> - Various daemons (not all) are enhance to accept configuration
>   information from /etc/nfs.conf
> - the conffile reader is enhanced to support include files, and
>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>   files usefully
> - nfs-config.service is removed, because it isn't really needed with
>   the above.
> - documentation for all the above is provided, including a new
>   nfs.systemd man page which gives the bigger picture.

Just based on that--sounds like an improvement to me.

But I'll see if I can take a closer in the coming week....

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (15):
>       Add man-page describing /etc/nfs.conf
>       conffile: add bool support
>       Add /etc/nfs.conf support to rpc.nfsd
>       Add /etc/nfs.conf support for mountd.
>       Add /etc/nfs.conf support for statd
>       Add /etc/nfs.conf support for sm-notify
>       conffile: free image of config file after parsing
>       conffile: split loading of file into a separate function.
>       conffile: add support for include files.
>       conffile: strip "quotes" from values in conf file.
>       conffile: ignore setting of empty string
>       conffile: allow $name expansion of tag values.
>       statd: allow --no-notify to be passed via environment variable.
>       systemd: Remove the nfs-config.service
>       Add nfs.systemd man page
> 
> 
>  configure.ac                     |    6 -
>  support/include/conffile.h       |    2 
>  support/nfs/conffile.c           |  147 +++++++++++++++++++++---------
>  systemd/Makefile.am              |    5 +
>  systemd/README                   |   22 +++-
>  systemd/nfs-blkmap.service       |    3 -
>  systemd/nfs-config.service.in    |   13 ---
>  systemd/nfs-idmapd.service       |    6 -
>  systemd/nfs-mountd.service       |    6 -
>  systemd/nfs-server.service       |    7 -
>  systemd/nfs.conf.man             |  186 ++++++++++++++++++++++++++++++++++++++
>  systemd/nfs.systemd.man          |  167 ++++++++++++++++++++++++++++++++++
>  systemd/rpc-gssd.service.in      |    7 -
>  systemd/rpc-statd-notify.service |    6 -
>  systemd/rpc-statd.service        |    7 -
>  systemd/rpc-svcgssd.service      |    6 -
>  utils/mountd/mountd.c            |   36 +++++++
>  utils/mountd/mountd.man          |   34 +++++++
>  utils/nfsd/nfsd.c                |   36 +++++++
>  utils/nfsd/nfsd.man              |   49 +++++++++-
>  utils/statd/sm-notify.c          |   11 ++
>  utils/statd/sm-notify.man        |   27 ++++++
>  utils/statd/statd.c              |   25 +++++
>  utils/statd/statd.man            |   40 ++++++++
>  24 files changed, 737 insertions(+), 117 deletions(-)
>  delete mode 100644 systemd/nfs-config.service.in
>  create mode 100644 systemd/nfs.conf.man
>  create mode 100644 systemd/nfs.systemd.man
> 
> --
> Signature
> 

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[Steve Dickson]

On 12/01/2016 10:58 PM, NeilBrown wrote:
> This is an RFC series.  A little voice at the back of my head keeps
> telling me that I'm over-engineering, but there isn't really that much
> new code, and I think the result has a lot to recommend it.
> 
> But please tell me if I'm wrong.
> 
> - Various daemons (not all) are enhance to accept configuration
>   information from /etc/nfs.conf
> - the conffile reader is enhanced to support include files, and
>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>   files usefully
> - nfs-config.service is removed, because it isn't really needed with
>   the above.
> - documentation for all the above is provided, including a new
>   nfs.systemd man page which gives the bigger picture.
This is some nice work! Its definitely a step in the right 
direction... IMHO... I especially like the idea of the include
statement, I'm very curious to see how that work! 

If this is finished, I'll start testing and hopefully 
get it in... soon.

steved.
  
> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (15):
>       Add man-page describing /etc/nfs.conf
>       conffile: add bool support
>       Add /etc/nfs.conf support to rpc.nfsd
>       Add /etc/nfs.conf support for mountd.
>       Add /etc/nfs.conf support for statd
>       Add /etc/nfs.conf support for sm-notify
>       conffile: free image of config file after parsing
>       conffile: split loading of file into a separate function.
>       conffile: add support for include files.
>       conffile: strip "quotes" from values in conf file.
>       conffile: ignore setting of empty string
>       conffile: allow $name expansion of tag values.
>       statd: allow --no-notify to be passed via environment variable.
>       systemd: Remove the nfs-config.service
>       Add nfs.systemd man page
> 
> 
>  configure.ac                     |    6 -
>  support/include/conffile.h       |    2 
>  support/nfs/conffile.c           |  147 +++++++++++++++++++++---------
>  systemd/Makefile.am              |    5 +
>  systemd/README                   |   22 +++-
>  systemd/nfs-blkmap.service       |    3 -
>  systemd/nfs-config.service.in    |   13 ---
>  systemd/nfs-idmapd.service       |    6 -
>  systemd/nfs-mountd.service       |    6 -
>  systemd/nfs-server.service       |    7 -
>  systemd/nfs.conf.man             |  186 ++++++++++++++++++++++++++++++++++++++
>  systemd/nfs.systemd.man          |  167 ++++++++++++++++++++++++++++++++++
>  systemd/rpc-gssd.service.in      |    7 -
>  systemd/rpc-statd-notify.service |    6 -
>  systemd/rpc-statd.service        |    7 -
>  systemd/rpc-svcgssd.service      |    6 -
>  utils/mountd/mountd.c            |   36 +++++++
>  utils/mountd/mountd.man          |   34 +++++++
>  utils/nfsd/nfsd.c                |   36 +++++++
>  utils/nfsd/nfsd.man              |   49 +++++++++-
>  utils/statd/sm-notify.c          |   11 ++
>  utils/statd/sm-notify.man        |   27 ++++++
>  utils/statd/statd.c              |   25 +++++
>  utils/statd/statd.man            |   40 ++++++++
>  24 files changed, 737 insertions(+), 117 deletions(-)
>  delete mode 100644 systemd/nfs-config.service.in
>  create mode 100644 systemd/nfs.conf.man
>  create mode 100644 systemd/nfs.systemd.man
> 
> --
> Signature
> 

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Wed, Dec 07 2016, Steve Dickson wrote:

> On 12/01/2016 10:58 PM, NeilBrown wrote:
>> This is an RFC series.  A little voice at the back of my head keeps
>> telling me that I'm over-engineering, but there isn't really that much
>> new code, and I think the result has a lot to recommend it.
>> 
>> But please tell me if I'm wrong.
>> 
>> - Various daemons (not all) are enhance to accept configuration
>>   information from /etc/nfs.conf
>> - the conffile reader is enhanced to support include files, and
>>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>>   files usefully
>> - nfs-config.service is removed, because it isn't really needed with
>>   the above.
>> - documentation for all the above is provided, including a new
>>   nfs.systemd man page which gives the bigger picture.
> This is some nice work! Its definitely a step in the right 
> direction... IMHO... I especially like the idea of the include
> statement, I'm very curious to see how that work! 
>
> If this is finished, I'll start testing and hopefully 
> get it in... soon.

I would be happy for this to go in as-is.  There are certainly some
changes needed (port, debug, host, etc) but they can be additions.
I people are happy with the overall approach, then let's do it.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[Steve Dickson]

On 12/06/2016 05:38 PM, NeilBrown wrote:
> On Wed, Dec 07 2016, Steve Dickson wrote:
> 
>> On 12/01/2016 10:58 PM, NeilBrown wrote:
>>> This is an RFC series.  A little voice at the back of my head keeps
>>> telling me that I'm over-engineering, but there isn't really that much
>>> new code, and I think the result has a lot to recommend it.
>>>
>>> But please tell me if I'm wrong.
>>>
>>> - Various daemons (not all) are enhance to accept configuration
>>>   information from /etc/nfs.conf
>>> - the conffile reader is enhanced to support include files, and
>>>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>>>   files usefully
>>> - nfs-config.service is removed, because it isn't really needed with
>>>   the above.
>>> - documentation for all the above is provided, including a new
>>>   nfs.systemd man page which gives the bigger picture.
>> This is some nice work! Its definitely a step in the right 
>> direction... IMHO... I especially like the idea of the include
>> statement, I'm very curious to see how that work! 
>>
>> If this is finished, I'll start testing and hopefully 
>> get it in... soon.
> 
> I would be happy for this to go in as-is.  There are certainly some
> changes needed (port, debug, host, etc) but they can be additions.
> I people are happy with the overall approach, then let's do it.
Sounds like a plan... 

steved.

> 
> Thanks,
> NeilBrown
> 

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[J. Bruce Fields]

On Fri, Dec 02, 2016 at 02:58:27PM +1100, NeilBrown wrote:
> This is an RFC series.  A little voice at the back of my head keeps
> telling me that I'm over-engineering, but there isn't really that much
> new code, and I think the result has a lot to recommend it.
> 
> But please tell me if I'm wrong.
> 
> - Various daemons (not all) are enhance to accept configuration
>   information from /etc/nfs.conf
> - the conffile reader is enhanced to support include files, and
>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>   files usefully

Currently those files are actually sourced by a shell, right?  So in
theory people could be doing tricky things in there that would no longer
be supported.  Probably unlikely, though, OK....

> - nfs-config.service is removed, because it isn't really needed with
>   the above.
> - documentation for all the above is provided, including a new
>   nfs.systemd man page which gives the bigger picture.

Still looks pretty good to me.

I'm a little worried about user interface churn.  We're not done yet
explaining that people have to run nfs-config.service after changing
things, soon we'll start telling them oh, never mind about that and oh,
by the way, you may want to start migrating your configuration to
/etc/nfs.conf....

But nfs.conf they can ignore and at worst they're going to get an error
trying to run nfs-config.service that they can ignore, so, OK, it
doesn't sound so bad.

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (15):
>       Add man-page describing /etc/nfs.conf
>       conffile: add bool support
>       Add /etc/nfs.conf support to rpc.nfsd
>       Add /etc/nfs.conf support for mountd.
>       Add /etc/nfs.conf support for statd
>       Add /etc/nfs.conf support for sm-notify
>       conffile: free image of config file after parsing
>       conffile: split loading of file into a separate function.
>       conffile: add support for include files.
>       conffile: strip "quotes" from values in conf file.
>       conffile: ignore setting of empty string
>       conffile: allow $name expansion of tag values.
>       statd: allow --no-notify to be passed via environment variable.
>       systemd: Remove the nfs-config.service
>       Add nfs.systemd man page
> 
> 
>  configure.ac                     |    6 -
>  support/include/conffile.h       |    2 
>  support/nfs/conffile.c           |  147 +++++++++++++++++++++---------
>  systemd/Makefile.am              |    5 +
>  systemd/README                   |   22 +++-
>  systemd/nfs-blkmap.service       |    3 -
>  systemd/nfs-config.service.in    |   13 ---
>  systemd/nfs-idmapd.service       |    6 -
>  systemd/nfs-mountd.service       |    6 -
>  systemd/nfs-server.service       |    7 -
>  systemd/nfs.conf.man             |  186 ++++++++++++++++++++++++++++++++++++++
>  systemd/nfs.systemd.man          |  167 ++++++++++++++++++++++++++++++++++
>  systemd/rpc-gssd.service.in      |    7 -
>  systemd/rpc-statd-notify.service |    6 -
>  systemd/rpc-statd.service        |    7 -
>  systemd/rpc-svcgssd.service      |    6 -
>  utils/mountd/mountd.c            |   36 +++++++
>  utils/mountd/mountd.man          |   34 +++++++
>  utils/nfsd/nfsd.c                |   36 +++++++
>  utils/nfsd/nfsd.man              |   49 +++++++++-
>  utils/statd/sm-notify.c          |   11 ++
>  utils/statd/sm-notify.man        |   27 ++++++
>  utils/statd/statd.c              |   25 +++++
>  utils/statd/statd.man            |   40 ++++++++
>  24 files changed, 737 insertions(+), 117 deletions(-)
>  delete mode 100644 systemd/nfs-config.service.in
>  create mode 100644 systemd/nfs.conf.man
>  create mode 100644 systemd/nfs.systemd.man
> 
> --
> Signature

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]

On Wed, Dec 07 2016, J. Bruce Fields wrote:

> On Fri, Dec 02, 2016 at 02:58:27PM +1100, NeilBrown wrote:
>> This is an RFC series.  A little voice at the back of my head keeps
>> telling me that I'm over-engineering, but there isn't really that much
>> new code, and I think the result has a lot to recommend it.
>> 
>> But please tell me if I'm wrong.
>> 
>> - Various daemons (not all) are enhance to accept configuration
>>   information from /etc/nfs.conf
>> - the conffile reader is enhanced to support include files, and
>>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>>   files usefully
>
> Currently those files are actually sourced by a shell, right?  So in
> theory people could be doing tricky things in there that would no longer
> be supported.  Probably unlikely, though, OK....

They are sourced by a shell, but they are read and written by tools.
SUSE has YAST, Debian has debconf.  I assume redhat has something
similar.
Those tools might let some shell syntax through, but people who try
games like that are already taking a risk I think.

>
>> - nfs-config.service is removed, because it isn't really needed with
>>   the above.
>> - documentation for all the above is provided, including a new
>>   nfs.systemd man page which gives the bigger picture.
>
> Still looks pretty good to me.
>
> I'm a little worried about user interface churn.  We're not done yet
> explaining that people have to run nfs-config.service after changing
> things, soon we'll start telling them oh, never mind about that and oh,
> by the way, you may want to start migrating your configuration to
> /etc/nfs.conf....

Did we tell people to run nfs-config.server?  The intention was that it
wouldn't be needed.  Any systemd transaction that needed nfs
configuration should run the command once.

Commit: c4940fad2a73 ("systemd: ensure nfs-config service is re-run as needed.")

should have made that happen.

Yes, some migration is needed.  I see distros as the primary target for
that, and the 'include' and '$name' functionality is supposed to allow
that to be transparent.  i.e. the configuration stays as it is, it just
gets into nfs daemons by a different path.
If people want to edit /etc/nfs.conf directly, then they are on their
own, just like people who edited their systemd unit files directly,
or created replacements in /etc.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[J. Bruce Fields]

On Wed, Dec 07, 2016 at 09:47:29AM +1100, NeilBrown wrote:
> On Wed, Dec 07 2016, J. Bruce Fields wrote:
> > On Fri, Dec 02, 2016 at 02:58:27PM +1100, NeilBrown wrote:
> >> - nfs-config.service is removed, because it isn't really needed with
> >>   the above.
> >> - documentation for all the above is provided, including a new
> >>   nfs.systemd man page which gives the bigger picture.
> >
> > Still looks pretty good to me.
> >
> > I'm a little worried about user interface churn.  We're not done yet
> > explaining that people have to run nfs-config.service after changing
> > things, soon we'll start telling them oh, never mind about that and oh,
> > by the way, you may want to start migrating your configuration to
> > /etc/nfs.conf....
> 
> Did we tell people to run nfs-config.server?  The intention was that it
> wouldn't be needed.  Any systemd transaction that needed nfs
> configuration should run the command once.
> 
> Commit: c4940fad2a73 ("systemd: ensure nfs-config service is re-run as needed.")
> 
> should have made that happen.

D'oh, I totally forgot that you fixed that, thanks.

> Yes, some migration is needed.  I see distros as the primary target for
> that, and the 'include' and '$name' functionality is supposed to allow
> that to be transparent.  i.e. the configuration stays as it is, it just
> gets into nfs daemons by a different path.
> If people want to edit /etc/nfs.conf directly, then they are on their
> own, just like people who edited their systemd unit files directly,
> or created replacements in /etc.

Yep, sounds like a plan.

--b.

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[Steve Dickson]

On 12/01/2016 10:58 PM, NeilBrown wrote:
> This is an RFC series.  A little voice at the back of my head keeps
> telling me that I'm over-engineering, but there isn't really that much
> new code, and I think the result has a lot to recommend it.
> 
> But please tell me if I'm wrong.
> 
> - Various daemons (not all) are enhance to accept configuration
>   information from /etc/nfs.conf
> - the conffile reader is enhanced to support include files, and
>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>   files usefully
> - nfs-config.service is removed, because it isn't really needed with
>   the above.
> - documentation for all the above is provided, including a new
>   nfs.systemd man page which gives the bigger picture.
> 
Here is the nfs.conf I will be using to do my testing... 
I'm just wondering if this is something we should put in the git tree? 

#
# This is a general conifguration for the 
# NFS daemons and tools
#
#[nfsdcltrack]
# storagedir=/var/lib/nfs/nfsdcltrack
#
#[nfsd]
# threads=8
# grace-time=90
# lease-time=90
# udp=y
# tcp=y
# vers2=n
# vers3=y
# vers4=y
# vers4.0=y
# vers4.1=y
# vers4.2=y
# rdma=n
# 
#[mountd]
# manage_gids=n
# descriptors=0
# port=42258
# threads=1
# reverse-lookup=n
# state-directory-path=/var/lib/nfs
# ha-callout=NULL
#
#[statd]
# port=0
# outgoing-port=0
# name=NULL
# state-directory-path=/var/lib/nfs/statd
# ha-callout=NULL
#
#[lockd]
# port=0
# udp-port=0
#
#[sm-notify]
# retry-time=900
# outgoing-port=NULL
# outgoing-addr=NULL
#

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

On Wed, Dec 07 2016, Steve Dickson wrote:

> On 12/01/2016 10:58 PM, NeilBrown wrote:
>> This is an RFC series.  A little voice at the back of my head keeps
>> telling me that I'm over-engineering, but there isn't really that much
>> new code, and I think the result has a lot to recommend it.
>> 
>> But please tell me if I'm wrong.
>> 
>> - Various daemons (not all) are enhance to accept configuration
>>   information from /etc/nfs.conf
>> - the conffile reader is enhanced to support include files, and
>>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>>   files usefully
>> - nfs-config.service is removed, because it isn't really needed with
>>   the above.
>> - documentation for all the above is provided, including a new
>>   nfs.systemd man page which gives the bigger picture.
>> 
> Here is the nfs.conf I will be using to do my testing... 
> I'm just wondering if this is something we should put in the git tree? 
>
> #
> # This is a general conifguration for the 
> # NFS daemons and tools
> #
> #[nfsdcltrack]
> # storagedir=/var/lib/nfs/nfsdcltrack
> #
> #[nfsd]
> # threads=8
> # grace-time=90
> # lease-time=90
> # udp=y
> # tcp=y
> # vers2=n
> # vers3=y
> # vers4=y
> # vers4.0=y
> # vers4.1=y
> # vers4.2=y
> # rdma=n
> # 
> #[mountd]
> # manage_gids=n
> # descriptors=0
> # port=42258
> # threads=1
> # reverse-lookup=n
> # state-directory-path=/var/lib/nfs
> # ha-callout=NULL

NULL??  That looks like it is meant to mean something.
Ditto for the "port=0" below.

You seem to be listing the default here, in which case I would just have

  ha-callout=

Otherwise, this is probably a good idea.  People seem to prefer to fill
in templates, rather than read documentation :-(

Thanks,
NeilBrown


> #
> #[statd]
> # port=0
> # outgoing-port=0
> # name=NULL
> # state-directory-path=/var/lib/nfs/statd

This is normally "/var/lib/nfs" - no "/statd".

> # ha-callout=NULL
> #
> #[lockd]
> # port=0
> # udp-port=0
> #
> #[sm-notify]
> # retry-time=900
> # outgoing-port=NULL
> # outgoing-addr=NULL
> #

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[Steve Dickson]

On 12/06/2016 05:51 PM, NeilBrown wrote:
> On Wed, Dec 07 2016, Steve Dickson wrote:
> 
>> On 12/01/2016 10:58 PM, NeilBrown wrote:
>>> This is an RFC series.  A little voice at the back of my head keeps
>>> telling me that I'm over-engineering, but there isn't really that much
>>> new code, and I think the result has a lot to recommend it.
>>>
>>> But please tell me if I'm wrong.
>>>
>>> - Various daemons (not all) are enhance to accept configuration
>>>   information from /etc/nfs.conf
>>> - the conffile reader is enhanced to support include files, and
>>>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>>>   files usefully
>>> - nfs-config.service is removed, because it isn't really needed with
>>>   the above.
>>> - documentation for all the above is provided, including a new
>>>   nfs.systemd man page which gives the bigger picture.
>>>
>> Here is the nfs.conf I will be using to do my testing... 
>> I'm just wondering if this is something we should put in the git tree? 
>>
>> #
>> # This is a general conifguration for the 
>> # NFS daemons and tools
>> #
>> #[nfsdcltrack]
>> # storagedir=/var/lib/nfs/nfsdcltrack
>> #
>> #[nfsd]
>> # threads=8
>> # grace-time=90
>> # lease-time=90
>> # udp=y
>> # tcp=y
>> # vers2=n
>> # vers3=y
>> # vers4=y
>> # vers4.0=y
>> # vers4.1=y
>> # vers4.2=y
>> # rdma=n
>> # 
>> #[mountd]
>> # manage_gids=n
>> # descriptors=0
>> # port=42258
>> # threads=1
>> # reverse-lookup=n
>> # state-directory-path=/var/lib/nfs
>> # ha-callout=NULL
> 
> NULL??  That looks like it is meant to mean something.
> Ditto for the "port=0" below.
> 
> You seem to be listing the default here, in which case I would just have
> 
>   ha-callout=
Fine. 

> 
> Otherwise, this is probably a good idea.  People seem to prefer to fill
> in templates, rather than read documentation :-(
I'll check it in but I don't think its a good idea to
write any install rules... I don't want to blindly overwrite
the current configurations... I'll let the distros deal with
that. 

> 
>> #
>> #[statd]
>> # port=0
>> # outgoing-port=0
>> # name=NULL
>> # state-directory-path=/var/lib/nfs/statd
> 
> This is normally "/var/lib/nfs" - no "/statd".
Got it...

steved.

> 
>> # ha-callout=NULL
>> #
>> #[lockd]
>> # port=0
>> # udp-port=0
>> #
>> #[sm-notify]
>> # retry-time=900
>> # outgoing-port=NULL
>> # outgoing-addr=NULL
>> #

Re: [RFC PATCH 00/15] Enhance /etc/nfs.conf usage and remove nfs-config.service

[Steve Dickson]

On 12/01/2016 10:58 PM, NeilBrown wrote:
> This is an RFC series.  A little voice at the back of my head keeps
> telling me that I'm over-engineering, but there isn't really that much
> new code, and I think the result has a lot to recommend it.
> 
> But please tell me if I'm wrong.
> 
> - Various daemons (not all) are enhance to accept configuration
>   information from /etc/nfs.conf
> - the conffile reader is enhanced to support include files, and
>   particularly to be able to include /etc/sysconf/X or /etc/defaults/X
>   files usefully
> - nfs-config.service is removed, because it isn't really needed with
>   the above.
> - documentation for all the above is provided, including a new
>   nfs.systemd man page which gives the bigger picture.
> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (15):
>       Add man-page describing /etc/nfs.conf
>       conffile: add bool support
>       Add /etc/nfs.conf support to rpc.nfsd
>       Add /etc/nfs.conf support for mountd.
>       Add /etc/nfs.conf support for statd
>       Add /etc/nfs.conf support for sm-notify
>       conffile: free image of config file after parsing
>       conffile: split loading of file into a separate function.
>       conffile: add support for include files.
>       conffile: strip "quotes" from values in conf file.
>       conffile: ignore setting of empty string
>       conffile: allow $name expansion of tag values.
>       statd: allow --no-notify to be passed via environment variable.
>       systemd: Remove the nfs-config.service
>       Add nfs.systemd man page
All 15 committed....

steved
> 
> 
>  configure.ac                     |    6 -
>  support/include/conffile.h       |    2 
>  support/nfs/conffile.c           |  147 +++++++++++++++++++++---------
>  systemd/Makefile.am              |    5 +
>  systemd/README                   |   22 +++-
>  systemd/nfs-blkmap.service       |    3 -
>  systemd/nfs-config.service.in    |   13 ---
>  systemd/nfs-idmapd.service       |    6 -
>  systemd/nfs-mountd.service       |    6 -
>  systemd/nfs-server.service       |    7 -
>  systemd/nfs.conf.man             |  186 ++++++++++++++++++++++++++++++++++++++
>  systemd/nfs.systemd.man          |  167 ++++++++++++++++++++++++++++++++++
>  systemd/rpc-gssd.service.in      |    7 -
>  systemd/rpc-statd-notify.service |    6 -
>  systemd/rpc-statd.service        |    7 -
>  systemd/rpc-svcgssd.service      |    6 -
>  utils/mountd/mountd.c            |   36 +++++++
>  utils/mountd/mountd.man          |   34 +++++++
>  utils/nfsd/nfsd.c                |   36 +++++++
>  utils/nfsd/nfsd.man              |   49 +++++++++-
>  utils/statd/sm-notify.c          |   11 ++
>  utils/statd/sm-notify.man        |   27 ++++++
>  utils/statd/statd.c              |   25 +++++
>  utils/statd/statd.man            |   40 ++++++++
>  24 files changed, 737 insertions(+), 117 deletions(-)
>  delete mode 100644 systemd/nfs-config.service.in
>  create mode 100644 systemd/nfs.conf.man
>  create mode 100644 systemd/nfs.systemd.man
> 
> --
> Signature
>