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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
> + ret = 0;
> + }
> goto cleanup;
> + }
>
> if (!dm_task_get_info(dmt, &info))
> goto cleanup;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
