Re: [PATCH] virconf: Fix config file path construction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 27/07/16 11:21, Andrea Bolognani wrote:
> On Wed, 2016-07-27 at 08:50 +0200, Erik Skultety wrote:
>> Since commit c4bdff19, the path to the configuration file has been constructed
>> in the following manner:
>>   - if no config filename was passed to virConfLoadConfigPath, libvirt.conf was
>>   used as default
>>   - otherwise the filename was concatenated with
>>   "<config_dir>/libvirt/libvirt%s%s.conf" which in admin case resulted in
>>   "libvirt-libvirt-admin.conf.conf". Obviously, this non-existent config led to
>>   ignoring  all user settings in libvirt-admin.conf. This patch requires the
>>   config filename to be always provided as an argument with the concatenation
>>   being simplified.
>>  
>>   Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1357364
>>  
>> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
>> ---
>>   src/libvirt.c      |  2 +-
>>   src/util/virconf.c | 12 ++++--------
>>   2 files changed, 5 insertions(+), 9 deletions(-)
>>  
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index 68c8317..52462e3 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -969,7 +969,7 @@ virConnectOpenInternal(const char *name,
>>       if (ret == NULL)
>>           return NULL;
>>   
>> -    if (virConfLoadConfig(&conf, NULL) < 0)
>> +    if (virConfLoadConfig(&conf, "libvirt.conf") < 0)
>>           goto failed;
>>   
>>       if (name && name[0] == '\0')
>> diff --git a/src/util/virconf.c b/src/util/virconf.c
>> index ee54072..3e49f41 100644
>> --- a/src/util/virconf.c
>> +++ b/src/util/virconf.c
>> @@ -1566,20 +1566,16 @@ virConfLoadConfigPath(const char *name)
>>   {
>>       char *path;
>>       if (geteuid() == 0) {
>> -        if (virAsprintf(&path, "%s/libvirt/libvirt%s%s.conf",
>> -                        SYSCONFDIR,
>> -                        name ? "-" : "",
>> -                        name ? name : "") < 0)
>> +        if (virAsprintf(&path, "%s/libvirt/%s",
>> +                        SYSCONFDIR, name) < 0)
>>               return NULL;
>>       } else {
>>           char *userdir = virGetUserConfigDirectory();
>>           if (!userdir)
>>               return NULL;
>>   
>> -        if (virAsprintf(&path, "%s/libvirt%s%s.conf",
>> -                        userdir,
>> -                        name ? "-" : "",
>> -                        name ? name : "") < 0) {
>> +        if (virAsprintf(&path, "%s/%s",
>> +                        userdir, name) < 0) {
>>               VIR_FREE(userdir);
>>               return NULL;
>>           }
> 
> You could also have changed src/libvirt-admin.c to call
> 
>     virConfLoadConfig(&conf, "admin")
> 
> instead, but your solution is clearer.
> 

Yeah, I know I could and I also thought about doing that, but I didn't
like it in the end, because imho (and you mentioned that too) being
explicit about the config file in this case is somewhat clearer. Also,
I'm not sure and I might be talking rubbish but I think I tried to use
the approach you mentioned above in the earlier versions of the patch
that modified this logic and was discouraged to do it that way in
reviews (but I would have to check the history :P to really confirm
that...). Anyway, thanks for review, I'll push that in a moment.

Erik

> ACK
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]