Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [RFC PATCH 0/1] Enable config.d directory to be processed.
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <a42154ffeb06a21590db01ab651870040597571c.camel@redhat.com>
Date:   Mon, 2 Nov 2020 10:16:53 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: 7bit
Message-Id: <820312BD-099F-4B53-81A3-12E6F4066A5C@oracle.com>
References: <20201029210401.446244-1-steved@redhat.com>
 <338aeb795a31c2233016d225dc114e33d02eb0cb.camel@redhat.com>
 <6f3caf91-296c-0aa8-ba41-bc35d500adaa@RedHat.com>
 <4836616f-3aa6-d0bd-22db-cd7fecf4dce9@RedHat.com>
 <a42154ffeb06a21590db01ab651870040597571c.camel@redhat.com>
To:     Alice Mitchell <ajmitchell@redhat.com>,
        Steve Dickson <SteveD@redhat.com>
Precedence: bulk


> 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