[RFC PATCH 0/1] Enable config.d directory to be processed.

[RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

The following patch looks for config.d directories
and configuration file in those directories will
be loaded. 

For example if /etc/nfs.conf.d or /etc/nfsmount.conf.d 
exists and there are config files in those directories 
will be loaded, but not the actual /etc/nfs.conf or 
the /etc/nfsmount.conf files will not be.

I do have a couple questions/concerns 

1) Is calling conf_load_file() more than once
   kosher... It appears variable will just be 
   over written. That does appear to happen
   with my testing. 

2) If conf.d file(s) do exist, should the give
   flat conf file also be loaded. At this point if 
   the conf.d file(s) do exist, then the given
   flat config file is not loaded. 

3) How to document this new feature.

Steve Dickson (1):
  conffile: process config.d directory config files.

 support/nfs/conffile.c | 78 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH 1/1] conffile: process config.d directory config files.

[Steve Dickson]

When a /etc/nfs.conf.d or /etc/nfsmount.conf.d directory
exists and there are config file(s) in those directory
exist, those config file(s) are used instead of the given
/etc/nfs.conf and /etc/nfsmount.conf files.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 support/nfs/conffile.c | 78 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 3d13610..c60e511 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -52,6 +52,7 @@
 #include <libgen.h>
 #include <sys/file.h>
 #include <time.h>
+#include <dirent.h>
 
 #include "conffile.h"
 #include "xlog.h"
@@ -609,6 +610,69 @@ conf_load_file(const char *conf_file)
 	return 0;
 }
 
+static int 
+conf_init_dir(const char *conf_file)
+{
+	struct dirent **namelist = NULL;
+	char *dname, fname[PATH_MAX + 1];
+	int n = 0, nfiles = 0, i, fname_len, dname_len;
+
+	dname = malloc(strlen(conf_file) + 3);
+	if (dname == NULL) {
+		xlog(L_WARNING, "conf_init_dir: malloc: %s", strerror(errno));
+		return nfiles;	
+	}
+	sprintf(dname, "%s.d", conf_file);
+
+	n = scandir(dname, &namelist, NULL, versionsort);
+	if (n < 0) {
+		if (errno != ENOENT) {
+			xlog(L_WARNING, "conf_init_dir: scandir %s: %s", 
+				dname, strerror(errno));
+		}
+		free(dname);
+		return nfiles;
+	} else if (n == 0) {
+		free(dname);
+		return nfiles;
+	}
+
+	dname_len = strlen(dname);
+	for (i = 0; i < n; i++ ) {
+		struct dirent *d = namelist[i];
+
+	 	switch (d->d_type) {
+			case DT_UNKNOWN:
+			case DT_REG:
+			case DT_LNK:
+				break;
+			default:
+				continue;
+		}
+		if (*d->d_name == '.')
+			continue;
+		
+		fname_len = strlen(d->d_name);
+		if (!fname_len || (fname_len + dname_len) > PATH_MAX) {
+			xlog(L_WARNING, "conf_init_dir: Too long file name: %s in %s", 
+				d->d_name, dname);
+			continue; 
+		}
+		sprintf(fname, "%s/%s", dname, d->d_name);
+
+		if (conf_load_file(fname))
+			continue;
+		nfiles++;
+	}
+
+	for (i = 0; i < n; i++)
+		free(namelist[i]);
+	free(namelist);
+	free(dname);
+	
+	return nfiles;
+}
+
 int
 conf_init_file(const char *conf_file)
 {
@@ -619,7 +683,19 @@ conf_init_file(const char *conf_file)
 
 	TAILQ_INIT (&conf_trans_queue);
 
-	if (conf_file == NULL) conf_file=NFS_CONFFILE;
+	if (conf_file == NULL) 
+		conf_file=NFS_CONFFILE;
+
+	/* 
+	 * Check to see if there is a config directory 
+	 * if so use those file(s) if they exist
+	 */
+	if (conf_init_dir(conf_file) != 0)
+		return 0;
+
+	/*
+	 * Otherwise using the giving config file
+	 */
 	return conf_load_file(conf_file);
 }
 
-- 
2.26.2

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Alice Mitchell]

On Thu, 2020-10-29 at 17:04 -0400, Steve Dickson wrote:
> The following patch looks for config.d directories
> and configuration file in those directories will
> be loaded. 
> 
> For example if /etc/nfs.conf.d or /etc/nfsmount.conf.d 
> exists and there are config files in those directories 
> will be loaded, but not the actual /etc/nfs.conf or 
> the /etc/nfsmount.conf files will not be.
> 
> I do have a couple questions/concerns 
> 
> 1) Is calling conf_load_file() more than once
>    kosher... It appears variable will just be 
>    over written. That does appear to happen
>    with my testing. 
> 
> 2) If conf.d file(s) do exist, should the give
>    flat conf file also be loaded. At this point if 
>    the conf.d file(s) do exist, then the given
>    flat config file is not loaded. 
> 
> 3) How to document this new feature.
> 
> Steve Dickson (1):
>   conffile: process config.d directory config files.
> 
>  support/nfs/conffile.c | 78
> +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)

NAK.

Each call to conf_load_file() erases all existing config values from
memory via a call to conf_free_bindings() so this wont work as you
expect.

You would need to write an equivalent of conf_load_file() that created
a new transaction id and read in all the files before committing them
to do it this way.

I can't help but wonder if this would be better handled as an
improvement to the 'include' directive to support file globbing, we
could then retain the functionality of the master nfs.conf file but
tack on an 'include nfs.conf.d/*.conf' at the end which would go off
and load any files dropped in there by package management.

-Alice

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

