by NeilBrown

[permalink] [raw]

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

On Wed, Apr 05 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.
>
> Added a warning to indicate that idmapd's -c option is deprecated.
>
> Signed-off-by: Scott Mayhew <[email protected]>
> ---
> nfs.conf | 3 +++
> systemd/nfs.conf.man | 9 +++++++++
> utils/idmapd/idmapd.c | 40 ++++++++++++++++++++++++++++------------
> utils/idmapd/idmapd.man | 21 ++++++++++++++++++++-
> 4 files changed, 60 insertions(+), 13 deletions(-)
>
> diff --git a/nfs.conf b/nfs.conf
> index 81ece06..ed516f5 100644
> --- a/nfs.conf
> +++ b/nfs.conf
> @@ -2,6 +2,9 @@
> # This is a general conifguration for the
> # NFS daemons and tools
> #
> +#[global]
> +# pipefs-directory=/var/lib/nfs/rpc_pipefs
> +#
> #[exportfs]
> # debug=0
> #
> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
> index bdc0988..f8849c5 100644
> --- a/systemd/nfs.conf.man
> +++ b/systemd/nfs.conf.man
> @@ -96,6 +96,15 @@ value, which can be one or more from the list
> .BR all .
> When a list is given, the members should be comma-separated.
> .TP
> +.B global
> +Recognized values:
> +.BR pipefs-directory .
> +
> +See
> +.BR rpc.idmapd (8)
> +for details.
> +
> +.TP
> .B nfsdcltrack
> Recognized values:
> .BR storagedir .
> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index f4e083a..5cac3a5 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -166,7 +166,7 @@ static uid_t nobodyuid;
> static gid_t nobodygid;
>
> /* Used by conffile.c in libnfs.a */
> -char *conf_path;
> +char *conf_path = NULL;
>
> static int
> flush_nfsd_cache(char *path, time_t now)
> @@ -220,7 +220,6 @@ main(int argc, char **argv)
> int ret;
> char *progname;
>
> - conf_path = _PATH_IDMAPDCONF;
> nobodyuser = NFS4NOBODY_USER;
> nobodygroup = NFS4NOBODY_GROUP;
> strlcpy(pipefsdir, PIPEFS_DIR, sizeof(pipefsdir));
> @@ -234,8 +233,11 @@ main(int argc, char **argv)
> #define GETOPTSTR "hvfd:p:U:G:c:CS"
> opterr=0; /* Turn off error messages */
> while ((opt = getopt(argc, argv, GETOPTSTR)) != -1) {
> - if (opt == 'c')
> + if (opt == 'c') {
> + warnx("-c is deprecated and may be removed in the "
> + "future. See idmapd(8).");
> conf_path = optarg;
> + }
> if (opt == '?') {
> if (strchr(GETOPTSTR, optopt))
> warnx("'-%c' option requires an argument.", optopt);
> @@ -247,17 +249,33 @@ 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;
> + if (conf_path) { /* deprecated -c option was specified */
> + 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"));
> + }
> } else {
> + conf_path = NFS_CONFFILE;
> 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"));
> + CONF_SAVE(xpipefsdir, conf_get_str("global", "pipefs-directory"));

Can we leave the global section of the config file named "general" -
i.e. it contains general configuration.
conf_get_str() uses strcasecmp() so "General","Pipefs-Directory" works
the same as "general","pipefs-directory".
Then gssd.c wouldn't need to be changed (unless you want to add a
deprecation warning when the gssd-specific setting is found).

Thanks,
NeilBrown

Attachments:

signature.asc (832.00 B)

2017-04-06 05:35:20

On Thu, Apr 06 2017, Scott Mayhew wrote:

> On Thu, 06 Apr 2017, NeilBrown wrote:
>
>> On Wed, Apr 05 2017, Scott Mayhew wrote:
>>
>> > The nfs.conf has config options for the rpc_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
>> > rpc_pipefs when a non-default value is specified in /etc/nfs.conf, as
>> > well as a target unit file to override the dependencies for the systemd
>> > units using the rpc_pipefs. The blkmapd, idmapd, and gssd service unit
>> > files have been modified to define their dependencies on the rpc_pipefs
>> > mountpoint indirectly via the rpc_pipefs target unit file.
>> >
>> > This patch also removes the dependency on the rpc_pipefs from the
>> > rpc-svcgssd.service unit file. rpc.svcgssd uses the sunrpc cache
>> > mechanism to exchange data with the kernel, not the rpc_pipefs.
>> >
>> > Signed-off-by: Scott Mayhew <[email protected]>
>>
>> I was reading through this, which I really thought would be a good patch
>> that I would give an Ack to, and looking at some of the detail involved,
>> I started to wonder if we were really doing the right thing here.
>> You go to some trouble to make the name of the .mount unit file match
>> the name of the location of the mountpoint. Is that really necessary?
>>
>> Unit files created by systemd-fstab-generator do follow that pattern,
>> but they don't have to.
>
>>
>> Maybe we should just:
>>
>> 1/ rename var-lib-nfs-rpc_pipefs.mount to rpc_pipefs.mount,
>> and modify the dependencies accordingly.
>>
> The mount doesn't work when I change the name.
>
> [root@fedora25 ~]# systemctl status rpc_pipefs.mount
> ● rpc_pipefs.mount - RPC Pipe File System
> Loaded: error (Reason: Invalid argument)
> Active: inactive (dead)
> Where: /var/lib/nfs/rpc_pipefs
> What: sunrpc
>
> Apr 06 08:17:59 localhost.localdomain systemd[1]: rpc_pipefs.mount: Where= setting doesn't match unit name. Refusing.
>
>
> Looking back at systemd.mount(5), it looks like they do have to follow
> that pattern:
>
> Mount units must be named after the mount point directories they control.
> Example: the mount point /home/lennart must be configured in a unit file
> home-lennart.mount. For details about the escaping logic used to convert a
> file system path to a unit name, see systemd.unit(5). Note that mount units
> cannot be templated, nor is possible to add multiple names to a mount unit
> by creating additional symlinks to it.
>

My turn not to read to the end of the man page I see...

That's a strange restriction that seem inconsistent with everything else
systemd does. Maybe it it helpful for implementing "RequiresMountsFor"
but I cannot think of any other justification.
Anyway, I guess we have to live with it.

I bothered me that you needed to include this:
>> > + fprintf(f, "DefaultDependencies=no\n");
>> > + fprintf(f, "After=systemd-tmpfiles-setup.service\n");
>> > + fprintf(f, "Conflicts=umount.target\n");

in the code, but I can't see anything we can do about that.

So:
Reviewed-by: NeilBrown <[email protected]>

for this patch.

Thanks,
NeilBrown

Attachments:

signature.asc (832.00 B)