| Message ID | 20201029210401.446244-1-steved@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Enable config.d directory to be processed. | expand |
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
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.
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, §ion, &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);
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, §ion, &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); >
> 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, §ion, &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
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);
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, §ion, &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 > > >
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.
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); > >
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
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); > > > >
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); >>> >>> >