[RFC nfs-utils PATCH 0/2] add systemd generator for the rpc_pipefs mountpoint

[RFC nfs-utils PATCH 0/2] add systemd generator for the rpc_pipefs mountpoint

[Scott Mayhew]

These patches aim to make it a little easier to change the mountpoint.
Right now if you change the pipefs-directory in /etc/nfs.conf, you still
need to manually override the dependencies in the systemd unit files in
order for the change to actually work.  

The first patch moves rpc.idmapd's (mostly) undocumented
pipefs-directory from /etc/idmapd.conf to /etc/nfs.conf, which rpc.gssd
already can use for it's pipefs-directory configuration.

The second patch adds a systemd generator that reads the
pipefs-directory configurations from /etc/nfs.conf, and if they differ
from the default it will automatically 1) create a systemd mount unit
file for the pipefs mountpoint and 2) it will create a drop-in
configuration file to override the Requires= and After= directives for
that service.

I did run into a bit of a snag though.  Depsite overriding the
dependencies for both idmapd and gssd, I wind up with two pipefs
filesystems mounted:

[root@coeurl ~]# grep pipefs /proc/mounts
sunrpc /var/lib/nfs/rpc_pipefs rpc_pipefs rw,relatime 0 0
sunrpc /run/rpc_pipefs rpc_pipefs rw,relatime 0 0

systemd still shows the dependency on the default pipefs mountpoint:

[root@coeurl ~]# systemctl list-dependencies --before var-lib-nfs-rpc_pipefs.mount
var-lib-nfs-rpc_pipefs.mount
● ├─nfs-idmapd.service
● └─rpc-gssd.service

as well as the new one:

[root@coeurl ~]# systemctl list-dependencies --before run-rpc_pipefs.mount
run-rpc_pipefs.mount
● ├─nfs-idmapd.service
● └─rpc-gssd.service

The drop-in configs to override the pipefs mountpoint look correct.  I'm
clearing both Requires= and After= before setting them:

[root@coeurl ~]# cat /run/systemd/generator/nfs-idmapd.service.d/10-pipefs.conf 
# Automatically generated by rpc-pipefs-generator

[Unit]
Requires=
Requires=run-rpc_pipefs.mount
After=
After=run-rpc_pipefs.mount local-fs.target

[root@coeurl ~]# cat /run/systemd/generator/rpc-gssd.service.d/10-pipefs.conf 
# Automatically generated by rpc-pipefs-generator

[Unit]
Requires=
Requires=run-rpc_pipefs.mount
After=
After=run-rpc_pipefs.mount

The generated mount unit file also looks correct:

[root@coeurl ~]# cat /run/systemd/generator/run-rpc_pipefs.mount 
# Automatically generated by rpc-pipefs-generator

[Unit]
Description=RPC Pipe File System
DefaultDependencies=no
After=systemd-tmpfiles-setup.service
Conflicts=umount.target

[Mount]
What=sunrpc
Where=/run/rpc_pipefs
Type=rpc_pipefs

systemd shows that the drop-in config was picked up:

[root@coeurl ~]# systemctl status nfs-idmapd
● nfs-idmapd.service - NFSv4 ID-name mapping service
   Loaded: loaded (/usr/lib/systemd/system/nfs-idmapd.service; static; vendor preset: disabled)
  Drop-In: /run/systemd/generator/nfs-idmapd.service.d
           └─10-pipefs.conf
   Active: active (running) since Fri 2017-03-31 16:54:24 EDT; 5min ago
  Process: 27831 ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS (code=exited, status=0/SUCCESS)
 Main PID: 27832 (rpc.idmapd)
    Tasks: 1 (limit: 4915)
   CGroup: /system.slice/nfs-idmapd.service
           └─27832 /usr/sbin/rpc.idmapd

and lsof shows that the correct mountpoint is being used:

[root@coeurl ~]# lsof -p 27832 2>/dev/null | grep pipefs
rpc.idmap 27832 root   10r      DIR               0,42        0        103 /run/rpc_pipefs/nfs

The same for gssd:

[root@coeurl ~]# systemctl status rpc-gssd
● rpc-gssd.service - RPC security service for NFS client and server
   Loaded: loaded (/usr/lib/systemd/system/rpc-gssd.service; static; vendor preset: disabled)
  Drop-In: /run/systemd/generator/rpc-gssd.service.d
           └─10-pipefs.conf
   Active: active (running) since Fri 2017-03-31 16:54:29 EDT; 6min ago
  Process: 27839 ExecStart=/usr/sbin/rpc.gssd $RPCGSSDARGS (code=exited, status=0/SUCCESS)
 Main PID: 27840 (rpc.gssd)
    Tasks: 1 (limit: 4915)
   CGroup: /system.slice/rpc-gssd.service
           └─27840 /usr/sbin/rpc.gssd

[root@coeurl ~]# lsof -p 27840 2>/dev/null | grep pipefs
rpc.gssd 27840 root  cwd       DIR               0,42        0   24637 /run/rpc_pipefs
rpc.gssd 27840 root    7r      DIR               0,42        0   24637 /run/rpc_pipefs
rpc.gssd 27840 root   11u     FIFO               0,42      0t0     112 /run/rpc_pipefs/gssd/clntXX/gssd

So it looks like systemd is using both sets of dependencies, even though
the programs themselves are only looking for what's specified in
/etc/nfs.conf.  I'm not sure what to do about that.  Maybe remove the
var-lib-nfs-rpc_pipefs.mount unit as well as the dependencies in the
nfs-idmapd.service and rpc-gssd.service files, and have the generator
create those automatically as well?

-Scott

Scott Mayhew (2):
  idmapd: move the pipefs-directory config option to nfs.conf
  systemd: add a generator for the rpc_pipefs mountpoint

 .gitignore                     |   1 +
 nfs.conf                       |   3 +
 systemd/Makefile.am            |   4 +-
 systemd/nfs.conf.man           |   9 ++
 systemd/rpc-pipefs-generator.c | 256 +++++++++++++++++++++++++++++++++++++++++
 systemd/rpc-svcgssd.service    |   3 +-
 utils/idmapd/idmapd.c          |  35 +++---
 utils/idmapd/idmapd.man        |  19 ++-
 8 files changed, 305 insertions(+), 25 deletions(-)
 create mode 100644 systemd/rpc-pipefs-generator.c

-- 
2.9.3

[RFC nfs-utils PATCH 1/2] idmapd: move the pipefs-directory config option to nfs.conf

[Scott Mayhew]

Changed idmapd to read its value for the pipefs-directory from
/etc/nfs.conf rather than /etc/idmapd.conf.  All other configurations
related to id mapping still reside in /etc/idmapd.conf for now.

Removed the -c option, since it would be confusing as whether it should
override nfs.conf, idmapd.conf, or both.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs.conf                |  3 +++
 systemd/nfs.conf.man    |  9 +++++++++
 utils/idmapd/idmapd.c   | 35 ++++++++++++++---------------------
 utils/idmapd/idmapd.man | 19 ++++++++++++++++++-
 4 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/nfs.conf b/nfs.conf
index 81ece06..4359904 100644
--- a/nfs.conf
+++ b/nfs.conf
@@ -17,6 +17,9 @@
 # cred-cache-directory=
 # preferred-realm=
 #
+#[idmapd]
+# pipefs-directory=/var/lib/nfs/rpc_pipefs
+#
 #[lockd]
 # port=0
 # udp-port=0
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index bdc0988..83cf84a 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -215,6 +215,15 @@ See
 for details.
 
 .TP
+.B idmapd
+Recognized values:
+.BR pipefs-directory .
+
+See
+.BR rpc.idmapd (8)
+for details.
+
+.TP
 .B svcgssd
 Recognized values:
 .BR principal .
diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index f4e083a..561a4b8 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -214,13 +214,11 @@ main(int argc, char **argv)
 	struct event initialize;
 	struct passwd *pw;
 	struct group *gr;
