[libvirt] [PATCH v2 00/10] Remove caching of address sets
From: Tomasz Flendrich <Darge@users.noreply.github.com> These patches delete the caching of pci, virtioSerial and ccw address sets. I am deleting them, because they can be recalculated from the domain definition, and there's no point in keeping redundant data, especially because handling a persistent cache of addresses required using functions that released addresses. These functions aren't useful anymore, so they are dropped too. I know that USB addresses were added and that they are cached now. I am sure that they can be calculated on demand too though. If this gets accepted, I will rebase the other patches that depend on this one, making functions less dependant on qemu, and removing the code duplication. Changes in v2: * I rebased it, because there were some changes that added a usb address set. * virDomainReleaseDeviceAddress is no longer deleted, because it's used for usb addresses. It also means that in qemu_hotplug.c, this function's calls are not deleted anymore. * In qemu_hotplug.c, in one place, virDomainPCIAddressSetFree() was previously called after the "error:" label. I moved it to the "cleanup:", so the address set is freed even if an error occurs. * Add one missing free of pci address set, in qemuDomainAttachNetDevice(). Tomasz Flendrich (10): add virDomainVirtioSerialAddrSetCreateFromDomain qemu_hotplug: generate vioserial address list on demand qemu: remove vioserialaddrs caching Add qemuDomainCCWAddrSetCreateFromDomain qemu_hotplug: generate ccw address list on demand qemu: remove ccwaddrs caching add qemuDomainPCIAddrSetCreateFromDomain qemu_hotplug: generate pci address list on demand qemu: remove pciaddrs caching Remove unused functions that release addresses src/conf/domain_addr.c | 131 +++++++++--------------------------- src/conf/domain_addr.h | 19 +----- src/libvirt_private.syms | 4 +- src/qemu/qemu_domain.c | 3 - src/qemu/qemu_domain.h | 3 - src/qemu/qemu_domain_address.c | 147 ++++++++++++++++++++--------------------- src/qemu/qemu_domain_address.h | 9 +++ src/qemu/qemu_hotplug.c | 111 ++++++++++++++++++++++++------- 8 files changed, 201 insertions(+), 226 deletions(-) -- 2.7.4 (Apple Git-66)
The address sets (pci, ccw, virtio serial) are currently cached in qemu private data, but all the information required to recreate these sets is in the domain definition. Therefore I am removing the redundant data and adding a way to recalculate these sets. Add a function that calculates the virtio serial address set from the domain definition. Credit goes to Cole Robinson. --- src/conf/domain_addr.c | 31 +++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 9 +-------- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index c3469ee..e09b409 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -975,6 +975,37 @@ virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) } } + +/* virDomainVirtioSerialAddrSetCreateFromDomain ++ * ++ * @def: Domain def to introspect ++ * ++ * Inspect the domain definition and return an address set containing ++ * every virtio serial address we find ++ */ +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def) +{ + virDomainVirtioSerialAddrSetPtr addrs; + virDomainVirtioSerialAddrSetPtr ret = NULL; + + if (!(addrs = virDomainVirtioSerialAddrSetCreate())) + goto cleanup; + + if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve, + addrs) < 0) + goto cleanup; + + ret = addrs; + cleanup: + if (!ret) + virDomainVirtioSerialAddrSetFree(addrs); + return ret; +} + static int virDomainVirtioSerialAddrSetAutoaddController(virDomainDefPtr def, virDomainVirtioSerialAddrSetPtr addrs, diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index ce94981..4584e0a 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -208,6 +208,9 @@ virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs); +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def) + ATTRIBUTE_NONNULL(1); bool virDomainVirtioSerialAddrIsComplete(virDomainDeviceInfoPtr info); int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9396c4e..6b54812 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -126,6 +126,7 @@ virDomainVirtioSerialAddrRelease; virDomainVirtioSerialAddrReserve; virDomainVirtioSerialAddrSetAddControllers; virDomainVirtioSerialAddrSetCreate; +virDomainVirtioSerialAddrSetCreateFromDomain; virDomainVirtioSerialAddrSetFree; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 787b357..2bd095f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -114,14 +114,7 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def, virDomainVirtioSerialAddrSetPtr addrs = NULL; qemuDomainObjPrivatePtr priv = NULL; - if (!(addrs = virDomainVirtioSerialAddrSetCreate())) - goto cleanup; - - if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0) - goto cleanup; - - if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve, - addrs) < 0) + if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) goto cleanup; VIR_DEBUG("Finished reserving existing ports"); -- 2.7.4 (Apple Git-66)
On Sat, Jul 23, 2016 at 03:47:06AM +0200, Tomasz Flendrich wrote:
The address sets (pci, ccw, virtio serial) are currently cached in qemu private data, but all the information required to recreate these sets is in the domain definition. Therefore I am removing the redundant data and adding a way to recalculate these sets.
Add a function that calculates the virtio serial address set from the domain definition.
Credit goes to Cole Robinson. --- src/conf/domain_addr.c | 31 +++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 9 +-------- 4 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index c3469ee..e09b409 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -975,6 +975,37 @@ virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) } }
+ +/* virDomainVirtioSerialAddrSetCreateFromDomain ++ * ++ * @def: Domain def to introspect ++ * ++ * Inspect the domain definition and return an address set containing ++ * every virtio serial address we find ++ */ +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def) +{ + virDomainVirtioSerialAddrSetPtr addrs;
One common thing to do in libvirt's codebase is (although of course there are too many ways to approach this): Initialize this to NULL,
+ virDomainVirtioSerialAddrSetPtr ret = NULL; + + if (!(addrs = virDomainVirtioSerialAddrSetCreate())) + goto cleanup; + + if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve, + addrs) < 0) + goto cleanup; + + ret = addrs;
set it to NULL here as well,
+ cleanup: + if (!ret) + virDomainVirtioSerialAddrSetFree(addrs);
and call this unconditionally. ACK with that changed.
Dropping the caching of virtio serial address set. Instead of using the cached address set, a function in qemu_hotplug.c now recalculates it on demand. Credit goes to Cole Robinson. --- src/qemu/qemu_hotplug.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4e4bf82..5c82361 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1551,21 +1551,28 @@ qemuDomainChrRemove(virDomainDefPtr vmdef, } static int -qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv, +qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, + qemuDomainObjPrivatePtr priv, virDomainChrDefPtr chr) { + int ret = -1; + virDomainVirtioSerialAddrSetPtr vioaddrs = NULL; + + if (!(vioaddrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) + goto cleanup; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) { - if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, + if (virDomainVirtioSerialAddrAutoAssign(NULL, vioaddrs, &chr->info, true) < 0) - return -1; - return 1; + goto cleanup; + ret = 1; } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) - return -1; - return 1; + goto cleanup; + ret = 1; } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) { @@ -1575,20 +1582,27 @@ qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv, } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) { - if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, + if (virDomainVirtioSerialAddrAutoAssign(NULL, vioaddrs, &chr->info, false) < 0) - return -1; - return 1; + goto cleanup; + ret = 1; } + if (ret == 1) + goto cleanup; + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unsupported address type for character device")); - return -1; + goto cleanup; } - return 0; + ret = 0; + + cleanup: + virDomainVirtioSerialAddrSetFree(vioaddrs); + return ret; } int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, @@ -1611,7 +1625,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; - if ((rc = qemuDomainAttachChrDeviceAssignAddr(priv, chr)) < 0) + if ((rc = qemuDomainAttachChrDeviceAssignAddr(vm->def, priv, chr)) < 0) goto cleanup; if (rc == 1) need_release = true; -- 2.7.4 (Apple Git-66)
On Sat, Jul 23, 2016 at 03:47:07AM +0200, Tomasz Flendrich wrote:
Dropping the caching of virtio serial address set. Instead of using the cached address set, a function in qemu_hotplug.c now recalculates it on demand.
Credit goes to Cole Robinson. --- src/qemu/qemu_hotplug.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4e4bf82..5c82361 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1551,21 +1551,28 @@ qemuDomainChrRemove(virDomainDefPtr vmdef, }
static int -qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv, +qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, + qemuDomainObjPrivatePtr priv, virDomainChrDefPtr chr) { + int ret = -1; + virDomainVirtioSerialAddrSetPtr vioaddrs = NULL; + + if (!(vioaddrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) + goto cleanup; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) { - if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, + if (virDomainVirtioSerialAddrAutoAssign(NULL, vioaddrs, &chr->info, true) < 0) - return -1; - return 1; + goto cleanup; + ret = 1;
} else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) - return -1; - return 1; + goto cleanup; + ret = 1;
} else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
You're missing one goto cleanup/ret = 1 here between these hunks. ACK with that changed.
Dropping the caching of virtio serial address set. The cached set is not required anymore, because the set is now being recalculated from the domain definition on demand, so the cache can be deleted. Credit goes to Cole Robinson. --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 18 +++--------------- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ceac22f..4aa9b51 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1329,7 +1329,6 @@ qemuDomainObjPrivateFree(void *data) virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); virDomainCCWAddressSetFree(priv->ccwaddrs); - virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); virDomainUSBAddressSetFree(priv->usbaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d10ebcc..eefd8d4 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -186,7 +186,6 @@ struct _qemuDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; virDomainCCWAddressSetPtr ccwaddrs; - virDomainVirtioSerialAddrSetPtr vioserialaddrs; virDomainUSBAddressSetPtr usbaddrs; virQEMUCapsPtr qemuCaps; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2bd095f..0df112b 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -106,13 +106,11 @@ qemuDomainSetSCSIControllerModel(const virDomainDef *def, static int -qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def, - virDomainObjPtr obj) +qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def) { int ret = -1; size_t i; virDomainVirtioSerialAddrSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) goto cleanup; @@ -137,13 +135,6 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def, goto cleanup; } - if (obj && obj->privateData) { - priv = obj->privateData; - /* if this is the live domain object, we persist the addresses */ - virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); - priv->vioserialaddrs = addrs; - addrs = NULL; - } ret = 0; cleanup: @@ -1767,7 +1758,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def, virDomainObjPtr obj, bool newDomain) { - if (qemuDomainAssignVirtioSerialAddresses(def, obj) < 0) + if (qemuDomainAssignVirtioSerialAddresses(def) < 0) return -1; if (qemuDomainAssignSpaprVIOAddresses(def, qemuCaps) < 0) @@ -1809,10 +1800,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, &info->addr.pci) < 0) VIR_WARN("Unable to release PCI address on %s", NULLSTR(devstr)); - if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && - virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, info) < 0) - VIR_WARN("Unable to release virtio-serial address on %s", - NULLSTR(devstr)); + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && priv->usbaddrs && virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) -- 2.7.4 (Apple Git-66)
On Sat, Jul 23, 2016 at 03:47:08AM +0200, Tomasz Flendrich wrote:
Dropping the caching of virtio serial address set. The cached set is not required anymore, because the set is now being recalculated from the domain definition on demand, so the cache can be deleted.
This patch shows how useless the keeping of that set was so neatly :) ACK
The address sets (pci, ccw, virtio serial) are currently cached in qemu private data, but all the information required to recreate these sets is in the domain definition. Therefore I am removing the redundant data and adding a way to recalculate these sets. Add a function that calculates the ccw address set from the domain definition. --- src/qemu/qemu_domain_address.c | 31 +++++++++++++++++++++++-------- src/qemu/qemu_domain_address.h | 4 ++++ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 0df112b..b23790f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -320,6 +320,28 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, } } +virDomainCCWAddressSetPtr +qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def) +{ + virDomainCCWAddressSetPtr addrs = NULL; + + if (!(addrs = virDomainCCWAddressSetCreate())) + goto error; + + if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate, + addrs) < 0) + goto error; + + if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate, + addrs) < 0) + goto error; + + return addrs; + + error: + virDomainCCWAddressSetFree(addrs); + return NULL; +} /* * Three steps populating CCW devnos @@ -341,16 +363,9 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, qemuDomainPrimeVirtioDeviceAddresses( def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); - if (!(addrs = virDomainCCWAddressSetCreate())) - goto cleanup; - - if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate, - addrs) < 0) + if (!(addrs = qemuDomainCCWAddrSetCreateFromDomain(def))) goto cleanup; - if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate, - addrs) < 0) - goto cleanup; } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { /* deal with legacy virtio-s390 */ qemuDomainPrimeVirtioDeviceAddresses( diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index ee326d7..11d6e92 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -41,6 +41,10 @@ void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, virDomainDeviceInfoPtr info, const char *devstr); +virDomainCCWAddressSetPtr +qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def) + ATTRIBUTE_NONNULL(1); + # define __QEMU_DOMAIN_ADDRESS_H__ #endif /* __QEMU_DOMAIN_ADDRESS_H__ */ -- 2.7.4 (Apple Git-66)
Dropping the caching of ccw address set. Instead of using the cached address set, functions in qemu_hotplug.c now recalculate it on demand. --- src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5c82361..3deeb0b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -313,6 +313,7 @@ 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; @@ -337,7 +338,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto cleanup; if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&disk->info, priv->ccwaddrs, + 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 || @@ -416,6 +419,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virJSONValueFree(secobjProps); virJSONValueFree(encobjProps); qemuDomainSecretDiskDestroy(disk); + virDomainCCWAddressSetFree(ccwaddrs); VIR_FREE(devstr); VIR_FREE(drivestr); VIR_FREE(drivealias); @@ -459,6 +463,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, const char* type = virDomainControllerTypeToString(controller->type); char *devstr = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainCCWAddressSetPtr ccwaddrs = NULL; bool releaseaddr = false; if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { @@ -500,7 +505,9 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) goto cleanup; } else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&controller->info, priv->ccwaddrs, + if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) + goto cleanup; + if (virDomainCCWAddressAssign(&controller->info, ccwaddrs, !controller->info.addr.ccw.assigned) < 0) goto cleanup; } @@ -533,6 +540,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); VIR_FREE(devstr); + virDomainCCWAddressSetFree(ccwaddrs); return ret; } @@ -901,6 +909,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, int actualType; virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainCCWAddressSetPtr ccwaddrs = NULL; size_t i; /* preallocate new slot for device */ @@ -1038,7 +1047,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (qemuDomainMachineIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; - if (virDomainCCWAddressAssign(&net->info, priv->ccwaddrs, + if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) + goto cleanup; + if (virDomainCCWAddressAssign(&net->info, ccwaddrs, !net->info.addr.ccw.assigned) < 0) goto cleanup; } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) { @@ -1212,6 +1223,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, VIR_FREE(vhostfd); VIR_FREE(vhostfdName); virObjectUnref(cfg); + virDomainCCWAddressSetFree(ccwaddrs); return ret; @@ -1692,6 +1704,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, bool chardevAdded = false; bool objAdded = false; virJSONValuePtr props = NULL; + virDomainCCWAddressSetPtr ccwaddrs = NULL; const char *type; int ret = -1; int rv; @@ -1722,9 +1735,11 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &rng->info) < 0) return -1; } else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&rng->info, priv->ccwaddrs, + if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) + goto cleanup; + if (virDomainCCWAddressAssign(&rng->info, ccwaddrs, !rng->info.addr.ccw.assigned) < 0) - return -1; + goto cleanup; } /* build required metadata */ @@ -1775,6 +1790,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, VIR_FREE(charAlias); VIR_FREE(objAlias); VIR_FREE(devstr); + virDomainCCWAddressSetFree(ccwaddrs); return ret; exit_monitor: -- 2.7.4 (Apple Git-66)
On Sat, Jul 23, 2016 at 03:47:10AM +0200, Tomasz Flendrich wrote:
Dropping the caching of ccw address set. Instead of using the cached address set, functions in qemu_hotplug.c now recalculate it on demand. --- src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5c82361..3deeb0b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -337,7 +338,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto cleanup;
if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&disk->info, priv->ccwaddrs, + if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) + goto error; + if (virDomainCCWAddressAssign(&disk->info, ccwaddrs, !disk->info.addr.ccw.assigned) < 0)
Ideally, we would create a wrapper for these two functions but that's just a nit for the future. Also just a guess, maybe it'll turn out that we won't need it at all... ACK
Dropping the caching of ccw address set. The cached set is not required anymore, because the set is now being recalculated from the domain definition on demand, so the cache can be deleted. --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 23 +++-------------------- 3 files changed, 3 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4aa9b51..9045f37 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1328,7 +1328,6 @@ qemuDomainObjPrivateFree(void *data) virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); - virDomainCCWAddressSetFree(priv->ccwaddrs); virDomainUSBAddressSetFree(priv->usbaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index eefd8d4..6f349a1 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -185,7 +185,6 @@ struct _qemuDomainObjPrivate { char *pidfile; virDomainPCIAddressSetPtr pciaddrs; - virDomainCCWAddressSetPtr ccwaddrs; virDomainUSBAddressSetPtr usbaddrs; virQEMUCapsPtr qemuCaps; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b23790f..66ab70e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -351,12 +351,10 @@ qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def) */ static int qemuDomainAssignS390Addresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) + virQEMUCapsPtr qemuCaps) { int ret = -1; virDomainCCWAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; if (qemuDomainMachineIsS390CCW(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { @@ -372,15 +370,6 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); } - if (obj && obj->privateData) { - priv = obj->privateData; - if (addrs) { - /* if this is the live domain object, we persist the CCW addresses*/ - virDomainCCWAddressSetFree(priv->ccwaddrs); - priv->ccwaddrs = addrs; - addrs = NULL; - } - } ret = 0; cleanup: @@ -1779,7 +1768,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def, if (qemuDomainAssignSpaprVIOAddresses(def, qemuCaps) < 0) return -1; - if (qemuDomainAssignS390Addresses(def, qemuCaps, obj) < 0) + if (qemuDomainAssignS390Addresses(def, qemuCaps) < 0) return -1; qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps); @@ -1804,13 +1793,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (!devstr) devstr = info->alias; - if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - qemuDomainMachineIsS390CCW(vm->def) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW) && - virDomainCCWAddressReleaseAddr(priv->ccwaddrs, info) < 0) - VIR_WARN("Unable to release CCW address on %s", - NULLSTR(devstr)); - else if (virDeviceInfoPCIAddressPresent(info) && + if (virDeviceInfoPCIAddressPresent(info) && virDomainPCIAddressReleaseSlot(priv->pciaddrs, &info->addr.pci) < 0) VIR_WARN("Unable to release PCI address on %s", -- 2.7.4 (Apple Git-66)
On Sat, Jul 23, 2016 at 03:47:11AM +0200, Tomasz Flendrich wrote:
Dropping the caching of ccw address set. The cached set is not required anymore, because the set is now being recalculated from the domain definition on demand, so the cache can be deleted. --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 23 +++-------------------- 3 files changed, 3 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b23790f..66ab70e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1804,13 +1793,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (!devstr) devstr = info->alias;
- if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - qemuDomainMachineIsS390CCW(vm->def) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW) && - virDomainCCWAddressReleaseAddr(priv->ccwaddrs, info) < 0) - VIR_WARN("Unable to release CCW address on %s", - NULLSTR(devstr)); - else if (virDeviceInfoPCIAddressPresent(info) && + if (virDeviceInfoPCIAddressPresent(info) && virDomainPCIAddressReleaseSlot(priv->pciaddrs, &info->addr.pci) < 0)
Indentation's off.
The address sets (pci, ccw, virtio serial) are currently cached in qemu private data, but all the information required to recreate these sets is in the domain definition. Therefore I am removing the redundant data and adding a way to recalculate these sets. Add a function that calculates the pci address set from the domain definition. The part of pci address assignment that is not a dryRun is separated from qemuDomainAssignPCIAddresses into a new function: qemuDomainPCIAddrSetCreateFromDomain. The first time this function is run, it can allocate some new addresses. After all the pci addresses are assigned, on subsequent runs to this function, it just recreates the pci address set. --- src/qemu/qemu_domain_address.c | 48 +++++++++++++++++++++++++++++++++++------- src/qemu/qemu_domain_address.h | 5 +++++ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 66ab70e..add6fb5 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1424,6 +1424,45 @@ qemuDomainAddressFindNewBusNr(virDomainDefPtr def) return lowestBusNr - 2; } +virDomainPCIAddressSetPtr +qemuDomainPCIAddrSetCreateFromDomain(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + virDomainPCIAddressSetPtr addrs = NULL; + int max_idx = -1; + int nbuses = 0; + virDomainPCIAddressSetPtr ret = NULL; + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if ((int) def->controllers[i]->idx > max_idx) + max_idx = def->controllers[i]->idx; + } + } + + nbuses = max_idx + 1; + + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) + goto cleanup; + + if (qemuDomainSupportsPCI(def, qemuCaps)) { + if (qemuDomainValidateDevicePCISlotsChipsets(def, qemuCaps, + addrs) < 0) + goto cleanup; + + if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) + goto cleanup; + } + + ret = addrs; + addrs = NULL; + + cleanup: + virDomainPCIAddressSetFree(addrs); + + return ret; +} static int qemuDomainAssignPCIAddresses(virDomainDefPtr def, @@ -1506,17 +1545,10 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup; } - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) + if (!(addrs = qemuDomainPCIAddrSetCreateFromDomain(def, qemuCaps))) goto cleanup; if (qemuDomainSupportsPCI(def, qemuCaps)) { - if (qemuDomainValidateDevicePCISlotsChipsets(def, qemuCaps, - addrs) < 0) - goto cleanup; - - if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) - goto cleanup; - for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index 11d6e92..546c57f 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -45,6 +45,11 @@ virDomainCCWAddressSetPtr qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); +virDomainPCIAddressSetPtr +qemuDomainPCIAddrSetCreateFromDomain(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + # define __QEMU_DOMAIN_ADDRESS_H__ #endif /* __QEMU_DOMAIN_ADDRESS_H__ */ -- 2.7.4 (Apple Git-66)
On Sat, Jul 23, 2016 at 03:47:12AM +0200, Tomasz Flendrich wrote:
The address sets (pci, ccw, virtio serial) are currently cached in qemu private data, but all the information required to recreate these sets is in the domain definition. Therefore I am removing the redundant data and adding a way to recalculate these sets.
Add a function that calculates the pci address set from the domain definition. The part of pci address assignment that is not a dryRun is separated from qemuDomainAssignPCIAddresses into a new function: qemuDomainPCIAddrSetCreateFromDomain. The first time this function is run, it can allocate some new addresses. After all the pci addresses are assigned, on subsequent runs to this function, it just recreates the pci address set. --- src/qemu/qemu_domain_address.c | 48 +++++++++++++++++++++++++++++++++++------- src/qemu/qemu_domain_address.h | 5 +++++ 2 files changed, 45 insertions(+), 8 deletions(-)
The patch, technically, is not flawed. However the naming got me confused a lot. That's because even though the function is named AddrSetCreate, it is actually the one that calls qemuDomainAssignDevicePCISlots(). I think we should split the calling function a bit more and have part of it named '...Prepare...' and then the rest. Also, even though it is still now totally broken, I'm somehow afraid we'll be missing some of the preparation stuff at some point. Although there is no possibility of such case currently. I'll continue with the review tomorrow, all previous patches are fine for now. Have a nice day, Martin
On Sat, Jul 23, 2016 at 03:47:12AM +0200, Tomasz Flendrich wrote:
The address sets (pci, ccw, virtio serial) are currently cached in qemu private data, but all the information required to recreate these sets is in the domain definition. Therefore I am removing the redundant data and adding a way to recalculate these sets.
Add a function that calculates the pci address set from the domain definition. The part of pci address assignment that is not a dryRun is separated from qemuDomainAssignPCIAddresses into a new function: qemuDomainPCIAddrSetCreateFromDomain. The first time this function is run, it can allocate some new addresses. After all the pci addresses are assigned, on subsequent runs to this function, it just recreates the pci address set. --- src/qemu/qemu_domain_address.c | 48 +++++++++++++++++++++++++++++++++++------- src/qemu/qemu_domain_address.h | 5 +++++ 2 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 66ab70e..add6fb5 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1424,6 +1424,45 @@ qemuDomainAddressFindNewBusNr(virDomainDefPtr def) return lowestBusNr - 2; }
+virDomainPCIAddressSetPtr +qemuDomainPCIAddrSetCreateFromDomain(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + virDomainPCIAddressSetPtr addrs = NULL; + int max_idx = -1; + int nbuses = 0; + virDomainPCIAddressSetPtr ret = NULL; + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if ((int) def->controllers[i]->idx > max_idx) + max_idx = def->controllers[i]->idx; + } + } + + nbuses = max_idx + 1; + + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) + goto cleanup; +
There actually is a call that does what we need and it's this one ^ We just need the number for buses (for some reason), which we can easily eliminate in that function itself. All the information that the functions below do is only needed when creating a new domain object as all the information is saved there. So we don't need to call these, neither have the calculation for the number of buses outside of that function. NACK to this approach, just fix qemuDomainPCIAddressSetCreate() itself and use that in the later patch.
Dropping the caching of pci address set. Instead of using the cached address set, functions in qemu_hotplug.c now recalculate it on demand. --- src/qemu/qemu_domain_address.c | 6 ------ src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++------- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index add6fb5..2191009 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1825,12 +1825,6 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (!devstr) devstr = info->alias; - if (virDeviceInfoPCIAddressPresent(info) && - virDomainPCIAddressReleaseSlot(priv->pciaddrs, - &info->addr.pci) < 0) - VIR_WARN("Unable to release PCI address on %s", - NULLSTR(devstr)); - if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && priv->usbaddrs && virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3deeb0b..aafd937 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -314,6 +314,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, bool secobjAdded = false; bool encobjAdded = false; virDomainCCWAddressSetPtr ccwaddrs = NULL; + virDomainPCIAddressSetPtr pciaddrs = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; @@ -345,7 +346,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; } else if (!disk->info.type || disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &disk->info) < 0) + if (!(pciaddrs = qemuDomainPCIAddrSetCreateFromDomain(vm->def, + priv->qemuCaps))) + goto error; + if (virDomainPCIAddressEnsureAddr(pciaddrs, &disk->info) < 0) goto error; } releaseaddr = true; @@ -420,6 +424,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virJSONValueFree(encobjProps); qemuDomainSecretDiskDestroy(disk); virDomainCCWAddressSetFree(ccwaddrs); + virDomainPCIAddressSetFree(pciaddrs); VIR_FREE(devstr); VIR_FREE(drivestr); VIR_FREE(drivealias); @@ -464,6 +469,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, char *devstr = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainCCWAddressSetPtr ccwaddrs = NULL; + virDomainPCIAddressSetPtr pciaddrs = NULL; bool releaseaddr = false; if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { @@ -502,7 +508,10 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) + if (!(pciaddrs = qemuDomainPCIAddrSetCreateFromDomain(vm->def, + priv->qemuCaps))) + goto cleanup; + if (virDomainPCIAddressEnsureAddr(pciaddrs, &controller->info) < 0) goto cleanup; } else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) @@ -541,6 +550,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, VIR_FREE(devstr); virDomainCCWAddressSetFree(ccwaddrs); + virDomainPCIAddressSetFree(pciaddrs); return ret; } @@ -910,6 +920,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; + virDomainPCIAddressSetPtr pciaddrs = NULL; size_t i; /* preallocate new slot for device */ @@ -1056,8 +1067,12 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("virtio-s390 net device cannot be hotplugged.")); goto cleanup; - } else if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &net->info) < 0) { - goto cleanup; + } else { + if (!(pciaddrs = qemuDomainPCIAddrSetCreateFromDomain(vm->def, + priv->qemuCaps))) + goto cleanup; + if (virDomainPCIAddressEnsureAddr(pciaddrs, &net->info) < 0) + goto cleanup; } releaseaddr = true; @@ -1224,6 +1239,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, VIR_FREE(vhostfdName); virObjectUnref(cfg); virDomainCCWAddressSetFree(ccwaddrs); + virDomainPCIAddressSetFree(pciaddrs); return ret; @@ -1275,6 +1291,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, bool teardownlabel = false; int backend; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainPCIAddressSetPtr pciaddrs = NULL; unsigned int flags = 0; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) @@ -1336,8 +1353,13 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto error; - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0) + + if (!(pciaddrs = qemuDomainPCIAddrSetCreateFromDomain(vm->def, priv->qemuCaps))) + goto error; + + if (virDomainPCIAddressEnsureAddr(pciaddrs, hostdev->info) < 0) goto error; + releaseaddr = true; if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PCI_CONFIGFD)) { @@ -1397,6 +1419,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, cleanup: virObjectUnref(cfg); + virDomainPCIAddressSetFree(pciaddrs); return -1; } @@ -1569,6 +1592,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, { int ret = -1; virDomainVirtioSerialAddrSetPtr vioaddrs = NULL; + virDomainPCIAddressSetPtr pciaddrs = NULL; if (!(vioaddrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) goto cleanup; @@ -1582,7 +1606,9 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) + if (!(pciaddrs = qemuDomainPCIAddrSetCreateFromDomain(def, priv->qemuCaps))) + goto cleanup; + if (virDomainPCIAddressEnsureAddr(pciaddrs, &chr->info) < 0) goto cleanup; ret = 1; @@ -1614,6 +1640,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, cleanup: virDomainVirtioSerialAddrSetFree(vioaddrs); + virDomainPCIAddressSetFree(pciaddrs); return ret; } @@ -1705,6 +1732,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, bool objAdded = false; virJSONValuePtr props = NULL; virDomainCCWAddressSetPtr ccwaddrs = NULL; + virDomainPCIAddressSetPtr pciaddrs = NULL; const char *type; int ret = -1; int rv; @@ -1732,8 +1760,10 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &rng->info) < 0) - return -1; + if (!(pciaddrs = qemuDomainPCIAddrSetCreateFromDomain(vm->def, priv->qemuCaps))) + goto cleanup; + if (virDomainPCIAddressEnsureAddr(pciaddrs, &rng->info) < 0) + goto cleanup; } else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) goto cleanup; @@ -1791,6 +1821,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, VIR_FREE(objAlias); VIR_FREE(devstr); virDomainCCWAddressSetFree(ccwaddrs); + virDomainPCIAddressSetFree(pciaddrs); return ret; exit_monitor: -- 2.7.4 (Apple Git-66)
The cached pci address set is not required anymore, because the set is now being recalculated from the domain definition on demand, so the cache can be deleted. --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 16 +++------------- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9045f37..d3abbaa 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1327,7 +1327,6 @@ qemuDomainObjPrivateFree(void *data) virObjectUnref(priv->qemuCaps); virCgroupFree(&priv->cgroup); - virDomainPCIAddressSetFree(priv->pciaddrs); virDomainUSBAddressSetFree(priv->usbaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6f349a1..35e3d09 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -184,7 +184,6 @@ struct _qemuDomainObjPrivate { bool beingDestroyed; char *pidfile; - virDomainPCIAddressSetPtr pciaddrs; virDomainUSBAddressSetPtr usbaddrs; virQEMUCapsPtr qemuCaps; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2191009..7d70089 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1466,12 +1466,10 @@ qemuDomainPCIAddrSetCreateFromDomain(virDomainDefPtr def, static int qemuDomainAssignPCIAddresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) + virQEMUCapsPtr qemuCaps) { int ret = -1; virDomainPCIAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; int max_idx = -1; int nbuses = 0; size_t i; @@ -1625,14 +1623,6 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, } } - if (obj && obj->privateData) { - priv = obj->privateData; - /* if this is the live domain object, we persist the PCI addresses */ - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->pciaddrs = addrs; - addrs = NULL; - } - ret = 0; cleanup: @@ -1791,7 +1781,7 @@ qemuDomainAssignUSBAddresses(virDomainDefPtr def, int qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj, + virDomainObjPtr obj ATTRIBUTE_UNUSED, bool newDomain) { if (qemuDomainAssignVirtioSerialAddresses(def) < 0) @@ -1805,7 +1795,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def, qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps); - if (qemuDomainAssignPCIAddresses(def, qemuCaps, obj) < 0) + if (qemuDomainAssignPCIAddresses(def, qemuCaps) < 0) return -1; if (newDomain && qemuDomainAssignUSBAddresses(def, obj) < 0) -- 2.7.4 (Apple Git-66)
Since address sets are now recalculated on demand instead of being cached, there's no need for functions that release addresses. --- src/conf/domain_addr.c | 100 ----------------------------------------------- src/conf/domain_addr.h | 16 -------- src/libvirt_private.syms | 3 -- 3 files changed, 119 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index e09b409..5368aa0 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -497,39 +497,6 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, } -int -virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, - virPCIDeviceAddressPtr addr) -{ - addrs->buses[addr->bus].slots[addr->slot] &= ~(1 << addr->function); - return 0; -} - -int -virDomainPCIAddressReleaseSlot(virDomainPCIAddressSetPtr addrs, - virPCIDeviceAddressPtr addr) -{ - /* permit any kind of connection type in validation, since we - * already had it, and are giving it back. - */ - virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPES_MASK; - int ret = -1; - char *addrStr = NULL; - - if (!(addrStr = virDomainPCIAddressAsString(addr))) - goto cleanup; - - if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags, false)) - goto cleanup; - - addrs->buses[addr->bus].slots[addr->slot] = 0; - ret = 0; - cleanup: - VIR_FREE(addrStr); - return ret; -} - - virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses) { @@ -781,29 +748,6 @@ virDomainCCWAddressValidate(virDomainDefPtr def ATTRIBUTE_UNUSED, return virDomainCCWAddressAssign(info, data, false); } -int -virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, - virDomainDeviceInfoPtr dev) -{ - char *addr; - int ret; - - addr = virDomainCCWAddressAsString(&(dev->addr.ccw)); - if (!addr) - return -1; - - if ((ret = virHashRemoveEntry(addrs->defined, addr)) == 0 && - dev->addr.ccw.cssid == addrs->next.cssid && - dev->addr.ccw.ssid == addrs->next.ssid && - dev->addr.ccw.devno < addrs->next.devno) { - addrs->next.devno = dev->addr.ccw.devno; - addrs->next.assigned = false; - } - - VIR_FREE(addr); - - return ret; -} void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs) { @@ -1239,50 +1183,6 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def ATTRIBUTE_UNUSED, return ret; } -/* virDomainVirtioSerialAddrRelease - * - * Release the virtio serial address of the device - */ -int -virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, - virDomainDeviceInfoPtr info) -{ - virBitmapPtr map; - char *str = NULL; - int ret = -1; - ssize_t i; - - if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || - info->addr.vioserial.port == 0) - return 0; - - VIR_DEBUG("Releasing virtio serial %u %u", info->addr.vioserial.controller, - info->addr.vioserial.port); - - i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); - if (i < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("virtio serial controller %u is missing"), - info->addr.vioserial.controller); - goto cleanup; - } - - map = addrs->controllers[i]->ports; - if (virBitmapClearBit(map, info->addr.vioserial.port) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("virtio serial controller %u does not have port %u"), - info->addr.vioserial.controller, - info->addr.vioserial.port); - goto cleanup; - } - - ret = 0; - - cleanup: - VIR_FREE(str); - return ret; -} - bool virDomainUSBAddressPortIsValid(unsigned int *port) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 4584e0a..87865e6 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -139,14 +139,6 @@ int virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, - virPCIDeviceAddressPtr addr) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - -int virDomainPCIAddressReleaseSlot(virDomainPCIAddressSetPtr addrs, - virPCIDeviceAddressPtr addr) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr next_addr, virDomainPCIConnectFlags flags) @@ -180,9 +172,6 @@ int virDomainCCWAddressValidate(virDomainDefPtr def, void *data) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); -int virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, - virDomainDeviceInfoPtr dev) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void); struct _virDomainVirtioSerialController { @@ -235,11 +224,6 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def, void *data) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); -int -virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, - virDomainDeviceInfoPtr info) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - bool virDomainUSBAddressPortIsValid(unsigned int *port) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6b54812..f020e2f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -87,7 +87,6 @@ virPCIDeviceAddressParseXML; # conf/domain_addr.h virDomainCCWAddressAllocate; virDomainCCWAddressAssign; -virDomainCCWAddressReleaseAddr; virDomainCCWAddressSetCreate; virDomainCCWAddressSetFree; virDomainCCWAddressValidate; @@ -97,7 +96,6 @@ virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; virDomainPCIAddressFlagsCompatible; virDomainPCIAddressGetNextSlot; -virDomainPCIAddressReleaseSlot; virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextSlot; virDomainPCIAddressReserveSlot; @@ -122,7 +120,6 @@ virDomainUSBAddressSetFree; virDomainVirtioSerialAddrAssign; virDomainVirtioSerialAddrAutoAssign; virDomainVirtioSerialAddrIsComplete; -virDomainVirtioSerialAddrRelease; virDomainVirtioSerialAddrReserve; virDomainVirtioSerialAddrSetAddControllers; virDomainVirtioSerialAddrSetCreate; -- 2.7.4 (Apple Git-66)
On 07/22/2016 09:47 PM, Tomasz Flendrich wrote:
Since address sets are now recalculated on demand instead of being cached, there's no need for functions that release addresses.
Note there's other bits to clean up here, some functions can be made private to this file. For example VirtioSerialAddrReserve and VirtioSerialAddrSetCreate, and probably many others. Just an idea for an additional patch! Thanks, Cole
On Sat, Jul 23, 2016 at 03:47:05AM +0200, Tomasz Flendrich wrote:
From: Tomasz Flendrich <Darge@users.noreply.github.com>
These patches delete the caching of pci, virtioSerial and ccw address sets. I am deleting them, because they can be recalculated from the domain definition, and there's no point in keeping redundant data, especially because handling a persistent cache of addresses required using functions that released addresses. These functions aren't useful anymore, so they are dropped too.
I know that USB addresses were added and that they are cached now. I am sure that they can be calculated on demand too though.
If this gets accepted, I will rebase the other patches that depend on this one, making functions less dependant on qemu, and removing the code duplication.
I went ahead and pushed patches 1-6 (that is ccw and virtio-serial related). Looking forward to v2 of the rest. I forgot to send this yesterday and it got stuck in my drafts O:-) Martin
participants (3)
-
Cole Robinson -
Martin Kletzander -
Tomasz Flendrich