[libvirt] [PATCH] Use XDG Base Directories instead of storing in home
Daniel P. Berrange
berrange at redhat.com
Tue May 1 08:25:19 UTC 2012
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 at redhat.com --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 :|
More information about the libvir-list
mailing list