-	struct stat sb;
 	char *xpipefsdir = NULL;
 	int serverstart = 1, clientstart = 1;
 	int ret;
 	char *progname;
 
-	conf_path = _PATH_IDMAPDCONF;
 	nobodyuser = NFS4NOBODY_USER;
 	nobodygroup = NFS4NOBODY_GROUP;
 	strlcpy(pipefsdir, PIPEFS_DIR, sizeof(pipefsdir));
@@ -231,11 +229,9 @@ main(int argc, char **argv)
 		progname = argv[0];
 	xlog_open(progname);
 
-#define GETOPTSTR "hvfd:p:U:G:c:CS"
+#define GETOPTSTR "hvfd:p:U:G:CS"
 	opterr=0; /* Turn off error messages */
 	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1) {
-		if (opt == 'c')
-			conf_path = optarg;
 		if (opt == '?') {
 			if (strchr(GETOPTSTR, optopt))
 				warnx("'-%c' option requires an argument.", optopt);
@@ -247,20 +243,19 @@ main(int argc, char **argv)
 	}
 	optind = 1;
 
-	if (stat(conf_path, &sb) == -1 && (errno == ENOENT || errno == EACCES)) {
-		warn("Skipping configuration file \"%s\"", conf_path);
-		conf_path = NULL;
-	} else {
-		conf_init();
-		verbose = conf_get_num("General", "Verbosity", 0);
-		cache_entry_expiration = conf_get_num("General",
-				"Cache-Expiration", DEFAULT_IDMAP_CACHE_EXPIRY);
-		CONF_SAVE(xpipefsdir, conf_get_str("General", "Pipefs-Directory"));
-		if (xpipefsdir != NULL)
-			strlcpy(pipefsdir, xpipefsdir, sizeof(pipefsdir));
-		CONF_SAVE(nobodyuser, conf_get_str("Mapping", "Nobody-User"));
-		CONF_SAVE(nobodygroup, conf_get_str("Mapping", "Nobody-Group"));
-	}
+	conf_path = NFS_CONFFILE;
+	conf_init();
+	CONF_SAVE(xpipefsdir, conf_get_str("idmapd", "pipefs-directory"));
+	if (xpipefsdir != NULL)
+		strlcpy(pipefsdir, xpipefsdir, sizeof(pipefsdir));
+
+	conf_path = _PATH_IDMAPDCONF;
+	conf_init();
+	verbose = conf_get_num("General", "Verbosity", 0);
+	cache_entry_expiration = conf_get_num("General",
+			"cache-expiration", DEFAULT_IDMAP_CACHE_EXPIRY);
+	CONF_SAVE(nobodyuser, conf_get_str("Mapping", "Nobody-User"));
+	CONF_SAVE(nobodygroup, conf_get_str("Mapping", "Nobody-Group"));
 
 	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1)
 		switch (opt) {
@@ -307,8 +302,6 @@ main(int argc, char **argv)
 #ifdef HAVE_NFS4_SET_DEBUG
 	nfs4_set_debug(verbose, xlog_warn);
 #endif
-	if (conf_path == NULL)
-		conf_path = _PATH_IDMAPDCONF;
 	if (nfs4_init_name_mapping(conf_path))
 		errx(1, "Unable to create name to user id mappings.");
 
diff --git a/utils/idmapd/idmapd.man b/utils/idmapd/idmapd.man
index d4ab894..0fbc24c 100644
--- a/utils/idmapd/idmapd.man
+++ b/utils/idmapd/idmapd.man
@@ -78,6 +78,21 @@ Client-only: perform no idmapping for any NFS server, even if one is detected.
 .It Fl S
 Server-only: perform no idmapping for any NFS client, even if one is detected.
 .El
+.Sh CONFIGURATION FILES
+The
+.Sy [idmapd]
+section of the
+.Pa /etc/nfs.conf
+configuration file recognizes the following value:
+.Bl -tag -width Ds_imagedir
+.It Sy pipefs-directory
+Equivalent to
+.Sy -p .
+.El
+.Pp
+All other settings related to id mapping are found in the
+.Pa /etc/idmapd.conf
+configuration file.
 .Sh EXAMPLES
 .Cm rpc.idmapd -f -vvv
 .Pp
@@ -94,9 +109,11 @@ messages to console, and with a verbosity level of 3.
 .\" This next request is for sections 1, 6, 7 & 8 only.
 .\" .Sh ENVIRONMENT
 .Sh FILES
-.Pa /etc/idmapd.conf
+.Pa /etc/idmapd.conf ,
+.Pa /etc/nfs.conf
 .Sh SEE ALSO
 .Xr idmapd.conf 5 ,
+.Xr nfs.conf 5 ,
 .Xr nfsidmap 8
 .\".Sh SEE ALSO
 .\".Xr nylon.conf 4
-- 
2.9.3

[RFC nfs-utils PATCH 2/2] systemd: add a generator for the rpc_pipefs mountpoint

[Scott Mayhew]

The nfs.conf has config options for the pipefs mountpoint.  Currently,
changing these from the default also requires manually overriding the
systemd unit files that are hard-coded to mount the filesystem on
/var/lib/nfs/rpc_pipefs.

This patch adds a generator that creates a mount unit file for the
pipefs when a non-default value is specified in /etc/nfs.conf, as well
as drop-in config files to override the dependencies for the systemd
units using the pipefs.

This patch also removes the dependency on the pipefs from the
rpc-svcgssd.service unit file.  rpc.svcgssd uses the sunrpc cache
mechanism to exchange data with the kernel, not the pipefs.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 .gitignore                     |   1 +
 systemd/Makefile.am            |   4 +-
 systemd/rpc-pipefs-generator.c | 256 +++++++++++++++++++++++++++++++++++++++++
 systemd/rpc-svcgssd.service    |   3 +-
 4 files changed, 261 insertions(+), 3 deletions(-)
 create mode 100644 systemd/rpc-pipefs-generator.c

diff --git a/.gitignore b/.gitignore
index 126d12c..941aca0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -70,6 +70,7 @@ tests/nsm_client/nlm_sm_inter_svc.c
 tests/nsm_client/nlm_sm_inter_xdr.c
 utils/nfsidmap/nfsidmap
 systemd/nfs-server-generator
+systemd/rpc-pipefs-generator
 systemd/nfs-config.service
 systemd/rpc-gssd.service
 # cscope database files
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 0d15b9f..4a04f5a 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -42,12 +42,14 @@ EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
 unit_dir = /usr/lib/systemd/system
 generator_dir = /usr/lib/systemd/system-generators
 
-EXTRA_PROGRAMS	= nfs-server-generator
+EXTRA_PROGRAMS	= nfs-server-generator rpc-pipefs-generator
 genexecdir = $(generator_dir)
 nfs_server_generator_LDADD = ../support/export/libexport.a \
 			     ../support/nfs/libnfs.a \
 			     ../support/misc/libmisc.a
 
+rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
+
 if INSTALL_SYSTEMD
 genexec_PROGRAMS = nfs-server-generator
 install-data-hook: $(unit_files)
diff --git a/systemd/rpc-pipefs-generator.c b/systemd/rpc-pipefs-generator.c
new file mode 100644
index 0000000..99d5a9b
--- /dev/null
+++ b/systemd/rpc-pipefs-generator.c
@@ -0,0 +1,256 @@
+/*
+ * rpc-pipefs-generator:
+ *   systemd generator to create ordering dependencies between
+ *   nfs services and the rpc_pipefs mount(s)
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ctype.h>
+#include <stdio.h>
+
+#include "nfslib.h"
+#include "conffile.h"
+
+#define NUM_PIPEFS_USERS 3
+#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
+char *conf_path;
+
+/*
+ * conf_name - the name as it appears (or would appear, in the case of idmapd,
+ *             in the /etc/nfs.conf
+ * unit_name - the name of the systemd service unit file (minus '.service')
+ * after_local_fs - should the After= directive have local-fs.target or not
+ * generate - should a drop-in config be generated or not
+ */
+struct pipefs_user {
+	char	*conf_name;
+	char	*unit_name;
+	int	after_local_fs;
+	int	generate;
+};
+
+/*
+ * blkmapd is a placeholder for (generate=0) because it does not have nfs.conf
+ * support and because the pipefs directory cannot be overriden.
+ */
+static struct pipefs_user	pipefs_users[NUM_PIPEFS_USERS] = {
+	{
+		.conf_name = "blkmapd",
+		.unit_name = "nfs-blkmap",
+		.after_local_fs = 0,
+		.generate = 0,
+	},
+	{
+		.conf_name = "gssd",
+		.unit_name = "rpc-gssd",
+		.after_local_fs = 0,
+		.generate = 1,
+	},
+	{
+		.conf_name = "idmapd",
+		.unit_name = "nfs-idmapd",
+		.after_local_fs = 1,
+		.generate = 1,
+	}
+};
+
+int systemd_len(char *path)
+{
+	char *p;
+	int len = 0;
+
+	p = path;
+	while (*p == '/')
+		p++;
+
+	if (!*p)
+		/* "/" becomes "-", otherwise leading "/" is ignored */
+		return 1;
+
+	while (*p) {
+		unsigned char c = *p++;
+
+		if (c == '/') {
+			/* multiple non-trailing slashes become '-' */
+			while (*p == '/')
+				p++;
+			if (*p)
+				len++;
+		} else if (isalnum(c) || c == ':' || c == '.' || c == '_')
+			len++;
+		else {
+			len += 4;
+		}
+	}
+
+	return len;
+}
+
+char *systemd_escape(char *path)
+{
+	char *result = NULL;
+	char *p;
+	int len;
+
+	len = systemd_len(path);
+	if (!len)
+		goto out;
+
+	result = malloc(len + strlen(".mount") + 1);
+	if (!result)
+		goto out;
+
+	p = result;
+	while (*path == '/')
+		path++;
+	if (!*path) {
+		/* "/" becomes "-", otherwise leading "/" is ignored */
+		*p++ = '-';
+		goto out_append;
+	}
+	while (*path) {
+		unsigned char c = *path++;
+
+		if (c == '/') {
+			/* multiple non-trailing slashes become '-' */
+			while (*path == '/')
+				path++;
+			if (*path)
+				*p++ = '-';
+		} else if (isalnum(c) || c == ':' || c == '.' || c == '_')
+			*p++ = c;
+		else {
+			*p++ = '\\';
+			*p++ = 'x';
+			*p++ = '0' + ((c&0xf0)>>4) + (c>=0xa0)*('a'-'9'-1);
+			*p++ = '0' + (c&0x0f) + ((c&0x0f)>=0x0a)*('a'-'9'-1);
+		}
+	}
+
+out_append:
+	sprintf(p, ".mount");
+out:
+	return result;
+}
+
+static int generate_mount_unit(const char *mountpoint, const char *unitname,
+			       const char *dirname)
+{
+	char	*path;
+	struct stat stb;
+	FILE	*f;
+
+	path = malloc(strlen(dirname) + 1 + strlen(unitname));
+	if (!path)
+		return 1;
+	sprintf(path, "%s/%s", dirname, unitname);
+	if (stat(path, &stb) == 0)
+		return 0;
+	f = fopen(path, "w");
+	if (!f)
+		return 1;
+
+	fprintf(f, "# Automatically generated by rpc-pipefs-generator\n\n[Unit]\n");
+	fprintf(f, "Description=RPC Pipe File System\n");
+	fprintf(f, "DefaultDependencies=no\n");
+	fprintf(f, "After=systemd-tmpfiles-setup.service\n");
+	fprintf(f, "Conflicts=umount.target\n");
+	fprintf(f, "\n[Mount]\n");
+	fprintf(f, "What=sunrpc\n");
+	fprintf(f, "Where=%s\n", mountpoint);
+	fprintf(f, "Type=rpc_pipefs\n");
+
+	fclose(f);
+	return 0;
+}
+
+static
+int generate_drop_in(char *conf_name, char *unit_name,
+		     const int after_local_fs, const char *dirname)
+{
+	char	*path;
+	char	dirbase[] = ".service.d";
+	char	filebase[] = "/10-pipefs.conf";
+	char	*s;
+	char	*pipefs_unit;
+	FILE	*f;
+	int 	ret = 0;
+
+	s = conf_get_str(conf_name, "pipefs-directory");
+	if (!s)
+		return 0;
+
+	if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
+			strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
+		return 0;
+
+	pipefs_unit = systemd_escape(s);
+	if (!pipefs_unit)
+		return 1;
+
+	ret = generate_mount_unit(s, pipefs_unit, dirname);
+	if (ret)
+		return ret;
+
+	path = malloc(strlen(dirname) + 1 + sizeof(unit_name) +
+			sizeof(dirbase) + sizeof(filebase));
+	if (!path)
+		return 2;
+	sprintf(path, "%s/%s%s", dirname, unit_name, dirbase);
+	mkdir(path, 0755);
+	strcat(path, filebase);
+	f = fopen(path, "w");
+	if (!f)
+		return 1;
+
+	fprintf(f, "# Automatically generated by rpc-pipefs-generator\n\n[Unit]\n");
+	fprintf(f, "Requires=\nRequires=");
+	fprintf(f, "%s\n", pipefs_unit);
+	fprintf(f, "After=\nAfter=");
+	fprintf(f, "%s", pipefs_unit);
+	if (after_local_fs)
+		fprintf(f, " local-fs.target");
+	fprintf(f, "\n");
+	fclose(f);
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	int	i;
+	struct pipefs_user p;
+	int 	ret;
+
+	/* Avoid using any external services */
+	xlog_syslog(0);
+
+	if (argc != 4 || argv[1][0] != '/') {
+		fprintf(stderr, "rpc-pipefs-generator: create systemd dependencies for nfs services\n");
+		fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
+		exit(1);
+	}
+
+	conf_path = NFS_CONFFILE;
+	conf_init();
+	for (i = 0; i < NUM_PIPEFS_USERS; i++) {
+		p = pipefs_users[i];
+		if (!p.generate)
+			continue;
+
+		ret = generate_drop_in(p.conf_name, p.unit_name,
+				       p.after_local_fs, argv[1]);
+		if (ret)
+			exit(ret);
+	}
+
+	exit(0);
+}
diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
index 7187e3c..cb2bcd4 100644
--- a/systemd/rpc-svcgssd.service
+++ b/systemd/rpc-svcgssd.service
@@ -1,8 +1,7 @@
 [Unit]
 Description=RPC security service for NFS server
 DefaultDependencies=no
-Requires=var-lib-nfs-rpc_pipefs.mount
-After=var-lib-nfs-rpc_pipefs.mount local-fs.target
+After=local-fs.target
 PartOf=nfs-server.service
 PartOf=nfs-utils.service
 
-- 
2.9.3

Re: [RFC nfs-utils PATCH 0/2] add systemd generator for the rpc_pipefs mountpoint

[NeilBrown]

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

On Fri, Mar 31 2017, Scott Mayhew wrote:

> These patches aim to make it a little easier to change the mountpoint.
> Right now if you change the pipefs-directory in /etc/nfs.conf, you still
> need to manually override the dependencies in the systemd unit files in
> order for the change to actually work.  
>
> The first patch moves rpc.idmapd's (mostly) undocumented
> pipefs-directory from /etc/idmapd.conf to /etc/nfs.conf, which rpc.gssd
> already can use for it's pipefs-directory configuration.
>
> The second patch adds a systemd generator that reads the
> pipefs-directory configurations from /etc/nfs.conf, and if they differ
> from the default it will automatically 1) create a systemd mount unit
> file for the pipefs mountpoint and 2) it will create a drop-in
> configuration file to override the Requires= and After= directives for
> that service.
>
> I did run into a bit of a snag though.  Depsite overriding the
> dependencies for both idmapd and gssd, I wind up with two pipefs
> filesystems mounted:
>
> [root@coeurl ~]# grep pipefs /proc/mounts
> sunrpc /var/lib/nfs/rpc_pipefs rpc_pipefs rw,relatime 0 0
> sunrpc /run/rpc_pipefs rpc_pipefs rw,relatime 0 0
>
> systemd still shows the dependency on the default pipefs mountpoint:
>
> [root@coeurl ~]# systemctl list-dependencies --before var-lib-nfs-rpc_pipefs.mount
> var-lib-nfs-rpc_pipefs.mount
> ● ├─nfs-idmapd.service
> ● └─rpc-gssd.service
>
> as well as the new one:
>
> [root@coeurl ~]# systemctl list-dependencies --before run-rpc_pipefs.mount
> run-rpc_pipefs.mount
> ● ├─nfs-idmapd.service
> ● └─rpc-gssd.service
>
> The drop-in configs to override the pipefs mountpoint look correct.  I'm
> clearing both Requires= and After= before setting them:
>
> [root@coeurl ~]# cat /run/systemd/generator/nfs-idmapd.service.d/10-pipefs.conf 
> # Automatically generated by rpc-pipefs-generator
>
> [Unit]
> Requires=
> Requires=run-rpc_pipefs.mount
> After=
> After=run-rpc_pipefs.mount local-fs.target
>
> [root@coeurl ~]# cat /run/systemd/generator/rpc-gssd.service.d/10-pipefs.conf 
> # Automatically generated by rpc-pipefs-generator
>
> [Unit]
> Requires=
> Requires=run-rpc_pipefs.mount
> After=
> After=run-rpc_pipefs.mount
>
> The generated mount unit file also looks correct:
>
> [root@coeurl ~]# cat /run/systemd/generator/run-rpc_pipefs.mount 
> # Automatically generated by rpc-pipefs-generator
>
> [Unit]
> Description=RPC Pipe File System
> DefaultDependencies=no
> After=systemd-tmpfiles-setup.service
> Conflicts=umount.target
>
> [Mount]
> What=sunrpc
> Where=/run/rpc_pipefs
> Type=rpc_pipefs
>
> systemd shows that the drop-in config was picked up:
>
> [root@coeurl ~]# systemctl status nfs-idmapd
> ● nfs-idmapd.service - NFSv4 ID-name mapping service
>    Loaded: loaded (/usr/lib/systemd/system/nfs-idmapd.service; static; vendor preset: disabled)
>   Drop-In: /run/systemd/generator/nfs-idmapd.service.d
>            └─10-pipefs.conf
>    Active: active (running) since Fri 2017-03-31 16:54:24 EDT; 5min ago
>   Process: 27831 ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS (code=exited, status=0/SUCCESS)
>  Main PID: 27832 (rpc.idmapd)
>     Tasks: 1 (limit: 4915)
>    CGroup: /system.slice/nfs-idmapd.service
>            └─27832 /usr/sbin/rpc.idmapd
>
> and lsof shows that the correct mountpoint is being used:
>
> [root@coeurl ~]# lsof -p 27832 2>/dev/null | grep pipefs
> rpc.idmap 27832 root   10r      DIR               0,42        0        103 /run/rpc_pipefs/nfs
>
> The same for gssd:
>
> [root@coeurl ~]# systemctl status rpc-gssd
> ● rpc-gssd.service - RPC security service for NFS client and server
>    Loaded: loaded (/usr/lib/systemd/system/rpc-gssd.service; static; vendor preset: disabled)
>   Drop-In: /run/systemd/generator/rpc-gssd.service.d
>            └─10-pipefs.conf
>    Active: active (running) since Fri 2017-03-31 16:54:29 EDT; 6min ago
>   Process: 27839 ExecStart=/usr/sbin/rpc.gssd $RPCGSSDARGS (code=exited, status=0/SUCCESS)
>  Main PID: 27840 (rpc.gssd)
>     Tasks: 1 (limit: 4915)
>    CGroup: /system.slice/rpc-gssd.service
>            └─27840 /usr/sbin/rpc.gssd
>
> [root@coeurl ~]# lsof -p 27840 2>/dev/null | grep pipefs
> rpc.gssd 27840 root  cwd       DIR               0,42        0   24637 /run/rpc_pipefs
> rpc.gssd 27840 root    7r      DIR               0,42        0   24637 /run/rpc_pipefs
> rpc.gssd 27840 root   11u     FIFO               0,42      0t0     112 /run/rpc_pipefs/gssd/clntXX/gssd
>
> So it looks like systemd is using both sets of dependencies, even though
> the programs themselves are only looking for what's specified in
> /etc/nfs.conf.  I'm not sure what to do about that.  Maybe remove the
> var-lib-nfs-rpc_pipefs.mount unit as well as the dependencies in the
> nfs-idmapd.service and rpc-gssd.service files, and have the generator
> create those automatically as well?
>

