Re: [Qemu-devel] [PATCH v3 2/2] qemu-bridge-helper: Use stateless configuration for bridge.conf

[Ikey Doherty <michael.i.doherty@intel.com>]

On 26/05/15 17:38, Eduardo Habkost wrote:
> On Tue, May 26, 2015 at 01:54:07PM +0100, Ikey Doherty wrote:
>> The goal of stateless, and thus this change, is to separate OS configuration
>> from system administrator configuration. With this change we will read the
>> default configuration data from /usr/share/defaults/qemu, in the absence of
>> an overriding site administrator configuration in /etc/qemu.
>>
>> A key advantage of this change is enabling a sane and immutable default OS
>> configuration, that is resiliant to upgrades. Ultimate power is still left
>> to the system administrator, with the ability to override the defaults if
>> required. Lastly, given that the sane defaults are always available, the
>> administrator may simply remove their site-config files to reset the
>> configuration to the "factory defaults" (i.e. OS configuration).
>>
>> Signed-off-by: Ikey Doherty <michael.i.doherty@intel.com>
>> ---
>>   configure            |  2 ++
>>   qemu-bridge-helper.c | 15 +++++++++------
>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>
> [...]
>> -#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
>> +#define DEFAULT_ACL_FILE CONFIG_QEMU_DEFAULTDIR "/bridge.conf"
>> +#define SITE_ACL_FILE    CONFIG_QEMU_CONFDIR    "/bridge.conf"
>>
>>   enum {
>>       ACL_ALLOW = 0,
>> @@ -272,11 +273,13 @@ int main(int argc, char **argv)
>>
>>       /* parse default acl file */
>>       QSIMPLEQ_INIT(&acl_list);
>> -    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
>> -        fprintf(stderr, "failed to parse default acl file `%s'\n",
>> -                DEFAULT_ACL_FILE);
>> -        ret = EXIT_FAILURE;
>> -        goto cleanup;
>> +    if (parse_acl_file(SITE_ACL_FILE, &acl_list) == -1) {
>> +        if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
>> +            fprintf(stderr, "failed to parse default acl file `%s'\n",
>> +                    DEFAULT_ACL_FILE);
>> +            ret = EXIT_FAILURE;
>> +            goto cleanup;
>> +        }
>>       }
>
> This will make syntax errors on SITE_ACL_FILE cause partial loading of
> the rules on SITE_ACL_FILE, and trigger loading of DEFAULT_ACL_FILE,
> instead of aborting bridge-helper.
>
> Wouldn't it be better to fallback to DEFAULT_ACL_FILE if and only if
> SITE_ACL_FILE is missing?
>

I could stat the file and fallback to default, and only parse_acl_file
the one that exists. Better?

-- 
Clear Linux Project for Intel Architecture
Intel Open Source Technology Center
http://www.clearlinux.org