Re: [libvirt] [PATCH v5 06/11] qemu_cgroup: Allow /dev/mapper/control for PR

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&gt; > --- > 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?

Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;