Towards the end of the systemd.unit man page is the text:

      Note that dependencies (After=, etc.) cannot be reset to an empty list,
       so dependencies can only be added in drop-ins. If you want to remove
       dependencies, you have to override the entire unit.


which is consistent with what you discovered.

Maybe create a "rpc_pipefs.target" which
  Requires=var-lib-nfs-rpc_pipefs.mount
  After=var-lib-nfs-rpc_pipefs.mount
and have all the other unit files specified their dependencies
against this target.

Then your generator would conditionally create a new "rpc_pipefs.target"
and matching foo.mount.  The new .target would depend in the foo.mount,
and the service files would already depend on that.

Might work.

Thanks,
NeilBrown


> -Scott
>
> Scott Mayhew (2):
>   idmapd: move the pipefs-directory config option to nfs.conf
>   systemd: add a generator for the rpc_pipefs mountpoint
>
>  .gitignore                     |   1 +
>  nfs.conf                       |   3 +
>  systemd/Makefile.am            |   4 +-
>  systemd/nfs.conf.man           |   9 ++
>  systemd/rpc-pipefs-generator.c | 256 +++++++++++++++++++++++++++++++++++++++++
>  systemd/rpc-svcgssd.service    |   3 +-
>  utils/idmapd/idmapd.c          |  35 +++---
>  utils/idmapd/idmapd.man        |  19 ++-
>  8 files changed, 305 insertions(+), 25 deletions(-)
>  create mode 100644 systemd/rpc-pipefs-generator.c
>
> -- 
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [RFC nfs-utils PATCH 1/2] idmapd: move the pipefs-directory config option to nfs.conf

