[libvirt] [PATCH 05/12] Split out qemuDomainEnsureVirtioAddress

John Ferlan jferlan at redhat.com
Thu Oct 19 12:09:06 UTC 2017



On 10/17/2017 11:04 AM, Ján Tomko wrote:
> Split out the common code responsible for reserving/assigning
> PCI/CCW addresses for virtio disks into a helper function
> for reuse by other virtio devices.
> ---
>  src/qemu/qemu_domain_address.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain_address.h |  4 ++++
>  src/qemu/qemu_hotplug.c        | 29 +++------------------------
>  3 files changed, 52 insertions(+), 26 deletions(-)
> 

Not an issue - just a note from review...

> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index c4485d4ab..ebe9eb861 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -2904,3 +2904,48 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
>          VIR_WARN("Unable to release USB address on %s",
>                   NULLSTR(devstr));
>  }
> +
> +
> +int
> +qemuDomainEnsureVirtioAddress(bool *releaseAddr,
> +                              virDomainObjPtr vm,
> +                              virDomainDeviceDefPtr dev,
> +                              const char *devicename)
> +{
> +    virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainCCWAddressSetPtr ccwaddrs = NULL;
> +    virQEMUDriverPtr driver = priv->driver;
> +    int ret = -1;
> +
> +    if (!info->type) {
> +        if (qemuDomainIsS390CCW(vm->def) &&
> +            virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
> +            info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> +        else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
> +            info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
> +    } else {
> +        if (!qemuCheckCCWS390AddressSupport(vm->def, *info, priv->qemuCaps,
> +                                            devicename))
> +            return -1;
> +    }
> +
> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> +        if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
> +            goto cleanup;
> +        if (virDomainCCWAddressAssign(info, ccwaddrs,
> +                                      !info->addr.ccw.assigned) < 0)
> +            goto cleanup;
> +    } else if (!info->type ||
> +               info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +        if (qemuDomainEnsurePCIAddress(vm, dev, driver) < 0)
> +            goto cleanup;
> +        *releaseAddr = true;

This is only setting *releaseAddr in this instance; whereas, the
previous code would also set it for info->type CCW [1]

Still looking at how @releaseaddr is used, we see that it's used to call
qemuDomainReleaseDeviceAddress which only cares about PCI and USB, so I
suppose this is fine - just different.

> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virDomainCCWAddressSetFree(ccwaddrs);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
> index b5644fa9c..e951a4c88 100644
> --- a/src/qemu/qemu_domain_address.h
> +++ b/src/qemu/qemu_domain_address.h
> @@ -59,6 +59,10 @@ qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
>  int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def,
>                                       virDomainMemoryDefPtr mem);
>  
> +int qemuDomainEnsureVirtioAddress(bool *releaseAddr,
> +                                  virDomainObjPtr vm,
> +                                  virDomainDeviceDefPtr dev,
> +                                  const char *devicename);
>  
>  # define __QEMU_DOMAIN_ADDRESS_H__
>  
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1e7931a84..177c8a01d 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -363,7 +363,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>      bool driveAdded = false;
>      bool secobjAdded = false;
>      bool encobjAdded = false;
> -    virDomainCCWAddressSetPtr ccwaddrs = NULL;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      const char *src = virDomainDiskGetSource(disk);
>      virJSONValuePtr secobjProps = NULL;
> @@ -372,33 +371,12 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>      qemuDomainSecretInfoPtr secinfo;
>      qemuDomainSecretInfoPtr encinfo;
>  
> -    if (!disk->info.type) {
> -        if (qemuDomainIsS390CCW(vm->def) &&
> -            virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
> -            disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> -        else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
> -            disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
> -    } else {
> -        if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps,
> -                                            disk->dst))
> -            goto cleanup;
> -    }
> -
>      if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
>          goto cleanup;
>  
> -    if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> -        if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
> -            goto error;
> -        if (virDomainCCWAddressAssign(&disk->info, ccwaddrs,
> -                                      !disk->info.addr.ccw.assigned) < 0)
> -            goto error;
> -    } else if (!disk->info.type ||
> -                disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> -        if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0)
> -            goto error;
> -    }
> -    releaseaddr = true;

[1] @releaseaddr is set for both CCW and PCI here.

> +    if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, disk->dst) < 0)
> +        goto error;
> +
>      if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
>          goto error;
>  
> @@ -477,7 +455,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>      virJSONValueFree(secobjProps);
>      virJSONValueFree(encobjProps);
>      qemuDomainSecretDiskDestroy(disk);
> -    virDomainCCWAddressSetFree(ccwaddrs);
>      VIR_FREE(devstr);
>      VIR_FREE(drivestr);
>      VIR_FREE(drivealias);
> 




More information about the libvir-list mailing list