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@xxxxxxxxxx> > --- > 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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