[NeilBrown]

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

On Fri, Mar 31 2017, Scott Mayhew wrote:

> Changed idmapd to read its value for the pipefs-directory from
> /etc/nfs.conf rather than /etc/idmapd.conf.  All other configurations
> related to id mapping still reside in /etc/idmapd.conf for now.
>
> Removed the -c option, since it would be confusing as whether it should
> override nfs.conf, idmapd.conf, or both.

You didn't remove mention of the -c option from idmapd.man.
Otherwise:
  Reviewed-by: NeilBrown <neilb@suse.com>


Thanks,
NeilBrown


>
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  nfs.conf                |  3 +++
>  systemd/nfs.conf.man    |  9 +++++++++
>  utils/idmapd/idmapd.c   | 35 ++++++++++++++---------------------
>  utils/idmapd/idmapd.man | 19 ++++++++++++++++++-
>  4 files changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/nfs.conf b/nfs.conf
> index 81ece06..4359904 100644
> --- a/nfs.conf
> +++ b/nfs.conf
> @@ -17,6 +17,9 @@
>  # cred-cache-directory=
>  # preferred-realm=
>  #
> +#[idmapd]
> +# pipefs-directory=/var/lib/nfs/rpc_pipefs
> +#
>  #[lockd]
>  # port=0
>  # udp-port=0
> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
> index bdc0988..83cf84a 100644
> --- a/systemd/nfs.conf.man
> +++ b/systemd/nfs.conf.man
> @@ -215,6 +215,15 @@ See
>  for details.
>  
>  .TP
> +.B idmapd
> +Recognized values:
> +.BR pipefs-directory .
> +
> +See
> +.BR rpc.idmapd (8)
> +for details.
> +
> +.TP
>  .B svcgssd
>  Recognized values:
>  .BR principal .
> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index f4e083a..561a4b8 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -214,13 +214,11 @@ main(int argc, char **argv)
>  	struct event initialize;
>  	struct passwd *pw;
>  	struct group *gr;
> -	struct stat sb;
>  	char *xpipefsdir = NULL;
>  	int serverstart = 1, clientstart = 1;
>  	int ret;
>  	char *progname;
>  
> -	conf_path = _PATH_IDMAPDCONF;
>  	nobodyuser = NFS4NOBODY_USER;
>  	nobodygroup = NFS4NOBODY_GROUP;
>  	strlcpy(pipefsdir, PIPEFS_DIR, sizeof(pipefsdir));
> @@ -231,11 +229,9 @@ main(int argc, char **argv)
>  		progname = argv[0];
>  	xlog_open(progname);
>  
> -#define GETOPTSTR "hvfd:p:U:G:c:CS"
> +#define GETOPTSTR "hvfd:p:U:G:CS"
>  	opterr=0; /* Turn off error messages */
>  	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1) {
> -		if (opt == 'c')
> -			conf_path = optarg;
>  		if (opt == '?') {
>  			if (strchr(GETOPTSTR, optopt))
>  				warnx("'-%c' option requires an argument.", optopt);
> @@ -247,20 +243,19 @@ main(int argc, char **argv)
>  	}
>  	optind = 1;
>  
> -	if (stat(conf_path, &sb) == -1 && (errno == ENOENT || errno == EACCES)) {
> -		warn("Skipping configuration file \"%s\"", conf_path);
> -		conf_path = NULL;
> -	} else {
> -		conf_init();
> -		verbose = conf_get_num("General", "Verbosity", 0);
> -		cache_entry_expiration = conf_get_num("General",
> -				"Cache-Expiration", DEFAULT_IDMAP_CACHE_EXPIRY);
> -		CONF_SAVE(xpipefsdir, conf_get_str("General", "Pipefs-Directory"));
> -		if (xpipefsdir != NULL)
> -			strlcpy(pipefsdir, xpipefsdir, sizeof(pipefsdir));
> -		CONF_SAVE(nobodyuser, conf_get_str("Mapping", "Nobody-User"));
> -		CONF_SAVE(nobodygroup, conf_get_str("Mapping", "Nobody-Group"));
> -	}
> +	conf_path = NFS_CONFFILE;
> +	conf_init();
> +	CONF_SAVE(xpipefsdir, conf_get_str("idmapd", "pipefs-directory"));
> +	if (xpipefsdir != NULL)
> +		strlcpy(pipefsdir, xpipefsdir, sizeof(pipefsdir));
> +
> +	conf_path = _PATH_IDMAPDCONF;
> +	conf_init();
> +	verbose = conf_get_num("General", "Verbosity", 0);
> +	cache_entry_expiration = conf_get_num("General",
> +			"cache-expiration", DEFAULT_IDMAP_CACHE_EXPIRY);
> +	CONF_SAVE(nobodyuser, conf_get_str("Mapping", "Nobody-User"));
> +	CONF_SAVE(nobodygroup, conf_get_str("Mapping", "Nobody-Group"));
>  
>  	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1)
>  		switch (opt) {
> @@ -307,8 +302,6 @@ main(int argc, char **argv)
>  #ifdef HAVE_NFS4_SET_DEBUG
>  	nfs4_set_debug(verbose, xlog_warn);
>  #endif
> -	if (conf_path == NULL)
> -		conf_path = _PATH_IDMAPDCONF;
>  	if (nfs4_init_name_mapping(conf_path))
>  		errx(1, "Unable to create name to user id mappings.");
>  
> diff --git a/utils/idmapd/idmapd.man b/utils/idmapd/idmapd.man
> index d4ab894..0fbc24c 100644
> --- a/utils/idmapd/idmapd.man
> +++ b/utils/idmapd/idmapd.man
> @@ -78,6 +78,21 @@ Client-only: perform no idmapping for any NFS server, even if one is detected.
>  .It Fl S
>  Server-only: perform no idmapping for any NFS client, even if one is detected.
>  .El
> +.Sh CONFIGURATION FILES
> +The
> +.Sy [idmapd]
> +section of the
> +.Pa /etc/nfs.conf
> +configuration file recognizes the following value:
> +.Bl -tag -width Ds_imagedir
> +.It Sy pipefs-directory
> +Equivalent to
> +.Sy -p .
> +.El
> +.Pp
> +All other settings related to id mapping are found in the
> +.Pa /etc/idmapd.conf
> +configuration file.
>  .Sh EXAMPLES
>  .Cm rpc.idmapd -f -vvv
>  .Pp
> @@ -94,9 +109,11 @@ messages to console, and with a verbosity level of 3.
>  .\" This next request is for sections 1, 6, 7 & 8 only.
>  .\" .Sh ENVIRONMENT
>  .Sh FILES
> -.Pa /etc/idmapd.conf
> +.Pa /etc/idmapd.conf ,
> +.Pa /etc/nfs.conf
>  .Sh SEE ALSO
>  .Xr idmapd.conf 5 ,
> +.Xr nfs.conf 5 ,
>  .Xr nfsidmap 8
>  .\".Sh SEE ALSO
>  .\".Xr nylon.conf 4
> -- 
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [RFC nfs-utils PATCH 0/2] add systemd generator for the rpc_pipefs mountpoint

[Scott Mayhew]

On Mon, 03 Apr 2017, NeilBrown wrote:

> On Fri, Mar 31 2017, Scott Mayhew wrote:
> 
> > These patches aim to make it a little easier to change the mountpoint.
> > Right now if you change the pipefs-directory in /etc/nfs.conf, you still
> > need to manually override the dependencies in the systemd unit files in
> > order for the change to actually work.  
> >
> > The first patch moves rpc.idmapd's (mostly) undocumented
> > pipefs-directory from /etc/idmapd.conf to /etc/nfs.conf, which rpc.gssd
> > already can use for it's pipefs-directory configuration.
> >
> > The second patch adds a systemd generator that reads the
> > pipefs-directory configurations from /etc/nfs.conf, and if they differ
> > from the default it will automatically 1) create a systemd mount unit
> > file for the pipefs mountpoint and 2) it will create a drop-in
> > configuration file to override the Requires= and After= directives for
> > that service.
> >
> > I did run into a bit of a snag though.  Depsite overriding the
> > dependencies for both idmapd and gssd, I wind up with two pipefs
> > filesystems mounted:
> >
> > [root@coeurl ~]# grep pipefs /proc/mounts
> > sunrpc /var/lib/nfs/rpc_pipefs rpc_pipefs rw,relatime 0 0
> > sunrpc /run/rpc_pipefs rpc_pipefs rw,relatime 0 0
> >
> > systemd still shows the dependency on the default pipefs mountpoint:
> >
> > [root@coeurl ~]# systemctl list-dependencies --before var-lib-nfs-rpc_pipefs.mount
> > var-lib-nfs-rpc_pipefs.mount
> > ● ├─nfs-idmapd.service
> > ● └─rpc-gssd.service
> >
> > as well as the new one:
> >
> > [root@coeurl ~]# systemctl list-dependencies --before run-rpc_pipefs.mount
> > run-rpc_pipefs.mount
> > ● ├─nfs-idmapd.service
> > ● └─rpc-gssd.service
> >
> > The drop-in configs to override the pipefs mountpoint look correct.  I'm
> > clearing both Requires= and After= before setting them:
> >
> > [root@coeurl ~]# cat /run/systemd/generator/nfs-idmapd.service.d/10-pipefs.conf 
> > # Automatically generated by rpc-pipefs-generator
> >
> > [Unit]
> > Requires=
> > Requires=run-rpc_pipefs.mount
> > After=
> > After=run-rpc_pipefs.mount local-fs.target
> >
> > [root@coeurl ~]# cat /run/systemd/generator/rpc-gssd.service.d/10-pipefs.conf 
> > # Automatically generated by rpc-pipefs-generator
> >
> > [Unit]
> > Requires=
> > Requires=run-rpc_pipefs.mount
> > After=
> > After=run-rpc_pipefs.mount
> >
> > The generated mount unit file also looks correct:
> >
> > [root@coeurl ~]# cat /run/systemd/generator/run-rpc_pipefs.mount 
> > # Automatically generated by rpc-pipefs-generator
> >
> > [Unit]
> > Description=RPC Pipe File System
> > DefaultDependencies=no
> > After=systemd-tmpfiles-setup.service
> > Conflicts=umount.target
> >
> > [Mount]
> > What=sunrpc
> > Where=/run/rpc_pipefs
> > Type=rpc_pipefs
> >
> > systemd shows that the drop-in config was picked up:
> >
> > [root@coeurl ~]# systemctl status nfs-idmapd
> > ● nfs-idmapd.service - NFSv4 ID-name mapping service
> >    Loaded: loaded (/usr/lib/systemd/system/nfs-idmapd.service; static; vendor preset: disabled)
> >   Drop-In: /run/systemd/generator/nfs-idmapd.service.d
> >            └─10-pipefs.conf
> >    Active: active (running) since Fri 2017-03-31 16:54:24 EDT; 5min ago
> >   Process: 27831 ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS (code=exited, status=0/SUCCESS)
> >  Main PID: 27832 (rpc.idmapd)
> >     Tasks: 1 (limit: 4915)
> >    CGroup: /system.slice/nfs-idmapd.service
> >            └─27832 /usr/sbin/rpc.idmapd
> >
> > and lsof shows that the correct mountpoint is being used:
> >
> > [root@coeurl ~]# lsof -p 27832 2>/dev/null | grep pipefs
> > rpc.idmap 27832 root   10r      DIR               0,42        0        103 /run/rpc_pipefs/nfs
> >
> > The same for gssd:
> >
> > [root@coeurl ~]# systemctl status rpc-gssd
> > ● rpc-gssd.service - RPC security service for NFS client and server
> >    Loaded: loaded (/usr/lib/systemd/system/rpc-gssd.service; static; vendor preset: disabled)
> >   Drop-In: /run/systemd/generator/rpc-gssd.service.d
> >            └─10-pipefs.conf
> >    Active: active (running) since Fri 2017-03-31 16:54:29 EDT; 6min ago
> >   Process: 27839 ExecStart=/usr/sbin/rpc.gssd $RPCGSSDARGS (code=exited, status=0/SUCCESS)
> >  Main PID: 27840 (rpc.gssd)
> >     Tasks: 1 (limit: 4915)
> >    CGroup: /system.slice/rpc-gssd.service
> >            └─27840 /usr/sbin/rpc.gssd
> >
> > [root@coeurl ~]# lsof -p 27840 2>/dev/null | grep pipefs
> > rpc.gssd 27840 root  cwd       DIR               0,42        0   24637 /run/rpc_pipefs
> > rpc.gssd 27840 root    7r      DIR               0,42        0   24637 /run/rpc_pipefs
> > rpc.gssd 27840 root   11u     FIFO               0,42      0t0     112 /run/rpc_pipefs/gssd/clntXX/gssd
> >
> > So it looks like systemd is using both sets of dependencies, even though
> > the programs themselves are only looking for what's specified in
> > /etc/nfs.conf.  I'm not sure what to do about that.  Maybe remove the
> > var-lib-nfs-rpc_pipefs.mount unit as well as the dependencies in the
> > nfs-idmapd.service and rpc-gssd.service files, and have the generator
> > create those automatically as well?
> >
> 
> Towards the end of the systemd.unit man page is the text:
> 
>       Note that dependencies (After=, etc.) cannot be reset to an empty list,
>        so dependencies can only be added in drop-ins. If you want to remove
>        dependencies, you have to override the entire unit.
> 
> 
> which is consistent with what you discovered.