On 11/2/20 8:03 AM, Alice Mitchell wrote:
> On Thu, 2020-10-29 at 17:04 -0400, Steve Dickson wrote:
>> The following patch looks for config.d directories
>> and configuration file in those directories will
>> be loaded. 
>>
>> For example if /etc/nfs.conf.d or /etc/nfsmount.conf.d 
>> exists and there are config files in those directories 
>> will be loaded, but not the actual /etc/nfs.conf or 
>> the /etc/nfsmount.conf files will not be.
>>
>> I do have a couple questions/concerns 
>>
>> 1) Is calling conf_load_file() more than once
>>    kosher... It appears variable will just be 
>>    over written. That does appear to happen
>>    with my testing. 
>>
>> 2) If conf.d file(s) do exist, should the give
>>    flat conf file also be loaded. At this point if 
>>    the conf.d file(s) do exist, then the given
>>    flat config file is not loaded. 
>>
>> 3) How to document this new feature.
>>
>> Steve Dickson (1):
>>   conffile: process config.d directory config files.
>>
>>  support/nfs/conffile.c | 78
>> +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
> NAK.
> 
> Each call to conf_load_file() erases all existing config values from
> memory via a call to conf_free_bindings() so this wont work as you
> expect.
I'm glad I asked.

> 
> You would need to write an equivalent of conf_load_file() that created
> a new transaction id and read in all the files before committing them
> to do it this way.
I kinda do think we should be able to read in multiple files...
If that free was not done until all the files are read in, would something
like that work? I guess I'm ask how difficult would be to re-work
the code to do something like this. 

> 
> I can't help but wonder if this would be better handled as an
> improvement to the 'include' directive to support file globbing, we
> could then retain the functionality of the master nfs.conf file but
> tack on an 'include nfs.conf.d/*.conf' at the end which would go off
> and load any files dropped in there by package management.
Well... there is no include for /etc/nfsmount.conf although we 
could probably add one. But I'm thinking most admins are expecting 
to simply drop a config in a .d directory to have the configuration
take effect. Editing a file is a bit more difficult... IMHO.

steved. 

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

Hello,

On 11/2/20 8:24 AM, Steve Dickson wrote:
>> You would need to write an equivalent of conf_load_file() that created
>> a new transaction id and read in all the files before committing them
>> to do it this way.
> I kinda do think we should be able to read in multiple files...
> If that free was not done until all the files are read in, would something
> like that work? I guess I'm ask how difficult would be to re-work
> the code to do something like this. 
> 
Something similar to this... load all the files under the same trans id:
(Compiled tested):
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index c60e511..f003fe1 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -578,6 +578,30 @@ static void conf_free_bindings(void)
 	}
 }
 
+static int
+conf_load_files(int trans, const char *conf_file)
+{
+	char *conf_data;
+	char *section = NULL;
+	char *subsection = NULL;
+
+	conf_data = conf_readfile(conf_file);
+	if (conf_data == NULL)
+		return 1;
+
+	/* Load default configuration values.  */
+	conf_load_defaults();
+
+	/* Parse config contents into the transaction queue */
+	conf_parse(trans, conf_data, &section, &subsection, conf_file);
+	if (section) 
+		free(section);
+	if (subsection) 
+		free(subsection);
+	free(conf_data);
+
+	return 0;
+}
 /* Open the config file and map it into our address space, then parse it.  */
 static int
 conf_load_file(const char *conf_file)
@@ -616,6 +640,7 @@ conf_init_dir(const char *conf_file)
 	struct dirent **namelist = NULL;
 	char *dname, fname[PATH_MAX + 1];
 	int n = 0, nfiles = 0, i, fname_len, dname_len;
+	int trans;
 
 	dname = malloc(strlen(conf_file) + 3);
 	if (dname == NULL) {
@@ -637,6 +662,7 @@ conf_init_dir(const char *conf_file)
 		return nfiles;
 	}
 
+	trans = conf_begin();
 	dname_len = strlen(dname);
 	for (i = 0; i < n; i++ ) {
 		struct dirent *d = namelist[i];
@@ -660,11 +686,17 @@ conf_init_dir(const char *conf_file)
 		}
 		sprintf(fname, "%s/%s", dname, d->d_name);
 
-		if (conf_load_file(fname))
+		if (conf_load_files(trans, fname))
 			continue;
 		nfiles++;
 	}
 
+	/* Free potential existing configuration.  */
+	conf_free_bindings();
+
+	/* Apply the new configuration values */
+	conf_end(trans, 1);
+
 	for (i = 0; i < n; i++)
 		free(namelist[i]);
 	free(namelist);

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Alice Mitchell]

Hi Steve,
That should work yes, although I am still dubious of the merits of
replacing the single config file with multiple ones rather than reading
them in addition to it. Surely this is going to lead to queries of why
the main config file is being ignored just because the directory also
existed.

I also have concerns that blindly loading -every- file in the directory
is also going to lead to problems, such as foo.conf.rpmorig files and
the like.  This is why i suggested a glob would be a better solution

-Alice


