[libvirt] [PATCH] Use XDG Base Directories instead of storing in home directory

[Daniel P. Berrange]

On Wed, May 02, 2012 at 01:53:36PM -0400, William Jon McCann wrote:

> +static int migrateProfile(void)
> +{
> +    char *old_base = NULL;
> +    char *updated = NULL;
> +    char *home = NULL;
> +    char *xdg_dir = NULL;
> +    char *config_dir = NULL;
> +    const char *config_home;
> +    int ret = -1;
> +    mode_t old_umask;
> +
> +    if (!(home = virGetUserDirectory(geteuid())))
> +        goto cleanup;
> +
> +    if (virAsprintf(&old_base, "%s/.libvirt", home) < 0) {
> +        goto cleanup;
> +    }
> +
> +    /* if the new directory is there or the old one is not: do nothing */
> +    if (!(config_dir = virGetUserConfigDirectory(geteuid())))
> +        goto cleanup;
> +
> +    if (!virFileIsDir(old_base) || virFileExists(config_dir)) {

This is missing an assignment 'ret = 0', since this is a success
(no-op) case, rather than a failure case.

> +        goto cleanup;
> +    }
> +static char *virGetXDGDirectory(uid_t uid, const char *xdgenvname, const char *xdgdefdir)
> +{
> +    char *path = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (uid == getuid ())
> +        path = getenv(xdgenvname);
> +
> +    if (path && path[0]) {
> +        virBufferAsprintf(&buf, "%s/libvirt/", path);
> +    } else {
> +        char *home;
> +        home = virGetUserEnt(uid, VIR_USER_ENT_DIRECTORY);
> +        virBufferAsprintf(&buf, "%s/%s/libvirt/", home, xdgdefdir);
> +        VIR_FREE(home);
> +    }
> +    VIR_FREE(path);

Opps, getenv() returns a string that should not be freed.
Also we can simplify this by just using virAsprintf()
since there's only one printf required here

> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    return virBufferContentAndReset(&buf);
> +}
> +
> +char *virGetUserConfigDirectory(uid_t uid)
> +{
> +    return virGetXDGDirectory(uid, "XDG_CONFIG_HOME", ".config");
> +}
> +
> +char *virGetUserCacheDirectory(uid_t uid)
> +{
> +     return virGetXDGDirectory(uid, "XDG_CACHE_HOME", ".cache");
> +}
> +


Basically I applied the following diff ontop of your patch and it then
worked as expected for me

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index bb25678..67248b3 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -770,6 +770,7 @@ static int migrateProfile(void)
         goto cleanup;
 
     if (!virFileIsDir(old_base) || virFileExists(config_dir)) {
+        ret = 0;
         goto cleanup;
     }
 
diff --git a/src/util/util.c b/src/util/util.c
index 447b7b7..b47e3d8 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2312,29 +2312,27 @@ char *virGetUserDirectory(uid_t uid)
 
 static char *virGetXDGDirectory(uid_t uid, const char *xdgenvname, const char *xdgdefdir)
 {
-    char *path = NULL;
-    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    const char *path = NULL;
+    char *ret = NULL;
+    char *home = virGetUserEnt(uid, VIR_USER_ENT_DIRECTORY);
 
-    if (uid == getuid ())
+    if (uid == getuid())
         path = getenv(xdgenvname);
 
     if (path && path[0]) {
-        virBufferAsprintf(&buf, "%s/libvirt/", path);
+        if (virAsprintf(&ret, "%s/libvirt/", path) < 0)
+            goto no_memory;
     } else {
-        char *home;
-        home = virGetUserEnt(uid, VIR_USER_ENT_DIRECTORY);
-        virBufferAsprintf(&buf, "%s/%s/libvirt/", home, xdgdefdir);
-        VIR_FREE(home);
-    }
-    VIR_FREE(path);
-
-    if (virBufferError(&buf)) {
-        virBufferFreeAndReset(&buf);
-        virReportOOMError();
-        return NULL;
+        if (virAsprintf(&ret, "%s/%s/libvirt/", home, xdgdefdir) < 0)
+            goto no_memory;
     }
 
-    return virBufferContentAndReset(&buf);
+cleanup:
+    VIR_FREE(home);
+    return ret;
+no_memory:
+    virReportOOMError();
+    goto cleanup;
 }
 
 char *virGetUserConfigDirectory(uid_t uid)
@@ -2349,8 +2347,7 @@ char *virGetUserCacheDirectory(uid_t uid)
 
 char *virGetUserRuntimeDirectory(uid_t uid)
 {
-    char *path = NULL;
-    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    const char *path = NULL;
 
     if (uid == getuid ())
         path = getenv("XDG_RUNTIME_DIR");
@@ -2358,16 +2355,12 @@ char *virGetUserRuntimeDirectory(uid_t uid)
     if (!path || !path[0]) {
         return virGetUserCacheDirectory(uid);
     } else {
-        virBufferAsprintf(&buf, "%s/libvirt/", path);
-        VIR_FREE(path);
-
-        if (virBufferError(&buf)) {
-            virBufferFreeAndReset(&buf);
+        char *ret;
+        if (virAsprintf(&ret, "%s/libvirt/", path) < 0) {
             virReportOOMError();
             return NULL;
         }
-
-        return virBufferContentAndReset(&buf);
+        return ret;
     }
 }
 




I would ACK this patch with those changes applied. Having said that I
think I would prefer to wait until after the 0.9.12 release is cut
before applying this, since I'm afraid there may be some unexpected
consequences. Waiting till after 0.9.13 gives us a month to test it
in GIT master before it gets into a release.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|