[libvirt] [PATCH 4/5] qemu: Handle multipath devices properly

[Peter Krempa]

On Mon, Mar 26, 2018 at 07:16:45 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1557769
> 
> Problem with multipath devices 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 at redhat.com>
> ---
>  libvirt.spec.in        |  2 ++
>  src/qemu/qemu_cgroup.c | 43 ++++++++++++++++++++++++++++++++++---
>  src/qemu/qemu_domain.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+), 3 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..a2198c9789 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -60,7 +60,12 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      int perms = VIR_CGROUP_DEVICE_READ;
> -    int ret;
> +    unsigned long long *mpathdevs = NULL;
> +    size_t nmpathdevs = 0;
> +    size_t i;
> +    char *devPath = NULL;
> +    int rv;
> +    int ret = -1;
>  
>      if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
>          return 0;
> @@ -71,12 +76,44 @@ 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 (virFileGetMPathTargets(path, &mpathdevs, &nmpathdevs) < 0 &&
> +        errno != ENOSYS && errno != EBADF) {
> +        virReportSystemError(errno,
> +                             _("Unable to get mpath targets for %s"),
> +                             path);
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < nmpathdevs; i++) {
> +        if (virFileMajMinToName(mpathdevs[i], &devPath) < 0) {

So for cgroups this really isn't necessary. You can use
virCgroupAllowDevice which already takes the major/minor numbers rather
than a path. Given that virCgroupAllowDevicePath would convert the path
to major/minor numbers, this conversion is really wasteful.

> +            virReportSystemError(errno,
> +                                 _("Unable to translate %llx to device path"),
> +                                 mpathdevs[i]);

This error message would be very useless, given that it's not coverted
into major/minor.

> +            goto cleanup;
> +        }
> +
> +        rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
> +
> +        virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
> +                                 virCgroupGetDevicePermsString(perms),
> +                                 rv);

Hmm, right. The message in the audit log when allowing the device would
not contain the path to the device, which would make it slightly harder
to use.


> +        if (rv < 0)
> +            goto cleanup;
> +        VIR_FREE(devPath);
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(devPath);
> +    VIR_FREE(mpathdevs);
>      return ret;

The only thing is that I'm not really a fan of calling the detection
code twice for every device you are going to add to the VM, once for
cgroups and once for namespaces. I think that the disk image setup code
should be extracted, so that the data needs to be obtained only once and
afterwards can be used to setup all the required images.

I'm not going to insist, but that also means that nobody will ever do
it. Especially since we for some reason hide all cgroup code setup into
qemu_cgroup. This also means that it has to be duplicated for
namespaces.

The code will require some changes if you implement my suggestions, but
it looks reasonable.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180326/b12d1519/attachment-0001.sig>