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 :|