<div dir="ltr">Hi <div> <div class="gmail_quote"><div dir="ltr">On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <<a href="mailto:mprivozn@redhat.com">mprivozn@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Since these two functions are nearly identical (with qemuSetupHostdevCgroup actually calling virCgroupAllowDevicePath) we can have one function call the other and thus de-duplicate some code. Signed-off-by: Michal Privoznik <<a href="mailto:mprivozn@redhat.com" class="gmail_msg" target="_blank">mprivozn@redhat.com</a>> </blockquote> Reviewed-by: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com">marcandre.lureau@redhat.com</a>> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- src/qemu/qemu_cgroup.c | 147 +++++-------------------------------------------- src/qemu/qemu_domain.c | 31 +++++++++-- src/qemu/qemu_domain.h | 4 ++ 3 files changed, 43 insertions(+), 139 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 89854b5bd..19832c209 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -264,147 +264,26 @@ int qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) { - int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; - virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci; - virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; - virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host; - virPCIDevicePtr pci = NULL; - virUSBDevicePtr usb = NULL; - virSCSIDevicePtr scsi = NULL; - virSCSIVHostDevicePtr host = NULL; char *path = NULL; - int rv; + int perms; + int ret = -1; - /* currently this only does something for PCI devices using vfio - * for device assignment, but it is called for *all* hostdev - * devices. - */ + if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0) + goto cleanup; - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; - - if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { - - switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - pci = virPCIDeviceNew(pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); - if (!pci) - goto cleanup; - - if (!(path = virPCIDeviceGetIOMMUGroupDev(pci))) - goto cleanup; - - VIR_DEBUG("Cgroup allow %s for PCI device assignment", path); - rv = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW, false); - virDomainAuditCgroupPath(vm, priv->cgroup, - "allow", path, "rw", rv == 0); - if (rv < 0) - goto cleanup; - } - break; - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - /* NB: hostdev->missing wasn't previously checked in the - * case of hotplug, only when starting a domain. Now it is - * always checked, and the cgroup setup skipped if true. - */ - if (dev->missing) - break; - if ((usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device, - NULL)) == NULL) { - goto cleanup; - } - - if (VIR_STRDUP(path, virUSBDeviceGetPath(usb)) < 0) - goto cleanup; - - VIR_DEBUG("Process path '%s' for USB device", path); - rv = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW, false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv == 0); - if (rv < 0) - goto cleanup; - break; - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { - if (scsisrc->protocol == - VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; - /* Follow qemuSetupDiskCgroup() and qemuSetImageCgroupInternal() - * which does nothing for non local storage - */ - VIR_DEBUG("Not updating cgroups for hostdev iSCSI path '%s'", - iscsisrc->path); - } else { - virDomainHostdevSubsysSCSIHostPtr scsihostsrc = - &scsisrc->u.host; - if ((scsi = virSCSIDeviceNew(NULL, - scsihostsrc->adapter, - scsihostsrc->bus, - scsihostsrc->target, - scsihostsrc->unit, - dev->readonly, - dev->shareable)) == NULL) - goto cleanup; - - if (VIR_STRDUP(path, virSCSIDeviceGetPath(scsi)) < 0) - goto cleanup; - - VIR_DEBUG("Process path '%s' for SCSI device", path); - rv = virCgroupAllowDevicePath(priv->cgroup, path, - virSCSIDeviceGetReadonly(scsi) ? - VIR_CGROUP_DEVICE_READ : - VIR_CGROUP_DEVICE_RW, false); - - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - virSCSIDeviceGetReadonly(scsi) ? "r" : "rw", - rv == 0); - if (rv < 0) - goto cleanup; - } - break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: { - if (hostsrc->protocol == - VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) { - if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn))) - goto cleanup; - - if (VIR_STRDUP(path, virSCSIVHostDeviceGetPath(host)) < 0) - goto cleanup; - - VIR_DEBUG("Process path '%s' for scsi_host device", path); - - rv = virCgroupAllowDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RW, false); - - virDomainAuditCgroupPath(vm, priv->cgroup, - "allow", path, "rw", rv == 0); - if (rv < 0) - goto cleanup; - } - break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: - break; - } + if (!path) { + /* There's no path that we need to allow. Claim success. */ + ret = 0; + goto cleanup; } - ret = 0; + VIR_DEBUG("Cgroup allow %s perms=%d", path, perms); + ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, false); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, + virCgroupGetDevicePermsString(perms), ret == 0); + cleanup: - virPCIDeviceFree(pci); - virUSBDeviceFree(usb); - virSCSIDeviceFree(scsi); - virSCSIVHostDeviceFree(host); VIR_FREE(path); return ret; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7c696963e..c6d32525f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6831,9 +6831,21 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, } -static int +/** + * qemuDomainGetHostdevPath: + * @dev: host device definition + * @path: resulting path to @dev + * @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms + * + * For given device @dev fetch its host path and store it at @path. Optionally, + * caller can get @perms on the path (e.g. rw/ro). + * + * Returns 0 on success, -1 otherwise. + */ +int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, - char **path) + char **path, + int *perms) { int ret = -1; virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; @@ -6864,6 +6876,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci))) goto cleanup; freeTmpPath = true; + if (perms) + *perms = VIR_CGROUP_DEVICE_RW; } break; @@ -6878,6 +6892,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, if (!(tmpPath = (char *) virUSBDeviceGetPath(usb))) goto cleanup; + if (perms) + *perms = VIR_CGROUP_DEVICE_RW; break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: @@ -6902,6 +6918,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, if (!(tmpPath = (char *) virSCSIDeviceGetPath(scsi))) goto cleanup; + if (perms) + *perms = virSCSIDeviceGetReadonly(scsi) ? + VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW; </blockquote><div> </div><div>missing a space after : </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> } break; @@ -6913,6 +6932,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host))) goto cleanup; + if (perms) + *perms = VIR_CGROUP_DEVICE_RW; } break; } @@ -7328,7 +7349,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, int ret = -1; char *path = NULL; - if (qemuDomainGetHostdevPath(dev, &path) < 0) + if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) goto cleanup; if (!path) { @@ -7964,7 +7985,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver, if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; - if (qemuDomainGetHostdevPath(hostdev, &path) < 0) + if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) goto cleanup; if (!path) { @@ -7995,7 +8016,7 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver, if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; - if (qemuDomainGetHostdevPath(hostdev, &path) < 0) + if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) goto cleanup; if (!path) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 5cfa3e114..f81550e2f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -802,6 +802,10 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps); +int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, + char **path, + int *perms); + int qemuDomainBuildNamespace(virQEMUDriverPtr driver, virDomainObjPtr vm); -- </blockquote> otherwise, Reviewed-by: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com">marcandre.lureau@redhat.com</a>> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2.11.0 -- libvir-list mailing list <a href="mailto:libvir-list@redhat.com" class="gmail_msg" target="_blank">libvir-list@redhat.com</a> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" rel="noreferrer" class="gmail_msg" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a> </blockquote></div></div></div><div dir="ltr">-- </div><div data-smartmail="gmail_signature"><div dir="ltr">Marc-André Lureau </div></div>