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

[John Ferlan]

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 at 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 at redhat.com>

John

> +            ret = 0;
> +        }
>          goto cleanup;
> +    }
>  
>      if (!dm_task_get_info(dmt, &info))
>          goto cleanup;
>