On Mon, Apr 30, 2012 at 02:55:06PM -0400, William Jon McCann wrote: > Hi, > > New to the list so hopefully I'm following the correct protocol. Almost - we tend to like the commit messages for patches to be fairly verbose about the change, so your description of advantages here is actually better being put in the commit message itself. The patch also came through as an attachment with application/octet-stream, text/plain is preferrred, or even better just inline text. The simplest way to achieve this would have been to use git send-email eg git send-email --to libvir-list@xxxxxxxxxx --no-chain-reply-to -1 60da72d7d502bfbb4cbde97898b98811a2221d4f Anyway, on with the real patch review.... > Attached is a patch (mostly untested at the moment) to change the > default storage location from .libvirt to the locations defined in the > XDG Base Directory Specification > (http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html). > > This has a number of advantages for us: > > It allows sharing a home directory between different machines, or > sessions (eg. using NFS) > Cleanly separates cache, runtime (eg. sockets), or app data from > user settings Yes, this is something we do a particularly bad job of with the current layout of $HOME/.libvirt. I have never liked the way the logs & lib directories are actually sub-dirs of the place where we store the XML configs. A shame this spec wasn't around back when I wrote this code originally. > Supports performing smart or selective migration of settings > between different OS versions > Supports reseting settings without breaking things > Makes it possible to clear cache data to make room when the disk > is filling up > Allows us to write a robust and efficient backup solution > Allows an admin flexibility to change where data and settings are stored > It dramatically reduces the complexity and incoherence of the > system for administrators > > This is a pretty important set of features for both enterprise and > individual use cases. Yep, I think the advantages of using XDG directories is pretty compelling. The only point in favour of using $HOME/.libvirt is that fact that it is what we have always done. I think we can deal with that by auto-matically migrating the content to the new location, and leaving in a symlink or two from the old location to the new. > diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c > index b098f6a..f98ba6f 100644 > --- a/daemon/libvirtd.c > +++ b/daemon/libvirtd.c > @@ -722,6 +731,59 @@ static int daemonStateInit(virNetServerPtr srv) > return 0; > } > > +static void migrateProfile(void) > +{ > + char *old_base = NULL; > + char *updated = NULL; > + char *home = NULL; > + char *xdg_dir = NULL; > + char *config_dir = NULL; > + > + home = virGetUserDirectory(geteuid()); Needs a check for home != NULL. > + if (virAsprintf(&old_base, "%s/.libvirt", home) < 0) { > + goto error; > + } > + > + /* if the new directory is there or the old one is not: do nothing */ > + config_dir = virGetUserConfigDirectory(geteuid()); Check config_dir != NULL > + if (!virFileIsDir(old_base) || virFileExists(config_dir)) { > + goto error; > + } > + > + /* test if we already attempted to migrate first */ > + if (virAsprintf(&updated, "%s/DEPRECATED-DIRECTORY", old_base) < 0) { > + goto error; > + } > + if (virFileExists(updated)) { > + goto error; > + } I'm not sure that this is actually needed - shouldn't the test for existance of 'config_dir' catch this. In any case, instead of using such a file, I think we should just add a symlink from $HOME/.libvirt to $HOME/.config/libvirt. So you could replace this with a check to see if old_base is a symlink > + > + xdg_dir = strdup(getenv("XDG_CONFIG_HOME")); > + if (!xdg_dir || xdg_dir[0] == '\0') { > + if (virAsprintf(&xdg_dir, "%s/.config", home) < 0) { > + goto error; > + } > + } > + > + if (virFileMakePathWithParents(xdg_dir, 0700) != 0) { > + goto error; > + } > + > + if (rename (old_base, config_dir) != 0) { We prefer to avoid ' ' between the function name & opening '(' and use '< 0' rather than '!= 0' for syscall checks. > + int fd = creat(updated, 0600); > + close(fd); In light of above recommendation, I think you should replace this with symlink(old_base, config_dir). NB, you'd need to use 'VIR_FORCE_CLOSE(fd)' here - if you run 'make syntax-check' it should tell you about this coding style mistake (and any others) > + goto error; > + } > + > + error: When the label is for a shared success/failure path our standard is to use 'cleanup'. Only use 'error' if it is a dedicated/exclusive failure path > + VIR_FREE(home); > + VIR_FREE(old_base); > + VIR_FREE(xdg_dir); > + VIR_FREE(config_dir); > + VIR_FREE(updated); We should also propagate a success/failure code back to the caller and exit as needed. > + > +} > + > /* Print command-line usage. */ > diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h > index 1504eec..aca9412 100644 > --- a/src/remote/remote_driver.h > +++ b/src/remote/remote_driver.h > @@ -37,7 +37,7 @@ unsigned long remoteVersion(void); > # define LIBVIRTD_TCP_PORT "16509" > # define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock" > # define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro" > -# define LIBVIRTD_USER_UNIX_SOCKET "/.libvirt/libvirt-sock" > +# define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock" > # define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirtd.conf" Reminds me that since we moving the paths anyway, we should take this opportunity to change from the socket from the abstract namespace, into the real filesystem namespace. This would allow us to remotely connect to the session sockets over ssh and solve our OS-X portability problem. No need for you todo this, I'll write a separate patch. > @@ -1271,6 +1277,67 @@ static int virFileMakePathHelper(char *path) > } > > /** > + * Creates the given directory with mode if it's not already existing. > + * > + * Returns 0 on success, or -1 if an error occurred (in which case, errno > + * is set appropriately). > + */ > +int virFileMakePathWithParents (const char *pathname, > + int mode) > +{ Hmm, our virFileMakePath already takes care of creating parents as needed. The only difference is we don't have a 'mode' parameter for the existing function. Rather than introduce this new function, I'd rather you just used the existing one. If you really need the 'mode' parameter, then we should update the existing one to have that (ought to be done as a separate patch to this though if required) > +char *virGetUserConfigDirectory(uid_t uid) > +{ > + char *path = NULL; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > + if (uid == getuid ()) > + path = getenv("XDG_CONFIG_HOME"); > + > + if (path && path[0]) { > + virBufferAsprintf(&buf, "%s/libvirt/", path); > + } else { > + char *home; > + home = virGetUserEnt(uid, VIR_USER_ENT_DIRECTORY); > + virBufferAsprintf(&buf, "%s/.config/libvirt/", home); > + VIR_FREE(home); > + } > + VIR_FREE(path); > + > + if (virBufferError(&buf)) { > + virBufferFreeAndReset(&buf); > + virReportOOMError(); > + return NULL; > + } > + > + return virBufferContentAndReset(&buf); > +} > + > +char *virGetUserCacheDirectory(uid_t uid) > +{ > + char *path = NULL; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > + if (uid == getuid ()) > + path = getenv("XDG_CACHE_HOME"); > + > + if (path && path[0]) { > + virBufferAsprintf(&buf, "%s/libvirt/", path); > + } else { > + char *home; > + home = virGetUserEnt(uid, VIR_USER_ENT_DIRECTORY); > + virBufferAsprintf(&buf, "%s/.cache/libvirt/", home); > + VIR_FREE(home); > + } > + VIR_FREE(path); > + > + if (virBufferError(&buf)) { > + virBufferFreeAndReset(&buf); > + virReportOOMError(); > + return NULL; > + } > + > + return virBufferContentAndReset(&buf); > +} These two functions could be made to share a common impl eg static char *virGetXDGDirectory(uid_t uid, const gchar *xdgenvname, const gchar *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/", xdgdefdir, home); VIR_FREE(home); } VIR_FREE(path); 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"); } > + > +char *virGetUserRuntimeDirectory(uid_t uid) > +{ > + char *path = NULL; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > + if (uid == getuid ()) > + path = getenv("XDG_RUNTIME_DIR"); > + > + if (!path || !path[0]) { > + return virGetUserCacheDirectory(uid); > + } else { > + virBufferAsprintf(&buf, "%s/libvirt/", path); > + VIR_FREE(path); > + > + if (virBufferError(&buf)) { > + virBufferFreeAndReset(&buf); > + virReportOOMError(); > + return NULL; > + } > + > + return virBufferContentAndReset(&buf); > + } > +} Unfortunately this one is slightly different so can't share Basically I think this patch's code is pretty sound and a good change overall. Look forward to an updated version. Don't forget to run 'make syntax-check' on it to validate coding style. Regards, 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