[libvirt] [PATCH v3 0/3] Improve handling of multipath devices
The saga continues. Turns out, this is bug in both libvirt AND kernel. Firstly, kernel uses different CGroup for checking open() than for ioctl() [1]. However, libvirt needs to allow all targets in the CGroup too. Not only it's logical, kernel devel says it never worked. QEMU was able to open the devmapper device but any subsequent read()/write() would fail because of CGroup perms. Anyway, it's not only RHEL which is affected by this bug. I've upgraded my Gentoo to 4.16 and I'm hitting it even though I was not with 4.15. It all started with upstream kernel commit of 519049afead4f7c3e6446028. This changed call in dm_blk_ioctl() from dm_grab_bdev_for_ioctl() to dm_get_bdev_for_ioctl(). The former merely increases refcount on some internal kernel strcut, the latter calls blkdev_get() which calls __blkdev_get() which actually performs CGroup check. 1: https://bugzilla.redhat.com/show_bug.cgi?id=1557769#c54 Diff to v2: - Changed virDevMapperGetTargets() to do recursive target fetching, and at the same time make it return string list of devices rather than MAJ:MIN pairs. This is because dm_task_set_name() accepts path as string. There's also dm_task_set_major_minor() but: a) handling multiple arrays and merging them (because of recursion) - the code would look ugly, b) we don't need to introduce virDomainAuditCgroupMajorMinor() just to pass MAJ:MIN to it. - Implemented dropping of permissions when unplugging a disk (was missing in previous versions) Michal Privoznik (3): util: Introduce virDevMapperGetTargets qemu_cgroup: Handle device mapper targets properly news: Document device mapper fix docs/news.xml | 10 +++ libvirt.spec.in | 2 + src/libvirt_private.syms | 4 + src/qemu/qemu_cgroup.c | 69 +++++++++++++--- src/util/Makefile.inc.am | 2 + src/util/virdevmapper.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdevmapper.h | 31 ++++++++ 7 files changed, 309 insertions(+), 9 deletions(-) create mode 100644 src/util/virdevmapper.c create mode 100644 src/util/virdevmapper.h -- 2.16.1
This helper fetches dependencies for given device mapper target. At the same time, we need to provide a dummy log function because by default libdevmapper prints out error messages to stderr which we need to suppress. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 4 + src/util/Makefile.inc.am | 2 + src/util/virdevmapper.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdevmapper.h | 31 ++++++++ 4 files changed, 237 insertions(+) create mode 100644 src/util/virdevmapper.c create mode 100644 src/util/virdevmapper.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 03fe3b315f..3b7a19cff1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1669,6 +1669,10 @@ virDBusMessageUnref; virDBusSetSharedBus; +# util/virdevmapper.h +virDevMapperGetTargets; + + # util/virdnsmasq.h dnsmasqAddDhcpHost; dnsmasqAddHost; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index a3c3b711fd..9624fb687c 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -35,6 +35,8 @@ UTIL_SOURCES = \ util/virdbus.c \ util/virdbus.h \ util/virdbuspriv.h \ + util/virdevmapper.c \ + util/virdevmapper.h \ util/virdnsmasq.c \ util/virdnsmasq.h \ util/virebtables.c \ diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c new file mode 100644 index 0000000000..9491848f48 --- /dev/null +++ b/src/util/virdevmapper.c @@ -0,0 +1,200 @@ +/* + * virdevmapper.c: Functions for handling devmapper + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#ifdef MAJOR_IN_MKDEV +# include <sys/mkdev.h> +#elif MAJOR_IN_SYSMACROS +# include <sys/sysmacros.h> +#endif + +#ifdef WITH_DEVMAPPER +# include <libdevmapper.h> +#endif + +#include "virdevmapper.h" +#include "internal.h" +#include "virthread.h" +#include "viralloc.h" +#include "virstring.h" + +#ifdef WITH_DEVMAPPER +static void +virDevMapperDummyLogger(int level ATTRIBUTE_UNUSED, + const char *file ATTRIBUTE_UNUSED, + int line ATTRIBUTE_UNUSED, + int dm_errno ATTRIBUTE_UNUSED, + const char *fmt ATTRIBUTE_UNUSED, + ...) +{ + return; +} + +static int +virDevMapperOnceInit(void) +{ + /* Ideally, we would not need this. But libdevmapper prints + * error messages to stderr by default. Sad but true. */ + dm_log_with_errno_init(virDevMapperDummyLogger); + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virDevMapper) + + +static int +virDevMapperGetTargetsImpl(const char *path, + char ***devPaths_ret, + unsigned int ttl) +{ + struct dm_task *dmt = NULL; + struct dm_deps *deps; + struct dm_info info; + char **devPaths = NULL; + char **recursiveDevPaths = NULL; + size_t i; + int ret = -1; + + *devPaths_ret = NULL; + + if (virDevMapperInitialize() < 0) + return ret; + + if (!ttl) { + errno = ELOOP; + return ret; + } + + if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) + return ret; + + if (!dm_task_set_name(dmt, path)) { + if (errno == ENOENT) { + /* It's okay, @path is not managed by devmapper => + * not a devmapper device. */ + ret = 0; + } + goto cleanup; + } + + dm_task_no_open_count(dmt); + + if (!dm_task_run(dmt)) + goto cleanup; + + if (!dm_task_get_info(dmt, &info)) + goto cleanup; + + if (!info.exists) { + ret = 0; + goto cleanup; + } + + if (!(deps = dm_task_get_deps(dmt))) + goto cleanup; + + if (VIR_ALLOC_N_QUIET(devPaths, deps->count + 1) < 0) + goto cleanup; + + for (i = 0; i < deps->count; i++) { + if (virAsprintfQuiet(&devPaths[i], "/dev/block/%u:%u", + major(deps->device[i]), + minor(deps->device[i])) < 0) + goto cleanup; + } + + recursiveDevPaths = NULL; + for (i = 0; i < deps->count; i++) { + char **tmpPaths; + + if (virDevMapperGetTargetsImpl(devPaths[i], &tmpPaths, ttl - 1) < 0) + goto cleanup; + + if (tmpPaths && + virStringListMerge(&recursiveDevPaths, &tmpPaths) < 0) { + virStringListFree(tmpPaths); + goto cleanup; + } + } + + if (virStringListMerge(&devPaths, &recursiveDevPaths) < 0) + goto cleanup; + + VIR_STEAL_PTR(*devPaths_ret, devPaths); + ret = 0; + cleanup: + virStringListFree(recursiveDevPaths); + virStringListFree(devPaths); + dm_task_destroy(dmt); + return ret; +} + + +/** + * virDevMapperGetTargets: + * @path: devmapper target + * @devPaths: returned string list of devices + * + * For given @path figure out its targets, and store them in + * @devPaths array. Note, @devPaths is a string list so it's NULL + * terminated. + * + * If @path is not a devmapper device, @devPaths is set to NULL and + * success is returned. + * + * If @path consists of yet another devmapper targets these are + * consulted recursively. + * + * If we don't have permissions to talk to kernel, -1 is returned + * and errno is set to EBADF. + * + * Returns 0 on success, + * -1 otherwise (with errno set, no libvirt error is + * reported) + */ +int +virDevMapperGetTargets(const char *path, + char ***devPaths) +{ + const unsigned int ttl = 4; + + /* Arbitrary limit on recursion level. A devmapper target can + * consist of devices or yet another targets. If that's the + * case, we have to stop recursion somewhere. My dice chose + * four. */ + + return virDevMapperGetTargetsImpl(path, devPaths, ttl); +} + +#else /* ! WITH_DEVMAPPER */ + +int +virDevMapperGetTargets(const char *path ATTRIBUTE_UNUSED, + char ***devPaths ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + return -1; +} +#endif /* ! WITH_DEVMAPPER */ diff --git a/src/util/virdevmapper.h b/src/util/virdevmapper.h new file mode 100644 index 0000000000..83ec03bd4e --- /dev/null +++ b/src/util/virdevmapper.h @@ -0,0 +1,31 @@ +/* + * virdevmapper.h: Functions for handling devmapper + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Michal Privoznik <mprivozn@redhat.com> + */ + +#ifndef __VIR_DEVMAPPER_H__ +# define __VIR_DEVMAPPER_H__ + +int +virDevMapperGetTargets(const char *path, + char ***devPaths); + +#endif /* __VIR_DEVMAPPER_H__ */ -- 2.16.1
On Tue, Apr 03, 2018 at 11:03:25 +0200, Michal Privoznik wrote:
This helper fetches dependencies for given device mapper target.
At the same time, we need to provide a dummy log function because by default libdevmapper prints out error messages to stderr which we need to suppress.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 4 + src/util/Makefile.inc.am | 2 + src/util/virdevmapper.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdevmapper.h | 31 ++++++++ 4 files changed, 237 insertions(+) create mode 100644 src/util/virdevmapper.c create mode 100644 src/util/virdevmapper.h
[...]
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c new file mode 100644 index 0000000000..9491848f48 --- /dev/null +++ b/src/util/virdevmapper.c @@ -0,0 +1,200 @@ +/* + * virdevmapper.c: Functions for handling devmapper
device mapper
+ * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h>
[...]
+static int +virDevMapperGetTargetsImpl(const char *path, + char ***devPaths_ret, + unsigned int ttl) +{ + struct dm_task *dmt = NULL; + struct dm_deps *deps; + struct dm_info info; + char **devPaths = NULL; + char **recursiveDevPaths = NULL; + size_t i; + int ret = -1; + + *devPaths_ret = NULL; + + if (virDevMapperInitialize() < 0) + return ret; + + if (!ttl) {
if (ttl == 0)
+ errno = ELOOP; + return ret; + } + + if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) + return ret; + + if (!dm_task_set_name(dmt, path)) { + if (errno == ENOENT) { + /* It's okay, @path is not managed by devmapper => + * not a devmapper device. */ + ret = 0; + } + goto cleanup; + } + + dm_task_no_open_count(dmt); + + if (!dm_task_run(dmt)) + goto cleanup; + + if (!dm_task_get_info(dmt, &info)) + goto cleanup; + + if (!info.exists) { + ret = 0; + goto cleanup; + } + + if (!(deps = dm_task_get_deps(dmt))) + goto cleanup; + + if (VIR_ALLOC_N_QUIET(devPaths, deps->count + 1) < 0) + goto cleanup; + + for (i = 0; i < deps->count; i++) { + if (virAsprintfQuiet(&devPaths[i], "/dev/block/%u:%u", + major(deps->device[i]), + minor(deps->device[i])) < 0) + goto cleanup; + } + + recursiveDevPaths = NULL; + for (i = 0; i < deps->count; i++) { + char **tmpPaths; + + if (virDevMapperGetTargetsImpl(devPaths[i], &tmpPaths, ttl - 1) < 0) + goto cleanup; + + if (tmpPaths && + virStringListMerge(&recursiveDevPaths, &tmpPaths) < 0) {
So these report libvirt errors, but don't mess them up. And only in case of OOM, so this is fine.
+ virStringListFree(tmpPaths); + goto cleanup; + } + } + + if (virStringListMerge(&devPaths, &recursiveDevPaths) < 0) + goto cleanup; + + VIR_STEAL_PTR(*devPaths_ret, devPaths); + ret = 0; + cleanup: + virStringListFree(recursiveDevPaths); + virStringListFree(devPaths); + dm_task_destroy(dmt); + return ret; +} + + +/** + * virDevMapperGetTargets: + * @path: devmapper target + * @devPaths: returned string list of devices + * + * For given @path figure out its targets, and store them in + * @devPaths array. Note, @devPaths is a string list so it's NULL + * terminated. + * + * If @path is not a devmapper device, @devPaths is set to NULL and + * success is returned. + * + * If @path consists of yet another devmapper targets these are + * consulted recursively. + * + * If we don't have permissions to talk to kernel, -1 is returned + * and errno is set to EBADF. + * + * Returns 0 on success, + * -1 otherwise (with errno set, no libvirt error is + * reported) + */ +int +virDevMapperGetTargets(const char *path, + char ***devPaths) +{ + const unsigned int ttl = 4;
So even trivial setups (dmcrypt + LVM) would exhaust half of your limit for the device mapper block devices, and would still recurse one more level to check the backing block device. I think we need some more headroom here. I'd go with 16 or 32.
+ + /* Arbitrary limit on recursion level. A devmapper target can + * consist of devices or yet another targets. If that's the + * case, we have to stop recursion somewhere. My dice chose + * four. */
Drop the last sentence of this comment.
+ + return virDevMapperGetTargetsImpl(path, devPaths, ttl); +} +
ACK if you fix the condition syntax, raise the recursion level significantly, and tweak the comment.
https://bugzilla.redhat.com/show_bug.cgi?id=1557769 Problem with device mapper targets is that there can be several other devices 'hidden' behind them. For instance, /dev/dm-1 can consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when setting up devices CGroup and namespaces we have to take this into account. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 2 ++ src/qemu/qemu_cgroup.c | 69 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index b55a947ec9..ebfac10866 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -796,6 +796,8 @@ Requires: gzip Requires: bzip2 Requires: lzop Requires: xz +# For mpath devices +Requires: device-mapper %if 0%{?fedora} || 0%{?rhel} > 7 Requires: systemd-container %endif diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b604edb31c..42502e1b03 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -37,6 +37,7 @@ #include "virtypedparam.h" #include "virnuma.h" #include "virsystemd.h" +#include "virdevmapper.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -60,7 +61,10 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; int perms = VIR_CGROUP_DEVICE_READ; - int ret; + char **targetPaths = NULL; + size_t i; + int rv; + int ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; @@ -71,12 +75,35 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, VIR_DEBUG("Allow path %s, perms: %s", path, virCgroupGetDevicePermsString(perms)); - ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true); + rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, virCgroupGetDevicePermsString(perms), - ret); + rv); + if (rv < 0) + goto cleanup; + if (virDevMapperGetTargets(path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno, + _("Unable to get devmapper targets for %s"), + path); + goto cleanup; + } + + for (i = 0; targetPaths && targetPaths[i]; i++) { + rv = virCgroupAllowDevicePath(priv->cgroup, targetPaths[i], perms, false); + + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", targetPaths[i], + virCgroupGetDevicePermsString(perms), + rv); + if (rv < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(targetPaths); return ret; } @@ -109,10 +136,11 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; - int perms = VIR_CGROUP_DEVICE_READ | - VIR_CGROUP_DEVICE_WRITE | - VIR_CGROUP_DEVICE_MKNOD; - int ret; + int perms = VIR_CGROUP_DEVICE_RWM; + char **targetPaths = NULL; + size_t i; + int rv; + int ret = -1; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) @@ -126,11 +154,34 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, VIR_DEBUG("Deny path %s", src->path); - ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); + rv = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, - virCgroupGetDevicePermsString(perms), ret); + virCgroupGetDevicePermsString(perms), rv); + if (rv < 0) + goto cleanup; + if (virDevMapperGetTargets(src->path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno, + _("Unable to get devmapper targets for %s"), + src->path); + goto cleanup; + } + + for (i = 0; targetPaths && targetPaths[i]; i++) { + rv = virCgroupDenyDevicePath(priv->cgroup, targetPaths[i], perms, false); + + virDomainAuditCgroupPath(vm, priv->cgroup, "deny", targetPaths[i], + virCgroupGetDevicePermsString(perms), + rv); + if (rv < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(targetPaths); return ret; } -- 2.16.1
On Tue, Apr 03, 2018 at 11:03:26 +0200, Michal Privoznik wrote:
This BZ is private. Please don't use links which can't be viewed by the public.
Problem with device mapper targets is that there can be several other devices 'hidden' behind them. For instance, /dev/dm-1 can consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when setting up devices CGroup and namespaces we have to take this into account.
Also I'd appreciate if you could justify this here. Mention that the kernel was fixed and this was supposed to be happening from the beggining.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 2 ++ src/qemu/qemu_cgroup.c | 69 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 9 deletions(-)
[...]
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b604edb31c..42502e1b03 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c
[...]
@@ -71,12 +75,35 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, VIR_DEBUG("Allow path %s, perms: %s", path, virCgroupGetDevicePermsString(perms));
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true); + rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, virCgroupGetDevicePermsString(perms), - ret); + rv); + if (rv < 0) + goto cleanup;
+ if (virDevMapperGetTargets(path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno, + _("Unable to get devmapper targets for %s"), + path); + goto cleanup; + } + + for (i = 0; targetPaths && targetPaths[i]; i++) { + rv = virCgroupAllowDevicePath(priv->cgroup, targetPaths[i], perms, false); + + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", targetPaths[i], + virCgroupGetDevicePermsString(perms), + rv); + if (rv < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(targetPaths); return ret; }
This looks okay. I did not check that the 'errnos' returned by libdevmapper are sane given your checks though. [...]
@@ -126,11 +154,34 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
VIR_DEBUG("Deny path %s", src->path);
- ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); + rv = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true);
virDomainAuditCgroupPath(vm, priv->cgroup, "deny", src->path, - virCgroupGetDevicePermsString(perms), ret); + virCgroupGetDevicePermsString(perms), rv); + if (rv < 0) + goto cleanup;
+ if (virDevMapperGetTargets(src->path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno, + _("Unable to get devmapper targets for %s"), + src->path); + goto cleanup; + } + + for (i = 0; targetPaths && targetPaths[i]; i++) { + rv = virCgroupDenyDevicePath(priv->cgroup, targetPaths[i], perms, false);
This isn't a good idea. If a backing device of a device mapper volume is shared between two disks, this would rip it out of cgroups even if it would be still used. I think a simple reproducer for this should be if you use two LVs in the same VG for backing two disks and hot-unplug one of them.
On Tue, Apr 03, 2018 at 11:03:26 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1557769
Problem with device mapper targets is that there can be several other devices 'hidden' behind them. For instance, /dev/dm-1 can consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when setting up devices CGroup and namespaces we have to take this into account.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 2 ++ src/qemu/qemu_cgroup.c | 69 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 9 deletions(-)
[...]
@@ -71,12 +75,35 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, VIR_DEBUG("Allow path %s, perms: %s", path, virCgroupGetDevicePermsString(perms));
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true); + rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
So this returns 1 if 'path' is not a char or block device ...
virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, virCgroupGetDevicePermsString(perms), - ret); + rv); + if (rv < 0) + goto cleanup;
+ if (virDevMapperGetTargets(path, &targetPaths) < 0 && + errno != ENOSYS && errno != EBADF) { + virReportSystemError(errno,
So in that case this is definitely not necessary and should be skipped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 87f52e83ef..065b60b6eb 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -49,6 +49,16 @@ </change> </section> <section title="Bug fixes"> + <change> + <summary> + Improve handling of device mapper targets + </summary> + <description> + When starting a domain with a disk backed by a + device-mapper volume libvirt also needs to allow the + storage backing the device mapper in cgroups. + </description> + </change> </section> </release> <release version="v4.2.0" date="2018-04-01"> -- 2.16.1
On Tue, Apr 03, 2018 at 11:03:27 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.xml | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml index 87f52e83ef..065b60b6eb 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -49,6 +49,16 @@ </change> </section> <section title="Bug fixes"> + <change> + <summary> + Improve handling of device mapper targets + </summary> + <description> + When starting a domain with a disk backed by a + device-mapper volume libvirt also needs to allow the + storage backing the device mapper in cgroups.
Again, some background for this would be welcome.
+ </description> + </change> </section> </release> <release version="v4.2.0" date="2018-04-01"> -- 2.16.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Michal Privoznik -
Peter Krempa