On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
> Hello,
> 
> On 11/2/20 8:24 AM, Steve Dickson wrote:
> > > You would need to write an equivalent of conf_load_file() that
> > > created
> > > a new transaction id and read in all the files before committing
> > > them
> > > to do it this way.
> > I kinda do think we should be able to read in multiple files...
> > If that free was not done until all the files are read in, would
> > something
> > like that work? I guess I'm ask how difficult would be to re-work
> > the code to do something like this. 
> > 
> Something similar to this... load all the files under the same trans
> id:
> (Compiled tested):
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index c60e511..f003fe1 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -578,6 +578,30 @@ static void conf_free_bindings(void)
>  	}
>  }
>  
> +static int
> +conf_load_files(int trans, const char *conf_file)
> +{
> +	char *conf_data;
> +	char *section = NULL;
> +	char *subsection = NULL;
> +
> +	conf_data = conf_readfile(conf_file);
> +	if (conf_data == NULL)
> +		return 1;
> +
> +	/* Load default configuration values.  */
> +	conf_load_defaults();
> +
> +	/* Parse config contents into the transaction queue */
> +	conf_parse(trans, conf_data, &section, &subsection, conf_file);
> +	if (section) 
> +		free(section);
> +	if (subsection) 
> +		free(subsection);
> +	free(conf_data);
> +
> +	return 0;
> +}
>  /* Open the config file and map it into our address space, then
> parse it.  */
>  static int
>  conf_load_file(const char *conf_file)
> @@ -616,6 +640,7 @@ conf_init_dir(const char *conf_file)
>  	struct dirent **namelist = NULL;
>  	char *dname, fname[PATH_MAX + 1];
>  	int n = 0, nfiles = 0, i, fname_len, dname_len;
> +	int trans;
>  
>  	dname = malloc(strlen(conf_file) + 3);
>  	if (dname == NULL) {
> @@ -637,6 +662,7 @@ conf_init_dir(const char *conf_file)
>  		return nfiles;
>  	}
>  
> +	trans = conf_begin();
>  	dname_len = strlen(dname);
>  	for (i = 0; i < n; i++ ) {
>  		struct dirent *d = namelist[i];
> @@ -660,11 +686,17 @@ conf_init_dir(const char *conf_file)
>  		}
>  		sprintf(fname, "%s/%s", dname, d->d_name);
>  
> -		if (conf_load_file(fname))
> +		if (conf_load_files(trans, fname))
>  			continue;
>  		nfiles++;
>  	}
>  
> +	/* Free potential existing configuration.  */
> +	conf_free_bindings();
> +
> +	/* Apply the new configuration values */
> +	conf_end(trans, 1);
> +
>  	for (i = 0; i < n; i++)
>  		free(namelist[i]);
>  	free(namelist);
> 

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Chuck Lever]

> On Nov 2, 2020, at 10:05 AM, Alice Mitchell <ajmitchell@redhat.com> wrote:
> 
> Hi Steve,
> That should work yes, although I am still dubious of the merits of
> replacing the single config file with multiple ones rather than reading
> them in addition to it. Surely this is going to lead to queries of why
> the main config file is being ignored just because the directory also
> existed.

I would follow the pattern used in other tools. How does /etc/exports.d/
work, for example?


> I also have concerns that blindly loading -every- file in the directory
> is also going to lead to problems, such as foo.conf.rpmorig files and
> the like.  This is why i suggested a glob would be a better solution

The usual behavior used by other tools is to load only files that match
a particular file extension. That way, files can be left in place in the
.d directory, but disabled, by simply renaming them.


> -Alice
> 
> 
> On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
>> Hello,
>> 
>> On 11/2/20 8:24 AM, Steve Dickson wrote:
>>>> You would need to write an equivalent of conf_load_file() that
>>>> created
>>>> a new transaction id and read in all the files before committing
>>>> them
>>>> to do it this way.
>>> I kinda do think we should be able to read in multiple files...
>>> If that free was not done until all the files are read in, would
>>> something
>>> like that work? I guess I'm ask how difficult would be to re-work
>>> the code to do something like this. 
>>> 
>> Something similar to this... load all the files under the same trans
>> id:
>> (Compiled tested):
>> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
>> index c60e511..f003fe1 100644
>> --- a/support/nfs/conffile.c
>> +++ b/support/nfs/conffile.c
>> @@ -578,6 +578,30 @@ static void conf_free_bindings(void)
>> 	}
>> }
>> 
>> +static int
>> +conf_load_files(int trans, const char *conf_file)
>> +{
>> +	char *conf_data;
>> +	char *section = NULL;
>> +	char *subsection = NULL;
>> +
>> +	conf_data = conf_readfile(conf_file);
>> +	if (conf_data == NULL)
>> +		return 1;
>> +
>> +	/* Load default configuration values.  */
>> +	conf_load_defaults();
>> +
>> +	/* Parse config contents into the transaction queue */
>> +	conf_parse(trans, conf_data, &section, &subsection, conf_file);
>> +	if (section) 
>> +		free(section);
>> +	if (subsection) 
>> +		free(subsection);
>> +	free(conf_data);
>> +
>> +	return 0;
>> +}
>> /* Open the config file and map it into our address space, then
>> parse it.  */
>> static int
>> conf_load_file(const char *conf_file)
>> @@ -616,6 +640,7 @@ conf_init_dir(const char *conf_file)
>> 	struct dirent **namelist = NULL;
>> 	char *dname, fname[PATH_MAX + 1];
>> 	int n = 0, nfiles = 0, i, fname_len, dname_len;
>> +	int trans;
>> 
>> 	dname = malloc(strlen(conf_file) + 3);
>> 	if (dname == NULL) {
>> @@ -637,6 +662,7 @@ conf_init_dir(const char *conf_file)
>> 		return nfiles;
>> 	}
>> 
>> +	trans = conf_begin();
>> 	dname_len = strlen(dname);
>> 	for (i = 0; i < n; i++ ) {
>> 		struct dirent *d = namelist[i];
>> @@ -660,11 +686,17 @@ conf_init_dir(const char *conf_file)
>> 		}
>> 		sprintf(fname, "%s/%s", dname, d->d_name);
>> 
>> -		if (conf_load_file(fname))
>> +		if (conf_load_files(trans, fname))
>> 			continue;
>> 		nfiles++;
>> 	}
>> 
>> +	/* Free potential existing configuration.  */
>> +	conf_free_bindings();
>> +
>> +	/* Apply the new configuration values */
>> +	conf_end(trans, 1);
>> +
>> 	for (i = 0; i < n; i++)
>> 		free(namelist[i]);
>> 	free(namelist);
>> 
> 

