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