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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list