--
Chuck Lever

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

Hello,

On 11/2/20 10:16 AM, Chuck Lever wrote:
> 
> 
>> On Nov 2, 2020, at 10:05 AM, Alice Mitchell <ajmitchell@redhat.com> wrote:
>>
>> Hi Steve,
>> That should work yes, although I am still dubious of the merits of
>> replacing the single config file with multiple ones rather than reading
>> them in addition to it. Surely this is going to lead to queries of why
>> the main config file is being ignored just because the directory also
>> existed.
> 
> I would follow the pattern used in other tools. How does /etc/exports.d/
> work, for example?
I pattern my patch on how exports.d works today. Scandir() all the 
dir-entries then process each file. 
 
> 
> 
>> I also have concerns that blindly loading -every- file in the directory
>> is also going to lead to problems, such as foo.conf.rpmorig files and
>> the like.  This is why i suggested a glob would be a better solution
> 
> The usual behavior used by other tools is to load only files that match
> a particular file extension. That way, files can be left in place in the
> .d directory, but disabled, by simply renaming them.
Interesting... So only process *.conf files? Ex. 001-nfs.conf, 002-nfs.conf??

steved.

> 
> 
>> -Alice
>>
>>
>> On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
>>> Hello,
>>>
>>> On 11/2/20 8:24 AM, Steve Dickson wrote:
>>>>> You would need to write an equivalent of conf_load_file() that
>>>>> created
>>>>> a new transaction id and read in all the files before committing
>>>>> them
>>>>> to do it this way.
>>>> I kinda do think we should be able to read in multiple files...
>>>> If that free was not done until all the files are read in, would
>>>> something
>>>> like that work? I guess I'm ask how difficult would be to re-work
>>>> the code to do something like this. 
>>>>
>>> Something similar to this... load all the files under the same trans
>>> id:
>>> (Compiled tested):
>>> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
>>> index c60e511..f003fe1 100644
>>> --- a/support/nfs/conffile.c
>>> +++ b/support/nfs/conffile.c
>>> @@ -578,6 +578,30 @@ static void conf_free_bindings(void)
>>> 	}
>>> }
>>>
>>> +static int
>>> +conf_load_files(int trans, const char *conf_file)
>>> +{
>>> +	char *conf_data;
>>> +	char *section = NULL;
>>> +	char *subsection = NULL;
>>> +
>>> +	conf_data = conf_readfile(conf_file);
>>> +	if (conf_data == NULL)
>>> +		return 1;
>>> +
>>> +	/* Load default configuration values.  */
>>> +	conf_load_defaults();
>>> +
>>> +	/* Parse config contents into the transaction queue */
>>> +	conf_parse(trans, conf_data, &section, &subsection, conf_file);
>>> +	if (section) 
>>> +		free(section);
>>> +	if (subsection) 
>>> +		free(subsection);
>>> +	free(conf_data);
>>> +
>>> +	return 0;
>>> +}
>>> /* Open the config file and map it into our address space, then
>>> parse it.  */
>>> static int
>>> conf_load_file(const char *conf_file)
>>> @@ -616,6 +640,7 @@ conf_init_dir(const char *conf_file)
>>> 	struct dirent **namelist = NULL;
>>> 	char *dname, fname[PATH_MAX + 1];
>>> 	int n = 0, nfiles = 0, i, fname_len, dname_len;
>>> +	int trans;
>>>
>>> 	dname = malloc(strlen(conf_file) + 3);
>>> 	if (dname == NULL) {
>>> @@ -637,6 +662,7 @@ conf_init_dir(const char *conf_file)
>>> 		return nfiles;
>>> 	}
>>>
>>> +	trans = conf_begin();
>>> 	dname_len = strlen(dname);
>>> 	for (i = 0; i < n; i++ ) {
>>> 		struct dirent *d = namelist[i];
>>> @@ -660,11 +686,17 @@ conf_init_dir(const char *conf_file)
>>> 		}
>>> 		sprintf(fname, "%s/%s", dname, d->d_name);
>>>
>>> -		if (conf_load_file(fname))
>>> +		if (conf_load_files(trans, fname))
>>> 			continue;
>>> 		nfiles++;
>>> 	}
>>>
>>> +	/* Free potential existing configuration.  */
>>> +	conf_free_bindings();
>>> +
>>> +	/* Apply the new configuration values */
>>> +	conf_end(trans, 1);
>>> +
>>> 	for (i = 0; i < n; i++)
>>> 		free(namelist[i]);
>>> 	free(namelist);
>>>
>>
> 
> --
> Chuck Lever
> 
> 
> 

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

Hey!

On 11/2/20 10:05 AM, Alice Mitchell wrote:
> That should work yes, although I am still dubious of the merits of
> replacing the single config file with multiple ones rather than reading
> them in addition to it. Surely this is going to lead to queries of why
> the main config file is being ignored just because the directory also
> existed.
Isn't that how Ansible works? They drop in its own config file assuming 
main config file is ignored (or even removed)?
 

> 
> I also have concerns that blindly loading -every- file in the directory
> is also going to lead to problems, such as foo.conf.rpmorig files and
> the like.  This is why i suggested a glob would be a better solution
Maybe used mrchuck's suggestion only process *.conf files? 

steved.

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Alice Mitchell]

On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
> Hello,
> 
> On 11/2/20 8:24 AM, Steve Dickson wrote:
> > > You would need to write an equivalent of conf_load_file() that
> > > created a new transaction id and read in all the files before
> > > committing them to do it this way.
> > 

