Re: [libvirt] [PATCH 4/7] qemuSetupHostdevCgroup: Use qemuDomainGetHostdevPath

[Marc-André Lureau]

Hi

On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik 
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 
>


Reviewed-by: Marc-André Lureau 


---
>  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 = >source.subsys.u.usb;
> -virDomainHostdevSubsysPCIPtr pcisrc = >source.subsys.u.pci;
> -virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
> -virDomainHostdevSubsysSCSIVHostPtr hostsrc =
> >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, , ) < 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 =
> >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 =
> ->u.host;
> -if ((scsi = virSCSIDeviceNew(NULL,
> - scsihostsrc->adapter,
> - scsihostsrc->bus,
> - scsihostsrc->target,
> -  

[libvirt] [PATCH 4/7] qemuSetupHostdevCgroup: Use qemuDomainGetHostdevPath

[Michal Privoznik]

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 
---
 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 = >source.subsys.u.usb;
-virDomainHostdevSubsysPCIPtr pcisrc = >source.subsys.u.pci;
-virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
-virDomainHostdevSubsysSCSIVHostPtr hostsrc = 
>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, , ) < 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 = 
>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 =
->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