[libvirt] [PATCH v2 0/4] Keep original file label
This is a rework of v1 with which it doesn't have a single line in common :) This version uses XATTR heavily. The first two patches utilize extended attributes to store the original owner and the reference counter, while the next two are just a proof of concept. They suffer with a serious defect - if there's already an ACL entry for user, we completely overwrite it and dispose. Anyway - I am solving atomicity problem for now. I need to understand virlockd better, so if somebody has a bright idea how to solve locking or somebody is willing to implement it himself, feel free. There are several questions I think we should agree on: - namespace of XATTR labels: There are currently 4 namespaces: security, system, trusted and user. I think trusted suits our needs the best. See man attr(5) for their description. - Is it enough if we keep the original label only on those filesystems supporting XATTR? If so, should we turn libattr into hard dependency? - Keeping the whole logic within XATTR drops need for security driver state XML. But what about those filesystems which does not support XATTR by design? (I don't want to point my finger) - The atomicity problem. I guess we want virlockd however we don't want to enforce users to configure anything in order to use this functionality. But I don't think this can be achieved, if locks need to be shared among other hosts, right? Michal Privoznik (4): virFile: Add APIs for extended attributes handling security_dac: Remember owner prior chown() and restore on relabel virfile: Introduce internal API for managing ACL security_dac: Favour ACLs over chown() configure.ac | 2 + libvirt.spec.in | 6 + src/libvirt_private.syms | 5 + src/security/security_dac.c | 209 +++++++++++++++++++++++++----- src/util/virfile.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 19 +++ 6 files changed, 515 insertions(+), 34 deletions(-) -- 1.8.1.5
Currently, only three wrappers are being implemented: virFileSetAttr for setting attributes virFileGetAttr for querying attributes (note we need to call it twice, first time to get length of attribute value, second to get actual value) virFileRemoveAttr for removing attributes --- configure.ac | 1 + libvirt.spec.in | 3 ++ src/libvirt_private.syms | 3 ++ src/util/virfile.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 12 ++++++ 5 files changed, 125 insertions(+) diff --git a/configure.ac b/configure.ac index 9d366e9..cbb20c4 100644 --- a/configure.ac +++ b/configure.ac @@ -275,6 +275,7 @@ dnl header could be found. AM_CONDITIONAL([HAVE_LIBTASN1], [test "x$ac_cv_header_libtasn1_h" = "xyes"]) AC_CHECK_LIB([intl],[gettext],[]) +AC_CHECK_LIB([attr],[getxattr]) dnl Do we have rpcgen? AC_PATH_PROG([RPCGEN], [rpcgen], [no]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 9fb753a..90629c8 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -577,6 +577,9 @@ BuildRequires: gawk # For storage wiping with different algorithms BuildRequires: scrub +# For xattr +BuildRequires: libattr-devel + %if %{with_numad} BuildRequires: numad %endif diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ed46479..fc7568e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1273,8 +1273,11 @@ virFileClose; virFileDirectFdFlag; virFileFclose; virFileFdopen; +virFileGetAttr; virFileLoopDeviceAssociate; +virFileRemoveAttr; virFileRewrite; +virFileSetAttr; virFileTouch; virFileUpdatePerm; virFileWrapperFdClose; diff --git a/src/util/virfile.c b/src/util/virfile.c index eec9bcc..aa4579e 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -37,6 +37,11 @@ # include <sys/ioctl.h> #endif +#ifdef HAVE_LIBATTR +# include <attr/xattr.h> +# include <sys/types.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -644,3 +649,104 @@ int virFileLoopDeviceAssociate(const char *file, } #endif /* __linux__ */ + +#ifdef HAVE_LIBATTR /* HAVE_LIBATTR */ +int +virFileSetAttr(const char *file, + const char *name, + const char *value) +{ + size_t valueSize = strlen(value); + if (setxattr(file, name, value, valueSize, 0) < 0) { + virReportSystemError(errno, + _("Unable to set extended attribute '%s' on '%s'"), + name, file); + return -1; + } + return 0; +} + +int +virFileGetAttr(const char *file, + const char *name, + char **value) +{ + int ret = -1; + char *buf = NULL; + ssize_t valueSize; + + /* get attribute length */ + if ((valueSize = getxattr(file, name, NULL, 0)) < 0) { + /* The Linux kernel does not define ENOATTR, but maps it to ENODATA. */ + if (errno == ENOATTR || errno == ENODATA) { + *value = NULL; + return 0; + } else { + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + return ret; + } + } + + if (VIR_ALLOC_N(buf, valueSize) < 0) { + virReportOOMError(); + return ret; + } + + if ((ret = getxattr(file, name, buf, valueSize)) < 0) { + VIR_FREE(buf); + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + } else { + *value = buf; + } + + return ret; +} + +int +virFileRemoveAttr(const char *file, + const char *name) +{ + if (removexattr(file, name) < 0) { + virReportSystemError(errno, + _("Unable to remove extended attribute '%s' on '%s'"), + name, file); + return -1; + } + return 0; +} + +#else /* HAVE_LIBATTR */ + +int +virFileSetAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + const char *value ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set extended attributes")); + return -1; +} + +int +virFileGetAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get extended attributes")); + return -1; +} + +int +virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to remove extended attributes")); + return -1; +} +#endif /* HAVE_LIBATTR */ diff --git a/src/util/virfile.h b/src/util/virfile.h index c885b73..3b4d672 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -108,4 +108,16 @@ int virFileUpdatePerm(const char *path, int virFileLoopDeviceAssociate(const char *file, char **dev); +int virFileSetAttr(const char *file, + const char *name, + const char *value) + ATTRIBUTE_NONNULL(3); + +int virFileGetAttr(const char *file, + const char *name, + char **value); + +int virFileRemoveAttr(const char *file, + const char *name); + #endif /* __VIR_FILES_H */ -- 1.8.1.5
On Wed, Mar 06, 2013 at 03:05:53PM +0100, Michal Privoznik wrote:
Currently, only three wrappers are being implemented: virFileSetAttr for setting attributes virFileGetAttr for querying attributes (note we need to call it twice, first time to get length of attribute value, second to get actual value) virFileRemoveAttr for removing attributes --- configure.ac | 1 + libvirt.spec.in | 3 ++ src/libvirt_private.syms | 3 ++ src/util/virfile.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 12 ++++++ 5 files changed, 125 insertions(+)
diff --git a/configure.ac b/configure.ac index 9d366e9..cbb20c4 100644 --- a/configure.ac +++ b/configure.ac @@ -275,6 +275,7 @@ dnl header could be found. AM_CONDITIONAL([HAVE_LIBTASN1], [test "x$ac_cv_header_libtasn1_h" = "xyes"])
AC_CHECK_LIB([intl],[gettext],[]) +AC_CHECK_LIB([attr],[getxattr])
We already have a check for libattr - see m4/virt-attr.m4
dnl Do we have rpcgen? AC_PATH_PROG([RPCGEN], [rpcgen], [no]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 9fb753a..90629c8 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -577,6 +577,9 @@ BuildRequires: gawk # For storage wiping with different algorithms BuildRequires: scrub
+# For xattr +BuildRequires: libattr-devel
Again we already have a BuildRequires on libattr-devel
diff --git a/src/util/virfile.c b/src/util/virfile.c index eec9bcc..aa4579e 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -37,6 +37,11 @@ # include <sys/ioctl.h> #endif
+#ifdef HAVE_LIBATTR +# include <attr/xattr.h> +# include <sys/types.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -644,3 +649,104 @@ int virFileLoopDeviceAssociate(const char *file, }
#endif /* __linux__ */ + +#ifdef HAVE_LIBATTR /* HAVE_LIBATTR */
Pointless comment at end of line
+int +virFileSetAttr(const char *file, + const char *name, + const char *value) +{ + size_t valueSize = strlen(value); + if (setxattr(file, name, value, valueSize, 0) < 0) { + virReportSystemError(errno, + _("Unable to set extended attribute '%s' on '%s'"), + name, file); + return -1; + } + return 0; +} + +int +virFileGetAttr(const char *file, + const char *name, + char **value) +{ + int ret = -1; + char *buf = NULL; + ssize_t valueSize; + + /* get attribute length */ + if ((valueSize = getxattr(file, name, NULL, 0)) < 0) { + /* The Linux kernel does not define ENOATTR, but maps it to ENODATA. */ + if (errno == ENOATTR || errno == ENODATA) { + *value = NULL; + return 0; + } else { + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + return ret; + } + } + + if (VIR_ALLOC_N(buf, valueSize) < 0) { + virReportOOMError(); + return ret; + } + + if ((ret = getxattr(file, name, buf, valueSize)) < 0) { + VIR_FREE(buf); + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + } else { + *value = buf; + } + + return ret; +} + +int +virFileRemoveAttr(const char *file, + const char *name) +{ + if (removexattr(file, name) < 0) { + virReportSystemError(errno, + _("Unable to remove extended attribute '%s' on '%s'"), + name, file); + return -1; + } + return 0; +}
Looks good besides the issues mentioned 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 :|
Currently, on domain startup a labeling of domain resources (e.g. disk images, sockets, ...) is done. However, we don't store the original owner anywhere. So when domain is shutting down, we cannot restore the original owner of files we have chown()-ed. This patch resolves this issue for DAC driver. --- src/security/security_dac.c | 187 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 153 insertions(+), 34 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..76a1dc6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -25,6 +25,7 @@ #include "security_dac.h" #include "virerror.h" +#include "virfile.h" #include "virutil.h" #include "viralloc.h" #include "virlog.h" @@ -34,6 +35,8 @@ #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount" typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; @@ -234,6 +237,117 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; } +static int +virSecurityDACRememberLabel(const char *path) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldOwner = NULL; + unsigned int refCount = 0; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) { + /* Not all filesystems support XATTR */ + if (virStorageFileIsSharedFS(path)) + return 0; + else + return ret; + } + + if (!refCountStr) { + struct stat sb; + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat '%s'"), path); + return ret; + } + + if (virAsprintf(&oldOwner, "+%u:+%u", + (unsigned) sb.st_uid, (unsigned) sb.st_gid) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, oldOwner) < 0) + goto cleanup; + } else if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + + if (virAsprintf(&refCountStr, "%u", refCount+1) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(oldOwner); + VIR_FREE(refCountStr); + return 0; +} + +static int +virSecurityDACGetRememberedLabel(const char *path, + uid_t *user, + uid_t *group) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldOwner = NULL; + unsigned int refCount; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) + return ret; + + if (!refCountStr) { + /* Backward compatibility. If domain was started with older libvirt, + * it doesn't have any XATTR set so we should not fail here */ + return 0; + } + + if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + VIR_FREE(refCountStr); + + if (--refCount) { + if (virAsprintf(&refCountStr, "%u", refCount) < 0) { + virReportOOMError(); + return ret; + } + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + ret = refCount; + } else { + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || + !oldOwner) + return ret; + + if (parseIds(oldOwner, user, group) < 0) + goto cleanup; + + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); + } + + ret = refCount; + +cleanup: + VIR_FREE(oldOwner); + VIR_FREE(refCountStr); + return ret; +} + static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) @@ -265,11 +379,18 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU } static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACSetOwnership(const char *path, + uid_t uid, + gid_t gid, + bool remember) { VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", path, (long) uid, (long) gid); + if (remember && + virSecurityDACRememberLabel(path) < 0) + return -1; + if (chown(path, uid, gid) < 0) { struct stat sb; int chown_errno = errno; @@ -310,23 +431,35 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) { struct stat buf; int rc = -1; + int ret; char *newpath = NULL; + uid_t user = (uid_t) -1; + gid_t group = (gid_t) -1; VIR_INFO("Restoring DAC user and group on '%s'", path); if (virFileResolveLink(path, &newpath) < 0) { virReportSystemError(errno, _("cannot resolve symlink %s"), path); - goto err; + goto cleanup; } if (stat(newpath, &buf) != 0) - goto err; + goto cleanup; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + ret = virSecurityDACGetRememberedLabel(newpath, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping label restore on '%s' as it is still " + "in use (refCount=%d)", newpath, ret); + rc = 0; + goto cleanup; + } + + rc = virSecurityDACSetOwnership(newpath, user, group, false); -err: +cleanup: VIR_FREE(newpath); return rc; } @@ -348,7 +481,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, if (virSecurityDACGetImageIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(path, user, group, true); } @@ -384,24 +517,9 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (!priv->dynamicOwnership) - return 0; - - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) - return 0; - - /* Don't restore labels on readoly/shared disks, because - * other VMs may still be accessing these - * Alternatively we could iterate over all running - * domains and try to figure out if it is in use, but - * this would not work for clustered filesystems, since - * we can't see running VMs using the file on other nodes - * Safest bet is thus to skip the restore step. - */ - if (disk->readonly || disk->shared) - return 0; - - if (!disk->src) + if (!priv->dynamicOwnership || + disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || + !disk->src) return 0; /* If we have a shared FS & doing migrated, we must not @@ -448,7 +566,7 @@ virSecurityDACSetSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, if (virSecurityDACGetIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(file, user, group); + return virSecurityDACSetOwnership(file, user, group, true); } @@ -467,7 +585,7 @@ virSecurityDACSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, if (virSecurityDACGetIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(file, user, group); + return virSecurityDACSetOwnership(file, user, group, true); } @@ -630,7 +748,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(dev->data.file.path, user, group); + ret = virSecurityDACSetOwnership(dev->data.file.path, + user, group, true); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -640,12 +759,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, goto done; } if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACSetOwnership(in, user, group) < 0) || - (virSecurityDACSetOwnership(out, user, group) < 0)) { + if ((virSecurityDACSetOwnership(in, user, group, true) < 0) || + (virSecurityDACSetOwnership(out, user, group, true) < 0)) { goto done; } } else if (virSecurityDACSetOwnership(dev->data.file.path, - user, group) < 0) { + user, group, true) < 0) { goto done; } ret = 0; @@ -815,11 +934,11 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + virSecurityDACSetOwnership(def->os.kernel, user, group, true) < 0) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(def->os.initrd, user, group) < 0) + virSecurityDACSetOwnership(def->os.initrd, user, group, true) < 0) return -1; return 0; @@ -838,7 +957,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(savefile, user, group); + return virSecurityDACSetOwnership(savefile, user, group, true); } -- 1.8.1.5
On Wed, Mar 06, 2013 at 03:05:54PM +0100, Michal Privoznik wrote:
Currently, on domain startup a labeling of domain resources (e.g. disk images, sockets, ...) is done. However, we don't store the original owner anywhere. So when domain is shutting down, we cannot restore the original owner of files we have chown()-ed. This patch resolves this issue for DAC driver. --- src/security/security_dac.c | 187 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 153 insertions(+), 34 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..76a1dc6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -25,6 +25,7 @@
#include "security_dac.h" #include "virerror.h" +#include "virfile.h" #include "virutil.h" #include "viralloc.h" #include "virlog.h" @@ -34,6 +35,8 @@
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount"
typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; @@ -234,6 +237,117 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; }
+static int +virSecurityDACRememberLabel(const char *path) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldOwner = NULL; + unsigned int refCount = 0; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) { + /* Not all filesystems support XATTR */ + if (virStorageFileIsSharedFS(path)) + return 0; + else + return ret; + } + + if (!refCountStr) { + struct stat sb; + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat '%s'"), path); + return ret; + } + + if (virAsprintf(&oldOwner, "+%u:+%u", + (unsigned) sb.st_uid, (unsigned) sb.st_gid) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, oldOwner) < 0) + goto cleanup; + } else if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + + if (virAsprintf(&refCountStr, "%u", refCount+1) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(oldOwner); + VIR_FREE(refCountStr); + return 0; +} + +static int +virSecurityDACGetRememberedLabel(const char *path, + uid_t *user, + uid_t *group) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldOwner = NULL; + unsigned int refCount; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) + return ret; + + if (!refCountStr) { + /* Backward compatibility. If domain was started with older libvirt, + * it doesn't have any XATTR set so we should not fail here */ + return 0; + } + + if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + VIR_FREE(refCountStr); + + if (--refCount) { + if (virAsprintf(&refCountStr, "%u", refCount) < 0) { + virReportOOMError(); + return ret; + } + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + ret = refCount; + } else { + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || + !oldOwner) + return ret; + + if (parseIds(oldOwner, user, group) < 0) + goto cleanup; + + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); + } + + ret = refCount; + +cleanup: + VIR_FREE(oldOwner); + VIR_FREE(refCountStr); + return ret; +} +
static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) @@ -265,11 +379,18 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU }
static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACSetOwnership(const char *path, + uid_t uid, + gid_t gid, + bool remember)
I don't think you should be adding this 'remember' flag here, because while it seems fine in this patch, when you add ACL support this gets really dubious semantics. Just stop virSecurityDACRestoreSecurityFileLabel from calling this method entirely - trying to share doesn't make sense when you introduce ACLs to the equation since you need fundamentally differnt APIs in the set vs restore case the.
{ VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", path, (long) uid, (long) gid);
+ if (remember && + virSecurityDACRememberLabel(path) < 0) + return -1; + if (chown(path, uid, gid) < 0) { struct stat sb; int chown_errno = errno; @@ -310,23 +431,35 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) { struct stat buf; int rc = -1; + int ret; char *newpath = NULL; + uid_t user = (uid_t) -1; + gid_t group = (gid_t) -1;
VIR_INFO("Restoring DAC user and group on '%s'", path);
if (virFileResolveLink(path, &newpath) < 0) { virReportSystemError(errno, _("cannot resolve symlink %s"), path); - goto err; + goto cleanup; }
if (stat(newpath, &buf) != 0) - goto err; + goto cleanup;
- /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + ret = virSecurityDACGetRememberedLabel(newpath, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping label restore on '%s' as it is still " + "in use (refCount=%d)", newpath, ret); + rc = 0; + goto cleanup; + } + + rc = virSecurityDACSetOwnership(newpath, user, group, false);
-err: +cleanup: VIR_FREE(newpath); return rc; } @@ -348,7 +481,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, if (virSecurityDACGetImageIds(def, priv, &user, &group)) return -1;
- return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(path, user, group, true);
...and this change & all that follow will go away if you don't add the extra 'remember' parameter. 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 :|
For now, only two APIs are implemented: virFileSetACL for setting requested permissions for a specific user, virFileRemoveACL to remove those permissions. Both these traverse the whole path from root directory and set or unset S_IXUSR flag on all directories met so user can really access the file. --- configure.ac | 1 + libvirt.spec.in | 3 + src/libvirt_private.syms | 2 + src/util/virfile.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 7 ++ 5 files changed, 215 insertions(+) diff --git a/configure.ac b/configure.ac index cbb20c4..0df8a72 100644 --- a/configure.ac +++ b/configure.ac @@ -276,6 +276,7 @@ AM_CONDITIONAL([HAVE_LIBTASN1], [test "x$ac_cv_header_libtasn1_h" = "xyes"]) AC_CHECK_LIB([intl],[gettext],[]) AC_CHECK_LIB([attr],[getxattr]) +AC_CHECK_LIB([acl], [acl_init]) dnl Do we have rpcgen? AC_PATH_PROG([RPCGEN], [rpcgen], [no]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 90629c8..67c5799 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -580,6 +580,9 @@ BuildRequires: scrub # For xattr BuildRequires: libattr-devel +# For ACL +BuildRequires: libacl-devel + %if %{with_numad} BuildRequires: numad %endif diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc7568e..980e3ea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1275,8 +1275,10 @@ virFileFclose; virFileFdopen; virFileGetAttr; virFileLoopDeviceAssociate; +virFileRemoveACL; virFileRemoveAttr; virFileRewrite; +virFileSetACL; virFileSetAttr; virFileTouch; virFileUpdatePerm; diff --git a/src/util/virfile.c b/src/util/virfile.c index aa4579e..5d4254d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -42,6 +42,11 @@ # include <sys/types.h> #endif +#ifdef HAVE_LIBACL +# include <sys/acl.h> +# include <sys/types.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -750,3 +755,200 @@ virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, return -1; } #endif /* HAVE_LIBATTR */ + +#ifdef HAVE_LIBACL /* HAVE_LIBACL */ +static acl_entry_t +findACLEntry(acl_t acl, acl_tag_t type, id_t id) +{ + acl_entry_t ent; + acl_tag_t e_type; + id_t *e_id_p; + + /* acl_get_entry returns 1 if there's an entry in @acl */ + if (acl_get_entry(acl, ACL_FIRST_ENTRY, &ent) != 1) + return NULL; + + while (true) { + acl_get_tag_type(ent, &e_type); + if (e_type == type) { + if (id == ACL_UNDEFINED_ID) + return ent; + + if (!(e_id_p = acl_get_qualifier(ent))) + return NULL; + if (*e_id_p == id) { + acl_free(e_id_p); + return ent; + } + acl_free(e_id_p); + } + if (acl_get_entry(acl, ACL_NEXT_ENTRY, &ent) != 1) + return NULL; + } +} + +static void +setACLPerms(acl_entry_t ent, mode_t perms) +{ + acl_permset_t set; + + acl_get_permset(ent, &set); + if (perms & S_IRUSR) + acl_add_perm(set, ACL_READ); + else + acl_delete_perm(set, ACL_READ); + if (perms & S_IWUSR) + acl_add_perm(set, ACL_WRITE); + else + acl_delete_perm(set, ACL_WRITE); + if (perms & S_IXUSR) + acl_add_perm(set, ACL_EXECUTE); + else + acl_delete_perm(set, ACL_EXECUTE); +} + +static int +cloneACLEntry(acl_t fromAcl, acl_tag_t fromType, id_t user, + acl_t *toAcl, acl_tag_t toType) +{ + acl_entry_t fromEntry, toEntry; + if (!(fromEntry = findACLEntry(fromAcl, fromType, user))) + return 1; + + if (acl_create_entry(toAcl, &toEntry) != 0) { + virReportSystemError(errno, "%s", _("Unable to clone ACL entry")); + return -1; + } + + acl_copy_entry(toEntry, fromEntry); + acl_set_tag_type(toEntry, toType); + return 0; +} + +static int +virFileSetOrRemoveACLHelper(const char *path, + uid_t user, + mode_t perms, + bool set) +{ + int ret = -1; + acl_t acl; + acl_entry_t ent; + + if (!(acl = acl_get_file(path, ACL_TYPE_ACCESS))) { + virReportSystemError(errno, _("Unable to get ACL on %s"), path); + return ret; + } + + ent = findACLEntry(acl, ACL_USER, user); + if (!ent && set) { + if (acl_create_entry(&acl, &ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to create ACL entity")); + goto cleanup; + } + acl_set_tag_type(ent, ACL_USER); + acl_set_qualifier(ent, &user); + + setACLPerms(ent, perms); + + /* Recompute mask */ + if (!findACLEntry(acl, ACL_MASK, ACL_UNDEFINED_ID) && + cloneACLEntry(acl, ACL_USER, user, &acl, ACL_MASK) < 0) + goto cleanup; + } else if (ent && !set) { + if (acl_delete_entry(acl, ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to delete ACL entity")); + goto cleanup; + } + + if ((ent = findACLEntry(acl, ACL_MASK, user)) && + acl_delete_entry(acl, ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to delete ACL entity")); + goto cleanup; + } + } + + if (acl_set_file(path, ACL_TYPE_ACCESS, acl) < 0) { + virReportSystemError(errno, _("Unable to set ACL on %s"), path); + goto cleanup; + } + + ret = 0; +cleanup: + acl_free(acl); + return ret; +} + +static int +virFileSetOrRemoveACL(const char *file, + uid_t user, + mode_t perms, + bool set) +{ + int ret = -1; + char *fileDup = NULL; + char *p; + + if (!(fileDup = strdup(file))) { + virReportOOMError(); + return ret; + } + + if (virFileSetOrRemoveACLHelper(file, user, perms, set) < 0) + goto cleanup; + + /* For parent directories we want executable flag */ + perms |= S_IXUSR; + + while ((p = strrchr(fileDup, '/')) != fileDup) { + if (!p) { + virReportSystemError(EINVAL, _("Invalid relative path '%s'"), file); + goto cleanup; + } + + *p = '\0'; + + if (virFileSetOrRemoveACLHelper(fileDup, user, perms, set) < 0) + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(fileDup); + return ret; +} + +int +virFileSetACL(const char *file, + uid_t user, + mode_t perms) +{ + return virFileSetOrRemoveACL(file, user, perms, true); +} + +int +virFileRemoveACL(const char *file, + uid_t user) +{ + return virFileSetOrRemoveACL(file, user, 0, false); +} + +#else /* HAVE_LIBACL */ +int +virFileSetACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED, + mode_t perms ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Unable to set ACL")); + return -1; +} + +int +virFileRemoveACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Unable to remove ACL")); + return -1; +} +#endif /* HAVE_LIBACL */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 3b4d672..b530aed 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -120,4 +120,11 @@ int virFileGetAttr(const char *file, int virFileRemoveAttr(const char *file, const char *name); +int virFileSetACL(const char *file, + uid_t user, + mode_t perms); + +int virFileRemoveACL(const char *file, + uid_t user); + #endif /* __VIR_FILES_H */ -- 1.8.1.5
On Wed, Mar 06, 2013 at 03:05:55PM +0100, Michal Privoznik wrote:
For now, only two APIs are implemented: virFileSetACL for setting requested permissions for a specific user, virFileRemoveACL to remove those permissions.
Both these traverse the whole path from root directory and set or unset S_IXUSR flag on all directories met so user can really access the file. --- configure.ac | 1 + libvirt.spec.in | 3 + src/libvirt_private.syms | 2 + src/util/virfile.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 7 ++ 5 files changed, 215 insertions(+)
diff --git a/configure.ac b/configure.ac index cbb20c4..0df8a72 100644 --- a/configure.ac +++ b/configure.ac @@ -276,6 +276,7 @@ AM_CONDITIONAL([HAVE_LIBTASN1], [test "x$ac_cv_header_libtasn1_h" = "xyes"])
AC_CHECK_LIB([intl],[gettext],[]) AC_CHECK_LIB([attr],[getxattr]) +AC_CHECK_LIB([acl], [acl_init])
Don't do this - add a m4/virt-acl.m4 file, following the example of virt-attr.m4 and then just call it from the same place as other library checks. You'll need to add libs + cflags variables to the Makefile.am too
diff --git a/src/util/virfile.c b/src/util/virfile.c index aa4579e..5d4254d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -42,6 +42,11 @@ # include <sys/types.h> #endif
+#ifdef HAVE_LIBACL +# include <sys/acl.h> +# include <sys/types.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -750,3 +755,200 @@ virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, return -1; } #endif /* HAVE_LIBATTR */ + +#ifdef HAVE_LIBACL /* HAVE_LIBACL */
Pointless comment at end of line
+static acl_entry_t +findACLEntry(acl_t acl, acl_tag_t type, id_t id)
Please use virFileACL as prefix for all methods, even static ones
+{ + acl_entry_t ent; + acl_tag_t e_type; + id_t *e_id_p; + + /* acl_get_entry returns 1 if there's an entry in @acl */ + if (acl_get_entry(acl, ACL_FIRST_ENTRY, &ent) != 1) + return NULL; + + while (true) { + acl_get_tag_type(ent, &e_type); + if (e_type == type) { + if (id == ACL_UNDEFINED_ID) + return ent; + + if (!(e_id_p = acl_get_qualifier(ent))) + return NULL; + if (*e_id_p == id) { + acl_free(e_id_p); + return ent; + } + acl_free(e_id_p); + } + if (acl_get_entry(acl, ACL_NEXT_ENTRY, &ent) != 1) + return NULL; + }
Instead of while (true); use do { ... } while(acl_get_entry(acl, ACL_NEXT_ENTRY) != 0);
+} + +static void +setACLPerms(acl_entry_t ent, mode_t perms) +{ + acl_permset_t set; + + acl_get_permset(ent, &set); + if (perms & S_IRUSR) + acl_add_perm(set, ACL_READ); + else + acl_delete_perm(set, ACL_READ); + if (perms & S_IWUSR) + acl_add_perm(set, ACL_WRITE); + else + acl_delete_perm(set, ACL_WRITE); + if (perms & S_IXUSR) + acl_add_perm(set, ACL_EXECUTE); + else + acl_delete_perm(set, ACL_EXECUTE); +} + +static int +cloneACLEntry(acl_t fromAcl, acl_tag_t fromType, id_t user, + acl_t *toAcl, acl_tag_t toType) +{ + acl_entry_t fromEntry, toEntry; + if (!(fromEntry = findACLEntry(fromAcl, fromType, user))) + return 1; + + if (acl_create_entry(toAcl, &toEntry) != 0) { + virReportSystemError(errno, "%s", _("Unable to clone ACL entry")); + return -1; + } + + acl_copy_entry(toEntry, fromEntry); + acl_set_tag_type(toEntry, toType); + return 0; +} + +static int +virFileSetOrRemoveACLHelper(const char *path, + uid_t user, + mode_t perms, + bool set) +{ + int ret = -1; + acl_t acl; + acl_entry_t ent; + + if (!(acl = acl_get_file(path, ACL_TYPE_ACCESS))) { + virReportSystemError(errno, _("Unable to get ACL on %s"), path); + return ret; + } + + ent = findACLEntry(acl, ACL_USER, user); + if (!ent && set) { + if (acl_create_entry(&acl, &ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to create ACL entity")); + goto cleanup; + } + acl_set_tag_type(ent, ACL_USER); + acl_set_qualifier(ent, &user); + + setACLPerms(ent, perms); + + /* Recompute mask */ + if (!findACLEntry(acl, ACL_MASK, ACL_UNDEFINED_ID) && + cloneACLEntry(acl, ACL_USER, user, &acl, ACL_MASK) < 0) + goto cleanup; + } else if (ent && !set) { + if (acl_delete_entry(acl, ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to delete ACL entity")); + goto cleanup; + } + + if ((ent = findACLEntry(acl, ACL_MASK, user)) && + acl_delete_entry(acl, ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to delete ACL entity")); + goto cleanup; + } + } + + if (acl_set_file(path, ACL_TYPE_ACCESS, acl) < 0) { + virReportSystemError(errno, _("Unable to set ACL on %s"), path); + goto cleanup; + } + + ret = 0; +cleanup: + acl_free(acl); + return ret; +} + +static int +virFileSetOrRemoveACL(const char *file, + uid_t user, + mode_t perms, + bool set) +{ + int ret = -1; + char *fileDup = NULL; + char *p; + + if (!(fileDup = strdup(file))) { + virReportOOMError(); + return ret; + } + + if (virFileSetOrRemoveACLHelper(file, user, perms, set) < 0) + goto cleanup; + + /* For parent directories we want executable flag */ + perms |= S_IXUSR; + + while ((p = strrchr(fileDup, '/')) != fileDup) { + if (!p) { + virReportSystemError(EINVAL, _("Invalid relative path '%s'"), file); + goto cleanup; + } + + *p = '\0'; + + if (virFileSetOrRemoveACLHelper(fileDup, user, perms, set) < 0) + goto cleanup; + }
Hmm, this is recursively setting ACLs all the way up the directory tree. How is this going to work when some files share some parent directories. eg virFileSetACL("/var/lib/libvirt/images/foo.img"); Sets ACLs on /var, /var/lib, /var/lib/libvirt, /var/lib/libvirt/images virFileSetACL("/var/lib/libvirt/images/bar.img"); This does the same virFileRemoveACL("/var/lib/libvirt/images/foo.img"); Removes ACLs on /var, /var/lib, /var/lib/libvirt, /var/lib/libvirt/images ....but bar.img still wanted them set. IMHO this recursive setting of ACLs doesn't belong in these APIs because they can't safely unset things recursively. Also such changing of directories is not related to conversion from chmod -> acls. This is a completely new feature, so should be done in a separate patch. 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 :|
On filesystems supporting ACLs we don't need to do a chown but we can just set ACLs to gain access for qemu. However, since we are setting these on too low level, where we don't know if disk is just a read only or read write, we set read write access unconditionally. --- src/security/security_dac.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 76a1dc6..8805a5b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -329,12 +329,12 @@ virSecurityDACGetRememberedLabel(const char *path, goto cleanup; ret = refCount; } else { - if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || - !oldOwner) - return ret; + if (!virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) && + oldOwner) { - if (parseIds(oldOwner, user, group) < 0) - goto cleanup; + if (parseIds(oldOwner, user, group) < 0) + goto cleanup; + } virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); @@ -384,6 +384,9 @@ virSecurityDACSetOwnership(const char *path, gid_t gid, bool remember) { + virErrorPtr err; + int rv; + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", path, (long) uid, (long) gid); @@ -391,6 +394,25 @@ virSecurityDACSetOwnership(const char *path, virSecurityDACRememberLabel(path) < 0) return -1; + if (remember) { + if ((rv = virFileSetACL(path, uid, S_IRUSR | S_IWUSR)) == 0) { + /* No chown is necessary, so remove oldOwner xattr. */ + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); + } + } else { + rv = virFileRemoveACL(path, uid); + } + + if (rv < 0) { + err = virGetLastError(); + /* If we obtained an error other than ENOSYS (=ACLs are not supported) + * report it. Otherwise fall back to chown(). */ + if (err && err->code != VIR_ERR_SYSTEM_ERROR && err->int1 != ENOSYS) + return rv; + } else { + return rv; + } + if (chown(path, uid, gid) < 0) { struct stat sb; int chown_errno = errno; -- 1.8.1.5
On Wed, Mar 06, 2013 at 03:05:56PM +0100, Michal Privoznik wrote:
On filesystems supporting ACLs we don't need to do a chown but we can just set ACLs to gain access for qemu. However, since we are setting these on too low level, where we don't know if disk is just a read only or read write, we set read write access unconditionally. --- src/security/security_dac.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 76a1dc6..8805a5b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -329,12 +329,12 @@ virSecurityDACGetRememberedLabel(const char *path, goto cleanup; ret = refCount; } else { - if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || - !oldOwner) - return ret; + if (!virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) && + oldOwner) {
- if (parseIds(oldOwner, user, group) < 0) - goto cleanup; + if (parseIds(oldOwner, user, group) < 0) + goto cleanup; + }
virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); @@ -384,6 +384,9 @@ virSecurityDACSetOwnership(const char *path, gid_t gid, bool remember) { + virErrorPtr err; + int rv; + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", path, (long) uid, (long) gid);
@@ -391,6 +394,25 @@ virSecurityDACSetOwnership(const char *path, virSecurityDACRememberLabel(path) < 0) return -1;
+ if (remember) { + if ((rv = virFileSetACL(path, uid, S_IRUSR | S_IWUSR)) == 0) { + /* No chown is necessary, so remove oldOwner xattr. */ + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER);
Ewww, just re-arrange this code, so that you don't set the xattr in the first place when you use ACLs.
+ } + } else { + rv = virFileRemoveACL(path, uid);
And this should just be done directly in the restore method 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 :|
participants (2)
-
Daniel P. Berrange -
Michal Privoznik