How about the following as an alternative...

It changes none of the past behaviour, but if you wanted to add an
optional directory structure to a config file then simply add this to
the default single config file that we ship.

/etc/nfsmount.conf:
[NFSMount_Global_Options]
include=-/etc/nfsmount.conf.d/*.conf


The leading minus tells it that it isnt an error if its empty, and it
will load all of the fragments it finds in there as well as the
existing single file.  Apply the same structure to any existing config
file that you want to also have a directory for.

-Alice



diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 58c03911..aade50c8 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -53,6 +53,7 @@
 #include <libgen.h>
 #include <sys/file.h>
 #include <time.h>
+#include <glob.h>
 
 #include "conffile.h"
 #include "xlog.h"
@@ -690,6 +691,7 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 	if (strcasecmp(line, "include")==0) {
 		/* load and parse subordinate config files */
 		_Bool optional = false;
+		glob_t globdata;
 
 		if (val && *val == '-') {
 			optional = true;
@@ -704,33 +706,46 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 			return;
 		}
 
-		subconf = conf_readfile(relpath);
-		if (subconf == NULL) {
-			if (!optional)
-				xlog_warn("config error at %s:%d: error loading included config",
-					  filename, lineno);
-			if (relpath)
-				free(relpath);
-			return;
-		}
+		if (glob(relpath, GLOB_MARK|GLOB_NOCHECK, NULL, &globdata)) {
+			xlog_warn("config error at %s:%d: error with matching pattern", filename, lineno);
+		} else 
+		{
+			for (size_t g=0; g<globdata.gl_pathc; g++) {
+				const char * subpath = globdata.gl_pathv[g];
 
-		/* copy the section data so the included file can inherit it
-		 * without accidentally changing it for us */
-		if (*section != NULL) {
-			inc_section = strdup(*section);
-			if (*subsection != NULL)
-				inc_subsection = strdup(*subsection);
-		}
+				if (subpath[strlen(subpath)-1] == '/') {
+					continue;
+				}
+				subconf = conf_readfile(subpath);
+				if (subconf == NULL) {
+					if (!optional)
+						xlog_warn("config error at %s:%d: error loading included config",
+							  filename, lineno);
+					if (relpath)
+						free(relpath);
+					return;
+				}
+
+				/* copy the section data so the included file can inherit it
+				 * without accidentally changing it for us */
+				if (*section != NULL) {
+					inc_section = strdup(*section);
+					if (*subsection != NULL)
+						inc_subsection = strdup(*subsection);
+				}
 
-		conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
+				conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
 
-		if (inc_section)
-			free(inc_section);
-		if (inc_subsection)
-			free(inc_subsection);
+				if (inc_section)
+					free(inc_section);
+				if (inc_subsection)
+					free(inc_subsection);
+				free(subconf);
+			}
+		}
 		if (relpath)
 			free(relpath);
-		free(subconf);
+		globfree(&globdata);
 	} else {
 		/* XXX Perhaps should we not ignore errors?  */
 		conf_set(trans, *section, *subsection, line, val, 0, 0);

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

Hello,

On 11/2/20 10:57 AM, Alice Mitchell wrote:
> On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
>> Hello,
>>
>> On 11/2/20 8:24 AM, Steve Dickson wrote:
>>>> You would need to write an equivalent of conf_load_file() that
>>>> created a new transaction id and read in all the files before
>>>> committing them to do it this way.
>>>
> 
> How about the following as an alternative...
> 
> It changes none of the past behaviour, but if you wanted to add an
> optional directory structure to a config file then simply add this to
> the default single config file that we ship.
> 
> /etc/nfsmount.conf:
> [NFSMount_Global_Options]
> include=-/etc/nfsmount.conf.d/*.conf
If it was this simple I would go for it... 
but it just not work... as expected. Here is why.

In relative_path() looks at the new file 
(/etc/nfsmount.conf.d/*.conf). If the path starts
with '/',  the path is strdup-ed and returned.

The '*' is never expanded. Even if it was... there
no code (that I see) that will process multiple
files.   TBL... This works 
include=-/etc/nfsmount.conf.d/nfsmount.conf

This doesn't 
include=-/etc/nfsmount.conf.d/*.conf

Also I don't know if it is a good idea to have the sub configs
dependent on the main config file. If the main config is remove
the sub config will never be seen. Is that a good thing?

I'm thinking we go with processing the sub configs separate 
from the main config and allow multiple sub configs be processed 
if they end with ".config" (mrchuck's suggestion).

Then document all this in the man pages.  

The last question should the main config be process,
not at all or after or before the sub configs?

steved.
  
> 
> 
> The leading minus tells it that it isnt an error if its empty, and it
> will load all of the fragments it finds in there as well as the
> existing single file.  Apply the same structure to any existing config
> file that you want to also have a directory for.
> 
> -Alice
> 
> 
> 
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index 58c03911..aade50c8 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -53,6 +53,7 @@
>  #include <libgen.h>
>  #include <sys/file.h>
>  #include <time.h>
> +#include <glob.h>
>  
>  #include "conffile.h"
>  #include "xlog.h"
> @@ -690,6 +691,7 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
>  	if (strcasecmp(line, "include")==0) {
>  		/* load and parse subordinate config files */
>  		_Bool optional = false;
> +		glob_t globdata;
>  
>  		if (val && *val == '-') {
>  			optional = true;
> @@ -704,33 +706,46 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
>  			return;
>  		}
>  
> -		subconf = conf_readfile(relpath);
> -		if (subconf == NULL) {
> -			if (!optional)
> -				xlog_warn("config error at %s:%d: error loading included config",
> -					  filename, lineno);
> -			if (relpath)
> -				free(relpath);
> -			return;
> -		}
> +		if (glob(relpath, GLOB_MARK|GLOB_NOCHECK, NULL, &globdata)) {
> +			xlog_warn("config error at %s:%d: error with matching pattern", filename, lineno);
> +		} else 
> +		{
> +			for (size_t g=0; g<globdata.gl_pathc; g++) {
> +				const char * subpath = globdata.gl_pathv[g];
>  
> -		/* copy the section data so the included file can inherit it
> -		 * without accidentally changing it for us */
> -		if (*section != NULL) {
> -			inc_section = strdup(*section);
> -			if (*subsection != NULL)
> -				inc_subsection = strdup(*subsection);
> -		}
> +				if (subpath[strlen(subpath)-1] == '/') {
> +					continue;
> +				}
> +				subconf = conf_readfile(subpath);
> +				if (subconf == NULL) {
> +					if (!optional)
> +						xlog_warn("config error at %s:%d: error loading included config",
> +							  filename, lineno);
> +					if (relpath)
> +						free(relpath);
> +					return;
> +				}
> +
> +				/* copy the section data so the included file can inherit it
> +				 * without accidentally changing it for us */
> +				if (*section != NULL) {
> +					inc_section = strdup(*section);
> +					if (*subsection != NULL)
> +						inc_subsection = strdup(*subsection);
> +				}
>  
> -		conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
> +				conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
>  
> -		if (inc_section)
> -			free(inc_section);
> -		if (inc_subsection)
> -			free(inc_subsection);
> +				if (inc_section)
> +					free(inc_section);
> +				if (inc_subsection)
> +					free(inc_subsection);
> +				free(subconf);
> +			}
> +		}
>  		if (relpath)
>  			free(relpath);
> -		free(subconf);
> +		globfree(&globdata);
>  	} else {
>  		/* XXX Perhaps should we not ignore errors?  */
>  		conf_set(trans, *section, *subsection, line, val, 0, 0);
> 
> 

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[McIntyre, Vincent (CASS, Marsfield)]

Would it be useful at this point to look at how autofs handles this
and contemplate an implementation consistent with the way that works?

Kind regards
Vince

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Alice Mitchell]

