Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mpriv...@redhat.com> wrote:
> 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 <mpriv...@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> --- > 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; > missing a space after : > } > 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); > > -- > otherwise, Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> 2.11.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- Marc-André Lureau
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list