Doh, that's what I get for not reading all the way to the end.

> 
> Maybe create a "rpc_pipefs.target" which
>   Requires=var-lib-nfs-rpc_pipefs.mount
>   After=var-lib-nfs-rpc_pipefs.mount
> and have all the other unit files specified their dependencies
> against this target.
> 
> Then your generator would conditionally create a new "rpc_pipefs.target"
> and matching foo.mount.  The new .target would depend in the foo.mount,
> and the service files would already depend on that.
> 
> Might work.

That looks like it works, but what should happen then if programs
were to intentionally specify two different values for the pipefs-directory
in nfs.conf?  Or is that something that wouldn't make sense to do and
should be prevented?  I can't think of a reason why you'd want more than
one rpc_pipefs filesystem mounted...

Maybe it would make sense to create a 'global' section and have the
programs all look at that instead of looking in their specific config
sections... or maybe have the program check global section and then a
program-specific section, but include a big fat warning that if you
specify it in the program-specific section then you need to override all
of them with the same value.

-Scott
> 
> Thanks,
> NeilBrown
> 
> 
> > -Scott
> >
> > Scott Mayhew (2):
> >   idmapd: move the pipefs-directory config option to nfs.conf
> >   systemd: add a generator for the rpc_pipefs mountpoint
> >
> >  .gitignore                     |   1 +
> >  nfs.conf                       |   3 +
> >  systemd/Makefile.am            |   4 +-
> >  systemd/nfs.conf.man           |   9 ++
> >  systemd/rpc-pipefs-generator.c | 256 +++++++++++++++++++++++++++++++++++++++++
> >  systemd/rpc-svcgssd.service    |   3 +-
> >  utils/idmapd/idmapd.c          |  35 +++---
> >  utils/idmapd/idmapd.man        |  19 ++-
> >  8 files changed, 305 insertions(+), 25 deletions(-)
> >  create mode 100644 systemd/rpc-pipefs-generator.c
> >
> > -- 
> > 2.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC nfs-utils PATCH 1/2] idmapd: move the pipefs-directory config option to nfs.conf