I know the code doesn't look like very much but it does do exactly what
I suggest, and i feel is quite a simple and elegant solution that is
inline with what many of services do.

relative_path() is just as it suggests only for building relative
paths, whatever comes out of it, either the constructed relative path
or the untouched absolute one gets handed to glob(3) which builds a
list of files which match the wildcard pattern given, the for() loop
around conf_readfile()/conf_parse() loads all of the contents of those
files into the current transaction as if it was one big config file,
the wildcard pattern will also do the file extension matching that
Chuck suggested.

Looking around many other services handle config directories in the
same way, not all admittedly, but things like apache always handled
their config this way and at Vincents suggestion I checked and this is
also how autofs handles it, a directive in the main config file that
loads a subdirectory.

/etc/auto.master :
# Include /etc/auto.master.d/*.autofs
# The included files must conform to the format of this file.
#
+dir:/etc/auto.master.d

So the patch i included adds comparable functionality to all of the NFS
tools, you simply add the include line to the master config files that
require directory configs as well.

-Alice



On Mon, 2020-11-02 at 14:42 -0500, Steve Dickson wrote:
> Hello,
> 
> On 11/2/20 10:57 AM, Alice Mitchell wrote:
> > On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
> > > Hello,
> > > 
> > > On 11/2/20 8:24 AM, Steve Dickson wrote:
> > > > > You would need to write an equivalent of conf_load_file()
> > > > > that
> > > > > created a new transaction id and read in all the files before
> > > > > committing them to do it this way.
> > 
> > How about the following as an alternative...
> > 
> > It changes none of the past behaviour, but if you wanted to add an
> > optional directory structure to a config file then simply add this
> > to
> > the default single config file that we ship.
> > 
> > /etc/nfsmount.conf:
> > [NFSMount_Global_Options]
> > include=-/etc/nfsmount.conf.d/*.conf
> If it was this simple I would go for it... 
> but it just not work... as expected. Here is why.
> 
> In relative_path() looks at the new file 
> (/etc/nfsmount.conf.d/*.conf). If the path starts
> with '/',  the path is strdup-ed and returned.
> 
> The '*' is never expanded. Even if it was... there
> no code (that I see) that will process multiple
> files.   TBL... This works 
> include=-/etc/nfsmount.conf.d/nfsmount.conf
> 
> This doesn't 
> include=-/etc/nfsmount.conf.d/*.conf
> 
> Also I don't know if it is a good idea to have the sub configs
> dependent on the main config file. If the main config is remove
> the sub config will never be seen. Is that a good thing?
> 
> I'm thinking we go with processing the sub configs separate 
> from the main config and allow multiple sub configs be processed 
> if they end with ".config" (mrchuck's suggestion).
> 
> Then document all this in the man pages.  
> 
> The last question should the main config be process,
> not at all or after or before the sub configs?
> 
> steved.
>   
> > 
> > The leading minus tells it that it isnt an error if its empty, and
> > it
> > will load all of the fragments it finds in there as well as the
> > existing single file.  Apply the same structure to any existing
> > config
> > file that you want to also have a directory for.
> > 
> > -Alice
> > 
> > 
> > 
> > diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> > index 58c03911..aade50c8 100644
> > --- a/support/nfs/conffile.c
> > +++ b/support/nfs/conffile.c
> > @@ -53,6 +53,7 @@
> >  #include <libgen.h>
> >  #include <sys/file.h>
> >  #include <time.h>
> > +#include <glob.h>
> >  
> >  #include "conffile.h"
> >  #include "xlog.h"
> > @@ -690,6 +691,7 @@ conf_parse_line(int trans, char *line, const
> > char *filename, int lineno, char **
> >  	if (strcasecmp(line, "include")==0) {
> >  		/* load and parse subordinate config files */
> >  		_Bool optional = false;
> > +		glob_t globdata;
> >  
> >  		if (val && *val == '-') {
> >  			optional = true;
> > @@ -704,33 +706,46 @@ conf_parse_line(int trans, char *line, const
> > char *filename, int lineno, char **
> >  			return;
> >  		}
> >  
> > -		subconf = conf_readfile(relpath);
> > -		if (subconf == NULL) {
> > -			if (!optional)
> > -				xlog_warn("config error at %s:%d: error
> > loading included config",
> > -					  filename, lineno);
> > -			if (relpath)
> > -				free(relpath);
> > -			return;
> > -		}
> > +		if (glob(relpath, GLOB_MARK|GLOB_NOCHECK, NULL,
> > &globdata)) {
> > +			xlog_warn("config error at %s:%d: error with
> > matching pattern", filename, lineno);
> > +		} else 
> > +		{
> > +			for (size_t g=0; g<globdata.gl_pathc; g++) {
> > +				const char * subpath =
> > globdata.gl_pathv[g];
> >  
> > -		/* copy the section data so the included file can
> > inherit it
> > -		 * without accidentally changing it for us */
> > -		if (*section != NULL) {
> > -			inc_section = strdup(*section);
> > -			if (*subsection != NULL)
> > -				inc_subsection = strdup(*subsection);
> > -		}
> > +				if (subpath[strlen(subpath)-1] == '/')
> > {
> > +					continue;
> > +				}
> > +				subconf = conf_readfile(subpath);
> > +				if (subconf == NULL) {
> > +					if (!optional)
> > +						xlog_warn("config error
> > at %s:%d: error loading included config",
> > +							  filename,
> > lineno);
> > +					if (relpath)
> > +						free(relpath);
> > +					return;
> > +				}
> > +
> > +				/* copy the section data so the
> > included file can inherit it
> > +				 * without accidentally changing it for
> > us */
> > +				if (*section != NULL) {
> > +					inc_section = strdup(*section);
> > +					if (*subsection != NULL)
> > +						inc_subsection =
> > strdup(*subsection);
> > +				}
> >  
> > -		conf_parse(trans, subconf, &inc_section,
> > &inc_subsection, relpath);
> > +				conf_parse(trans, subconf,
> > &inc_section, &inc_subsection, relpath);
> >  
> > -		if (inc_section)
> > -			free(inc_section);
> > -		if (inc_subsection)
> > -			free(inc_subsection);
> > +				if (inc_section)
> > +					free(inc_section);
> > +				if (inc_subsection)
> > +					free(inc_subsection);
> > +				free(subconf);
> > +			}
> > +		}
> >  		if (relpath)
> >  			free(relpath);
> > -		free(subconf);
> > +		globfree(&globdata);
> >  	} else {
> >  		/* XXX Perhaps should we not ignore errors?  */
> >  		conf_set(trans, *section, *subsection, line, val, 0,
> > 0);
> > 
> > 

Re: [RFC PATCH 0/1] Enable config.d directory to be processed.

[Steve Dickson]

On 11/3/20 5:14 AM, Alice Mitchell wrote:
> I know the code doesn't look like very much but it does do exactly what
> I suggest, and i feel is quite a simple and elegant solution that is
> inline with what many of services do.
I'm not saying the include does not work... 
Its just the example you gave does not work.

I mistakenly include the sub conf in the sub conf
which cause an infinite loop... but again that was
a pilot error on my part.

> 
> relative_path() is just as it suggests only for building relative
> paths, whatever comes out of it, either the constructed relative path
> or the untouched absolute one gets handed to glob(3) which builds a
> list of files which match the wildcard pattern given, the for() loop
> around conf_readfile()/conf_parse() loads all of the contents of those
> files into the current transaction as if it was one big config file,
> the wildcard pattern will also do the file extension matching that
> Chuck suggested.
Here is what I'm seeing:
mount -v steved-fedora:/home/tmp /mnt/tmp
conf_readfile: stat(/etc/nfsmount.conf) 0 errno 2
relative_path: newfile '/etc/nfsmount.conf.d/*.conf'
conf_parse_line: relpath '/etc/nfsmount.conf.d/*.conf'
conf_readfile: stat(/etc/nfsmount.conf.d/*.conf) -1 errno 2 
conf_parse_line: subconf '(null)'
^^^^ this NULL stop the processing so the vers=4 mount option 
is never processed in the sub config file. 

> 
> Looking around many other services handle config directories in the
> same way, not all admittedly, but things like apache always handled
> their config this way and at Vincents suggestion I checked and this is
> also how autofs handles it, a directive in the main config file that
> loads a subdirectory.
> 
> /etc/auto.master :
> # Include /etc/auto.master.d/*.autofs
> # The included files must conform to the format of this file.
> #
> +dir:/etc/auto.master.d
Question I have is... Do all the files under /etc/auto.master.d
get processed?

> 
> So the patch i included adds comparable functionality to all of the NFS
> tools, you simply add the include line to the master config files that
> require directory configs as well.
Well not all of the tools... As said before, I pattern my patch
after what exportfs does with /etc/export.d. 

steved.
> 
> -Alice
> 
> 
> 
> On Mon, 2020-11-02 at 14:42 -0500, Steve Dickson wrote:
>> Hello,
>>
>> On 11/2/20 10:57 AM, Alice Mitchell wrote:
>>> On Mon, 2020-11-02 at 09:23 -0500, Steve Dickson wrote:
>>>> Hello,
>>>>
>>>> On 11/2/20 8:24 AM, Steve Dickson wrote:
>>>>>> You would need to write an equivalent of conf_load_file()
>>>>>> that
>>>>>> created a new transaction id and read in all the files before
>>>>>> committing them to do it this way.
>>>
>>> How about the following as an alternative...
>>>
>>> It changes none of the past behaviour, but if you wanted to add an
>>> optional directory structure to a config file then simply add this
>>> to
>>> the default single config file that we ship.
>>>
>>> /etc/nfsmount.conf:
>>> [NFSMount_Global_Options]
>>> include=-/etc/nfsmount.conf.d/*.conf
>> If it was this simple I would go for it... 
>> but it just not work... as expected. Here is why.
>>
>> In relative_path() looks at the new file 
>> (/etc/nfsmount.conf.d/*.conf). If the path starts
>> with '/',  the path is strdup-ed and returned.
>>
>> The '*' is never expanded. Even if it was... there
>> no code (that I see) that will process multiple
>> files.   TBL... This works 
>> include=-/etc/nfsmount.conf.d/nfsmount.conf
>>
>> This doesn't 
>> include=-/etc/nfsmount.conf.d/*.conf
>>
>> Also I don't know if it is a good idea to have the sub configs
>> dependent on the main config file. If the main config is remove
>> the sub config will never be seen. Is that a good thing?
>>
>> I'm thinking we go with processing the sub configs separate 
>> from the main config and allow multiple sub configs be processed 
>> if they end with ".config" (mrchuck's suggestion).
>>
>> Then document all this in the man pages.  
>>
>> The last question should the main config be process,
>> not at all or after or before the sub configs?
>>
>> steved.
>>   
>>>
>>> The leading minus tells it that it isnt an error if its empty, and
>>> it
>>> will load all of the fragments it finds in there as well as the
>>> existing single file.  Apply the same structure to any existing
>>> config
>>> file that you want to also have a directory for.
>>>
>>> -Alice
>>>
>>>
>>>
>>> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
>>> index 58c03911..aade50c8 100644
>>> --- a/support/nfs/conffile.c
>>> +++ b/support/nfs/conffile.c
>>> @@ -53,6 +53,7 @@
>>>  #include <libgen.h>
>>>  #include <sys/file.h>
>>>  #include <time.h>
>>> +#include <glob.h>
>>>  
>>>  #include "conffile.h"
>>>  #include "xlog.h"
>>> @@ -690,6 +691,7 @@ conf_parse_line(int trans, char *line, const
>>> char *filename, int lineno, char **
>>>  	if (strcasecmp(line, "include")==0) {
>>>  		/* load and parse subordinate config files */
>>>  		_Bool optional = false;
>>> +		glob_t globdata;
>>>  
>>>  		if (val && *val == '-') {
>>>  			optional = true;
>>> @@ -704,33 +706,46 @@ conf_parse_line(int trans, char *line, const
>>> char *filename, int lineno, char **
>>>  			return;
>>>  		}
>>>  
>>> -		subconf = conf_readfile(relpath);
>>> -		if (subconf == NULL) {
>>> -			if (!optional)
>>> -				xlog_warn("config error at %s:%d: error
>>> loading included config",
>>> -					  filename, lineno);
>>> -			if (relpath)
>>> -				free(relpath);
>>> -			return;
>>> -		}
>>> +		if (glob(relpath, GLOB_MARK|GLOB_NOCHECK, NULL,
>>> &globdata)) {
>>> +			xlog_warn("config error at %s:%d: error with
>>> matching pattern", filename, lineno);
>>> +		} else 
>>> +		{
>>> +			for (size_t g=0; g<globdata.gl_pathc; g++) {
>>> +				const char * subpath =
>>> globdata.gl_pathv[g];
>>>  
>>> -		/* copy the section data so the included file can
>>> inherit it
>>> -		 * without accidentally changing it for us */
>>> -		if (*section != NULL) {
>>> -			inc_section = strdup(*section);
>>> -			if (*subsection != NULL)
>>> -				inc_subsection = strdup(*subsection);
>>> -		}
>>> +				if (subpath[strlen(subpath)-1] == '/')
>>> {
>>> +					continue;
>>> +				}
>>> +				subconf = conf_readfile(subpath);
>>> +				if (subconf == NULL) {
>>> +					if (!optional)
>>> +						xlog_warn("config error
>>> at %s:%d: error loading included config",
>>> +							  filename,
>>> lineno);
>>> +					if (relpath)
>>> +						free(relpath);
>>> +					return;
>>> +				}
>>> +
>>> +				/* copy the section data so the
>>> included file can inherit it
>>> +				 * without accidentally changing it for
>>> us */
>>> +				if (*section != NULL) {
>>> +					inc_section = strdup(*section);
>>> +					if (*subsection != NULL)
>>> +						inc_subsection =
>>> strdup(*subsection);
>>> +				}
>>>  
>>> -		conf_parse(trans, subconf, &inc_section,
>>> &inc_subsection, relpath);
>>> +				conf_parse(trans, subconf,
>>> &inc_section, &inc_subsection, relpath);
>>>  
>>> -		if (inc_section)
>>> -			free(inc_section);
>>> -		if (inc_subsection)
>>> -			free(inc_subsection);
>>> +				if (inc_section)
>>> +					free(inc_section);
>>> +				if (inc_subsection)
>>> +					free(inc_subsection);
>>> +				free(subconf);
>>> +			}
>>> +		}
>>>  		if (relpath)
>>>  			free(relpath);
>>> -		free(subconf);
>>> +		globfree(&globdata);
>>>  	} else {
>>>  		/* XXX Perhaps should we not ignore errors?  */
>>>  		conf_set(trans, *section, *subsection, line, val, 0,
>>> 0);
>>>
>>>
>