7:34 a.m.
On 04/23/2018 09:14 AM, Michal Privoznik wrote:
Just like in previous commit, qemu-pr-helper might want to open
/dev/mapper/control under certain circumstances. Therefore we
have to allow it in cgroups.
The change virdevmapper.c might look spurious but it isn't. After
6dd84f6850ca437 any path that we're allowing in deivces CGroup is
devices
subject to virDevMapperGetTargets() inspection. And libdevmapper
returns ENXIO for the path from subject.
^^^ IMO: This explanation (minus the commit id reference) belongs where
you check for ENXIO. As a reader of code I don't necessarily check
commit messages for reasons a check exists.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 33 ++++++++++++++++++++++++++++++---
src/util/virdevmapper.c | 8 +++++++-
2 files changed, 37 insertions(+), 4 deletions(-)
I would say a similar echo here as in the NS patch - since the
subsequent patch will have a way to know that PR is running/started,
then why not use that knowledge or similar logic to helper determine
whether we need to add NS/Cgroup and whether we need to remove the
cgroup reference (if that even matters by that point in time).
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index d88eb7881f..546a4c8e63 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
}
+#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
+
static int
qemuSetupImageCgroupInternal(virDomainObjPtr vm,
virStorageSourcePtr src,
@@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm,
return 0;
}
+ if (virStoragePRDefIsManaged(src->pr) &&
+ qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0)
+ return -1;
+
return qemuSetupImagePathCgroup(vm, src->path, src->readonly ||
forceReadonly);
}
@@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
virStorageSourcePtr src)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- int perms = VIR_CGROUP_DEVICE_READ |
- VIR_CGROUP_DEVICE_WRITE |
- VIR_CGROUP_DEVICE_MKNOD;
+ int perms = VIR_CGROUP_DEVICE_RWM;
+ size_t i;
int ret;
if (!virCgroupHasController(priv->cgroup,
@@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
return 0;
}
+ for (i = 0; i < vm->def->ndisks; i++) {
+ virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
+
+ if (src == diskSrc)
+ continue;
+
+ if (virStoragePRDefIsManaged(diskSrc->pr))
+ break;
+ }
+
+ if (i == vm->def->ndisks) {
+ VIR_DEBUG("Disabling device mapper control");
+ ret = virCgroupDenyDevicePath(priv->cgroup,
+ DEVICE_MAPPER_CONTROL_PATH, perms, true);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
+ DEVICE_MAPPER_CONTROL_PATH,
+ virCgroupGetDevicePermsString(perms), ret);
+ if (ret < 0)
+ return ret;
+ }
+
+
VIR_DEBUG("Deny path %s", src->path);
ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true);
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index d2c25af003..ef4b1e480a 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -101,8 +101,14 @@ virDevMapperGetTargetsImpl(const char *path,
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt))
+ if (!dm_task_run(dmt)) {
+ if (errno == ENXIO) {
+ /* In some cases devmapper realizes this late device
+ * is not managed by it. */
So my question here is that is "some cases" limited to just one device
or would it be multiple? Let's be explicit - better to understand now
than chase later. I think one only consider Laine's chasing in nwfilter
to realize that if we have to do something to handle some special
condition as a result of how we use something, then documenting it for
future bug chaster to find *in code* as opposed to *in commit messages*
may actually help diagnose problems quicker.
So for the code and assuming the comments get rearranged a bit,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ ret = 0;
+ }
goto cleanup;
+ }
if (!dm_task_get_info(dmt, &info))
goto cleanup;