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

[NeilBrown <neilb@suse.com>]

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

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 <smayhew@redhat.com>
> ---
>  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

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