[Steve Dickson]

Sorry for the delayed response... A long weekend ;-)

On 03/31/2017 05:56 PM, Scott Mayhew wrote:
> Changed idmapd to read its value for the pipefs-directory from
> /etc/nfs.conf rather than /etc/idmapd.conf.  All other configurations
> related to id mapping still reside in /etc/idmapd.conf for now.
> 
> Removed the -c option, since it would be confusing as whether it should
> override nfs.conf, idmapd.conf, or both.
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  nfs.conf                |  3 +++
>  systemd/nfs.conf.man    |  9 +++++++++
>  utils/idmapd/idmapd.c   | 35 ++++++++++++++---------------------
>  utils/idmapd/idmapd.man | 19 ++++++++++++++++++-
>  4 files changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/nfs.conf b/nfs.conf
> index 81ece06..4359904 100644
> --- a/nfs.conf
> +++ b/nfs.conf
> @@ -17,6 +17,9 @@
>  # cred-cache-directory=
>  # preferred-realm=
>  #
> +#[idmapd]
> +# pipefs-directory=/var/lib/nfs/rpc_pipefs
> +#
>  #[lockd]
>  # port=0
>  # udp-port=0
> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
> index bdc0988..83cf84a 100644
> --- a/systemd/nfs.conf.man
> +++ b/systemd/nfs.conf.man
> @@ -215,6 +215,15 @@ See
>  for details.
>  
>  .TP
> +.B idmapd
> +Recognized values:
> +.BR pipefs-directory .
> +
> +See
> +.BR rpc.idmapd (8)
> +for details.
> +
> +.TP
>  .B svcgssd
>  Recognized values:
>  .BR principal .
Now that the pipefs-directory define for both gssd and idmap, let move the
definition into a [global] section so it does not have to changed
in two different places. 

> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index f4e083a..561a4b8 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -214,13 +214,11 @@ main(int argc, char **argv)
>  	struct event initialize;
>  	struct passwd *pw;
>  	struct group *gr;
> -	struct stat sb;
>  	char *xpipefsdir = NULL;
>  	int serverstart = 1, clientstart = 1;
>  	int ret;
>  	char *progname;
>  
> -	conf_path = _PATH_IDMAPDCONF;
>  	nobodyuser = NFS4NOBODY_USER;
>  	nobodygroup = NFS4NOBODY_GROUP;
>  	strlcpy(pipefsdir, PIPEFS_DIR, sizeof(pipefsdir));
> @@ -231,11 +229,9 @@ main(int argc, char **argv)
>  		progname = argv[0];
>  	xlog_open(progname);
>  
> -#define GETOPTSTR "hvfd:p:U:G:c:CS"
> +#define GETOPTSTR "hvfd:p:U:G:CS"
>  	opterr=0; /* Turn off error messages */
>  	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1) {
> -		if (opt == 'c')
> -			conf_path = optarg;
Removing command line argument (aka changing API) is always a 
difficult thing... IMHO. You never know what you are going
to break... 

How about this... Instead just removing the flag, deprecated it. 
Meaning when the flag is use throw out an error message
saying the flag has be deprecated, please use /etc/nfs.conf.
Also mentioning in the man page will probably give us
enough cover :-)

BTW, thanks for do this work!

steved.

