Re: [PATCH] Allow root users to have their own configuration file

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On 09/12/2013 11:53 AM, Martin Kletzander wrote:
> Currently, we have two configuration file paths, one global (where
> "global" means root-only and we're probably not changing this in near
> future) and one per-user.  Unfortunately root user cannot use the
> second option because until now we were choosing the file path
> depending only on whether the user is root or not.
> 
> This patch modifies the mentioned behavior for root only, allowing him
> to set his own configuration files without changing anything in
> system-wide configuration folders.
> 
> This also makes the virsh-uriprecedence test pass its first test case
> when ran as root.
> 
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> ---
> 
> Notes:
>     I'm playing along previously mentioned "proper behavior" in this
>     patch.  However, IMNSHO, our "global" or "system-wide" configuration
>     file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible
>     for all users since this has no security impact (security information
>     may be in files 'libvirtd.conf' or 'qemu.conf').  This file should be
>     also read and used for all users.  After that, settings in user
>     configuration file (defaulting to '~/.config/libvirt/libvirt.conf')
>     may override some of these settings for that user.
>     
>     This is how all sensible configurations are loaded and that's also
>     what I'd prefer.  Unfortunately some developers feels this should be
>     done in completely different way.
> 
>  src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
> 

If someone doesn't like the 'bool global' parameter, I'm fine with
splitting the functions.  If mentioned with an ACK, I'll squash this in:

diff --git a/src/libvirt.c b/src/libvirt.c
index bfc466b..927868f 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -961,25 +961,31 @@ error:
  * Return code 0 means no error, but doesn't guarantee path != NULL.
  */
 static int
-virConnectGetConfigFilePath(char **path, bool global)
+virConnectGetGlobalConfigFilePath(char **path)
 {
-    char *userdir = NULL;
-    int ret = -1;
     *path = NULL;

     /* Don't provide the global configuration file to non-root users */
-    if (geteuid() != 0 && global)
+    if (geteuid() != 0)
         return 0;

-    if (global) {
-        if (virAsprintf(path, "%s/libvirt/libvirt.conf",
-                        SYSCONFDIR) < 0)
-            goto cleanup;
-    } else {
-        if (!(userdir = virGetUserConfigDirectory()) ||
-            virAsprintf(path, "%s/libvirt.conf", userdir) < 0)
-            goto cleanup;
-    }
+    if (virAsprintf(path, "%s/libvirt/libvirt.conf",
+                    SYSCONFDIR) < 0)
+        return -1;
+
+    return 0;
+}
+
+static int
+virConnectGetUserConfigFilePath(char **path)
+{
+    char *userdir = NULL;
+    int ret = -1;
+    *path = NULL;
+
+    if (!(userdir = virGetUserConfigDirectory()) ||
+        virAsprintf(path, "%s/libvirt.conf", userdir) < 0)
+        goto cleanup;

     ret = 0;
  cleanup:
@@ -996,14 +1002,14 @@ virConnectGetConfigFile(virConfPtr *conf)
     *conf = NULL;

     /* Try reading user configuration file unconditionally */
-    if (virConnectGetConfigFilePath(&filename, false) < 0)
+    if (virConnectGetUserConfigFilePath(&filename) < 0)
         goto cleanup;

     if (!virFileExists(filename)) {
         /* and in case there is none, try the global one. */

         VIR_FREE(filename);
-        if (virConnectGetConfigFilePath(&filename, true) < 0)
+        if (virConnectGetGlobalConfigFilePath(&filename) < 0)
             goto cleanup;

         if (!filename ||
--
Martin

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