>  		if (opt == '?') {
>  			if (strchr(GETOPTSTR, optopt))
>  				warnx("'-%c' option requires an argument.", optopt);
> @@ -247,20 +243,19 @@ main(int argc, char **argv)
>  	}
>  	optind = 1;
>  
> -	if (stat(conf_path, &sb) == -1 && (errno == ENOENT || errno == EACCES)) {
> -		warn("Skipping configuration file \"%s\"", conf_path);
> -		conf_path = NULL;
> -	} else {
> -		conf_init();
> -		verbose = conf_get_num("General", "Verbosity", 0);
> -		cache_entry_expiration = conf_get_num("General",
> -				"Cache-Expiration", DEFAULT_IDMAP_CACHE_EXPIRY);
> -		CONF_SAVE(xpipefsdir, conf_get_str("General", "Pipefs-Directory"));
> -		if (xpipefsdir != NULL)
> -			strlcpy(pipefsdir, xpipefsdir, sizeof(pipefsdir));
> -		CONF_SAVE(nobodyuser, conf_get_str("Mapping", "Nobody-User"));
> -		CONF_SAVE(nobodygroup, conf_get_str("Mapping", "Nobody-Group"));
> -	}
> +	conf_path = NFS_CONFFILE;
> +	conf_init();
> +	CONF_SAVE(xpipefsdir, conf_get_str("idmapd", "pipefs-directory"));
> +	if (xpipefsdir != NULL)
> +		strlcpy(pipefsdir, xpipefsdir, sizeof(pipefsdir));
> +
> +	conf_path = _PATH_IDMAPDCONF;
> +	conf_init();
> +	verbose = conf_get_num("General", "Verbosity", 0);
> +	cache_entry_expiration = conf_get_num("General",
> +			"cache-expiration", DEFAULT_IDMAP_CACHE_EXPIRY);
> +	CONF_SAVE(nobodyuser, conf_get_str("Mapping", "Nobody-User"));
> +	CONF_SAVE(nobodygroup, conf_get_str("Mapping", "Nobody-Group"));
>  
>  	while ((opt = getopt(argc, argv, GETOPTSTR)) != -1)
>  		switch (opt) {
> @@ -307,8 +302,6 @@ main(int argc, char **argv)
>  #ifdef HAVE_NFS4_SET_DEBUG
>  	nfs4_set_debug(verbose, xlog_warn);
>  #endif
> -	if (conf_path == NULL)
> -		conf_path = _PATH_IDMAPDCONF;
>  	if (nfs4_init_name_mapping(conf_path))
>  		errx(1, "Unable to create name to user id mappings.");
>  
> diff --git a/utils/idmapd/idmapd.man b/utils/idmapd/idmapd.man
> index d4ab894..0fbc24c 100644
> --- a/utils/idmapd/idmapd.man
> +++ b/utils/idmapd/idmapd.man
> @@ -78,6 +78,21 @@ Client-only: perform no idmapping for any NFS server, even if one is detected.
>  .It Fl S
>  Server-only: perform no idmapping for any NFS client, even if one is detected.
>  .El
> +.Sh CONFIGURATION FILES
> +The
> +.Sy [idmapd]
> +section of the
> +.Pa /etc/nfs.conf
> +configuration file recognizes the following value:
> +.Bl -tag -width Ds_imagedir
> +.It Sy pipefs-directory
> +Equivalent to
> +.Sy -p .
> +.El
> +.Pp
> +All other settings related to id mapping are found in the
> +.Pa /etc/idmapd.conf
> +configuration file.
>  .Sh EXAMPLES
>  .Cm rpc.idmapd -f -vvv
>  .Pp
> @@ -94,9 +109,11 @@ messages to console, and with a verbosity level of 3.
>  .\" This next request is for sections 1, 6, 7 & 8 only.
>  .\" .Sh ENVIRONMENT
>  .Sh FILES
> -.Pa /etc/idmapd.conf
> +.Pa /etc/idmapd.conf ,
> +.Pa /etc/nfs.conf
>  .Sh SEE ALSO
>  .Xr idmapd.conf 5 ,
> +.Xr nfs.conf 5 ,
>  .Xr nfsidmap 8
>  .\".Sh SEE ALSO
>  .\".Xr nylon.conf 4
> 

Re: [RFC nfs-utils PATCH 0/2] add systemd generator for the rpc_pipefs mountpoint

[NeilBrown]

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

On Mon, Apr 03 2017, Scott Mayhew wrote:

> On Mon, 03 Apr 2017, NeilBrown wrote:
>
>> On Fri, Mar 31 2017, Scott Mayhew wrote:
>> 
>> > These patches aim to make it a little easier to change the mountpoint.
>> > Right now if you change the pipefs-directory in /etc/nfs.conf, you still
>> > need to manually override the dependencies in the systemd unit files in
>> > order for the change to actually work.  
>> >
>> > The first patch moves rpc.idmapd's (mostly) undocumented
>> > pipefs-directory from /etc/idmapd.conf to /etc/nfs.conf, which rpc.gssd
>> > already can use for it's pipefs-directory configuration.
>> >
>> > The second patch adds a systemd generator that reads the
>> > pipefs-directory configurations from /etc/nfs.conf, and if they differ
>> > from the default it will automatically 1) create a systemd mount unit
>> > file for the pipefs mountpoint and 2) it will create a drop-in
>> > configuration file to override the Requires= and After= directives for
>> > that service.
>> >
>> > I did run into a bit of a snag though.  Depsite overriding the
>> > dependencies for both idmapd and gssd, I wind up with two pipefs
>> > filesystems mounted:
>> >
>> > [root@coeurl ~]# grep pipefs /proc/mounts
>> > sunrpc /var/lib/nfs/rpc_pipefs rpc_pipefs rw,relatime 0 0
>> > sunrpc /run/rpc_pipefs rpc_pipefs rw,relatime 0 0
>> >
>> > systemd still shows the dependency on the default pipefs mountpoint:
>> >
>> > [root@coeurl ~]# systemctl list-dependencies --before var-lib-nfs-rpc_pipefs.mount
>> > var-lib-nfs-rpc_pipefs.mount
>> > ● ├─nfs-idmapd.service
>> > ● └─rpc-gssd.service
>> >
>> > as well as the new one:
>> >
>> > [root@coeurl ~]# systemctl list-dependencies --before run-rpc_pipefs.mount
>> > run-rpc_pipefs.mount
>> > ● ├─nfs-idmapd.service
>> > ● └─rpc-gssd.service
>> >
>> > The drop-in configs to override the pipefs mountpoint look correct.  I'm
>> > clearing both Requires= and After= before setting them:
>> >
>> > [root@coeurl ~]# cat /run/systemd/generator/nfs-idmapd.service.d/10-pipefs.conf 
>> > # Automatically generated by rpc-pipefs-generator
>> >
>> > [Unit]
>> > Requires=
>> > Requires=run-rpc_pipefs.mount
>> > After=
>> > After=run-rpc_pipefs.mount local-fs.target
>> >
>> > [root@coeurl ~]# cat /run/systemd/generator/rpc-gssd.service.d/10-pipefs.conf 
>> > # Automatically generated by rpc-pipefs-generator
>> >
>> > [Unit]
>> > Requires=
>> > Requires=run-rpc_pipefs.mount
>> > After=
>> > After=run-rpc_pipefs.mount
>> >
>> > The generated mount unit file also looks correct:
>> >
>> > [root@coeurl ~]# cat /run/systemd/generator/run-rpc_pipefs.mount 
>> > # Automatically generated by rpc-pipefs-generator
>> >
>> > [Unit]
>> > Description=RPC Pipe File System
>> > DefaultDependencies=no
>> > After=systemd-tmpfiles-setup.service
>> > Conflicts=umount.target
>> >
>> > [Mount]
>> > What=sunrpc
>> > Where=/run/rpc_pipefs
>> > Type=rpc_pipefs
>> >
>> > systemd shows that the drop-in config was picked up:
>> >
>> > [root@coeurl ~]# systemctl status nfs-idmapd
>> > ● nfs-idmapd.service - NFSv4 ID-name mapping service
>> >    Loaded: loaded (/usr/lib/systemd/system/nfs-idmapd.service; static; vendor preset: disabled)
>> >   Drop-In: /run/systemd/generator/nfs-idmapd.service.d
>> >            └─10-pipefs.conf
>> >    Active: active (running) since Fri 2017-03-31 16:54:24 EDT; 5min ago
>> >   Process: 27831 ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS (code=exited, status=0/SUCCESS)
>> >  Main PID: 27832 (rpc.idmapd)
>> >     Tasks: 1 (limit: 4915)
>> >    CGroup: /system.slice/nfs-idmapd.service
>> >            └─27832 /usr/sbin/rpc.idmapd
>> >
>> > and lsof shows that the correct mountpoint is being used:
>> >
>> > [root@coeurl ~]# lsof -p 27832 2>/dev/null | grep pipefs
>> > rpc.idmap 27832 root   10r      DIR               0,42        0        103 /run/rpc_pipefs/nfs
>> >
>> > The same for gssd:
>> >
>> > [root@coeurl ~]# systemctl status rpc-gssd
>> > ● rpc-gssd.service - RPC security service for NFS client and server
>> >    Loaded: loaded (/usr/lib/systemd/system/rpc-gssd.service; static; vendor preset: disabled)
>> >   Drop-In: /run/systemd/generator/rpc-gssd.service.d
>> >            └─10-pipefs.conf
>> >    Active: active (running) since Fri 2017-03-31 16:54:29 EDT; 6min ago
>> >   Process: 27839 ExecStart=/usr/sbin/rpc.gssd $RPCGSSDARGS (code=exited, status=0/SUCCESS)
>> >  Main PID: 27840 (rpc.gssd)
>> >     Tasks: 1 (limit: 4915)
>> >    CGroup: /system.slice/rpc-gssd.service
>> >            └─27840 /usr/sbin/rpc.gssd
>> >
>> > [root@coeurl ~]# lsof -p 27840 2>/dev/null | grep pipefs
>> > rpc.gssd 27840 root  cwd       DIR               0,42        0   24637 /run/rpc_pipefs
>> > rpc.gssd 27840 root    7r      DIR               0,42        0   24637 /run/rpc_pipefs
>> > rpc.gssd 27840 root   11u     FIFO               0,42      0t0     112 /run/rpc_pipefs/gssd/clntXX/gssd
>> >
>> > So it looks like systemd is using both sets of dependencies, even though
>> > the programs themselves are only looking for what's specified in
>> > /etc/nfs.conf.  I'm not sure what to do about that.  Maybe remove the
>> > var-lib-nfs-rpc_pipefs.mount unit as well as the dependencies in the
>> > nfs-idmapd.service and rpc-gssd.service files, and have the generator
>> > create those automatically as well?
>> >
>> 
>> Towards the end of the systemd.unit man page is the text:
>> 
>>       Note that dependencies (After=, etc.) cannot be reset to an empty list,
>>        so dependencies can only be added in drop-ins. If you want to remove
>>        dependencies, you have to override the entire unit.
>> 
>> 
>> which is consistent with what you discovered.
>
> Doh, that's what I get for not reading all the way to the end.
>
>> 
>> Maybe create a "rpc_pipefs.target" which
>>   Requires=var-lib-nfs-rpc_pipefs.mount
>>   After=var-lib-nfs-rpc_pipefs.mount
>> and have all the other unit files specified their dependencies
>> against this target.
>> 
>> Then your generator would conditionally create a new "rpc_pipefs.target"
>> and matching foo.mount.  The new .target would depend in the foo.mount,
>> and the service files would already depend on that.
>> 
>> Might work.
>
> That looks like it works, but what should happen then if programs
> were to intentionally specify two different values for the pipefs-directory
> in nfs.conf?  Or is that something that wouldn't make sense to do and
> should be prevented?  I can't think of a reason why you'd want more than
> one rpc_pipefs filesystem mounted...

I agree.  I cannot think of any good reason.
I'm not sure it would be such a bad idea to fix a mount point at compile
time, but not that we have run-time configuration it is safe to stick
with it.

>
> Maybe it would make sense to create a 'global' section and have the
> programs all look at that instead of looking in their specific config
> sections... or maybe have the program check global section and then a
> program-specific section, but include a big fat warning that if you
> specify it in the program-specific section then you need to override all
> of them with the same value.

I support a "global" section of rpc-pipefs mount point.
I probably should have put pipefs-directory in a "global" section to
start with.  I don't recall why I didn't.

Thanks,
NeilBrown


>
> -Scott
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>> > -Scott
>> >
>> > Scott Mayhew (2):
>> >   idmapd: move the pipefs-directory config option to nfs.conf
>> >   systemd: add a generator for the rpc_pipefs mountpoint
>> >
>> >  .gitignore                     |   1 +
>> >  nfs.conf                       |   3 +
>> >  systemd/Makefile.am            |   4 +-
>> >  systemd/nfs.conf.man           |   9 ++
>> >  systemd/rpc-pipefs-generator.c | 256 +++++++++++++++++++++++++++++++++++++++++
>> >  systemd/rpc-svcgssd.service    |   3 +-
>> >  utils/idmapd/idmapd.c          |  35 +++---
>> >  utils/idmapd/idmapd.man        |  19 ++-
>> >  8 files changed, 305 insertions(+), 25 deletions(-)
>> >  create mode 100644 systemd/rpc-pipefs-generator.c
>> >
>> > -- 
>> > 2.9.3
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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