[libvirt] [PATCH RFC 0/5] Remove usb address set caching
During my work on removing addresses caching, usb addresses were added :). It should be applied on top of: https://www.redhat.com/archives/libvir-list/2016-August/msg00945.html or some small conflicts appear. I took an approach where exactly the same function is used to both calculate and recalculate the address set. It forces us to keep all the usb address handling in such state that reusing that function doesn't perform any changes that aren't needed. It might even be possible to unify the handling of usb devices into one function, "qemuDomainAssignUSBAddresses", and remove some code. For example, virDomainUSBAddressEnsure looks almost exactly like qemuDomainAssignUSBPorts plus virDomainUSBAddressReserve. The end goal would be simply calling qemuDomainAssignUSBAddresses() as many times as we want, for example when creating a new domain or after a hotplug. To see what I mean, please take a look at qemuDomainAttachUSBMassStorageDevice. If I remove the call to virDomainUSBAddressEnsure and move creating the USB address set somewhere down (after virDomainDiskInsertPreAlloced), it works - the address is assigned flawlessly. Does it make any sense? Is there any reason why we don't add new usb hubs when hotplugging devices? By the way please excuse me for resending an old patch series a few minutes ago, it was an accident. Tomasz Flendrich (5): Add qemuDomainUSBAddrSetCreateFromDomain qemu_hotplug: use a recalculated usb address set remove qemuDomainReleaseDeviceAddress and its uses remove unused virDomainUSBAddressRelease Add a USB hotplug testcase for reattachment src/conf/domain_addr.c | 30 ------------- src/conf/domain_addr.h | 5 --- src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_domain.h | 2 - src/qemu/qemu_domain_address.c | 62 ++++++++++++--------------- src/qemu/qemu_domain_address.h | 8 ++-- src/qemu/qemu_hotplug.c | 95 +++++++++++------------------------------- src/qemu/qemu_process.c | 6 +-- tests/qemuhotplugtest.c | 7 +++- 10 files changed, 62 insertions(+), 157 deletions(-) -- 1.9.1
In order to drop caching of the usb address set, a function is added that creates or recreates the usb address set, assigning addresses if necessary. --- src/qemu/qemu_domain_address.c | 39 ++++++++++++++++++++++++++++----------- src/qemu/qemu_domain_address.h | 3 +++ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 0aa940b..99df92c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1743,42 +1743,59 @@ qemuDomainUSBAddressAddHubs(virDomainDefPtr def) } -static int -qemuDomainAssignUSBAddresses(virDomainDefPtr def, - virDomainObjPtr obj) +virDomainUSBAddressSetPtr +qemuDomainUSBAddrSetCreateFromDomain(virDomainDefPtr def) { - int ret = -1; virDomainUSBAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; if (!(addrs = virDomainUSBAddressSetCreate())) - goto cleanup; + goto error; if (qemuDomainUSBAddressAddHubs(def) < 0) - goto cleanup; + goto error; if (virDomainUSBAddressSetAddControllers(addrs, def) < 0) - goto cleanup; + goto error; if (virDomainUSBDeviceDefForeach(def, virDomainUSBAddressReserve, addrs, true) < 0) - goto cleanup; + goto error; VIR_DEBUG("Existing USB addresses have been reserved"); if (qemuDomainAssignUSBHubs(addrs, def) < 0) - goto cleanup; + goto error; if (qemuDomainAssignUSBPorts(addrs, def) < 0) - goto cleanup; + goto error; VIR_DEBUG("Finished assigning USB ports"); + return addrs; + + error: + virDomainUSBAddressSetFree(addrs); + return NULL; +} + + +static int +qemuDomainAssignUSBAddresses(virDomainDefPtr def, + virDomainObjPtr obj) +{ + int ret = -1; + virDomainUSBAddressSetPtr addrs = NULL; + qemuDomainObjPrivatePtr priv = NULL; + + if (!(addrs = qemuDomainUSBAddrSetCreateFromDomain(def))) + goto cleanup; + if (obj && obj->privateData) { priv = obj->privateData; priv->usbaddrs = addrs; addrs = NULL; } + ret = 0; cleanup: diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index dee352b..dfa76d2 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -51,6 +51,9 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, bool dryRun, bool assign); +virDomainUSBAddressSetPtr +qemuDomainUSBAddrSetCreateFromDomain(virDomainDefPtr def); + # define __QEMU_DOMAIN_ADDRESS_H__ #endif /* __QEMU_DOMAIN_ADDRESS_H__ */ -- 1.9.1
Since we now have a way of relculating the usb address set, all the places that previously used the cached set now use the recalculated one. --- src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 51f06bc..078d936 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -738,13 +738,13 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, bool driveAdded = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); - bool releaseaddr = false; + virDomainUSBAddressSetPtr usbaddrs = NULL; - if (priv->usbaddrs) { - if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0) - goto cleanup; - releaseaddr = true; - } + if (!(usbaddrs = qemuDomainUSBAddrSetCreateFromDomain(vm->def))) + goto cleanup; + + if (virDomainUSBAddressEnsure(usbaddrs, &disk->info) < 0) + goto cleanup; if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -789,8 +789,7 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, ret = 0; cleanup: - if (ret < 0 && releaseaddr) - virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); + virDomainUSBAddressSetFree(usbaddrs); VIR_FREE(devstr); VIR_FREE(drivealias); VIR_FREE(drivestr); @@ -1616,6 +1615,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, int ret = -1; virDomainVirtioSerialAddrSetPtr vioaddrs = NULL; virDomainPCIAddressSetPtr pciaddrs = NULL; + virDomainUSBAddressSetPtr usbaddrs = NULL; if (!(vioaddrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) goto cleanup; @@ -1638,7 +1638,9 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) { - if (virDomainUSBAddressEnsure(priv->usbaddrs, &chr->info) < 0) + if (!(usbaddrs = qemuDomainUSBAddrSetCreateFromDomain(def))) + goto cleanup; + if (virDomainUSBAddressEnsure(usbaddrs, &chr->info) < 0) goto cleanup; ret = 1; @@ -1665,6 +1667,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, cleanup: virDomainVirtioSerialAddrSetFree(vioaddrs); virDomainPCIAddressSetFree(pciaddrs); + virDomainUSBAddressSetFree(usbaddrs); return ret; } @@ -2002,17 +2005,17 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; char *devstr = NULL; - bool releaseaddr = false; bool added = false; bool teardowncgroup = false; bool teardownlabel = false; + virDomainUSBAddressSetPtr usbaddrs = NULL; int ret = -1; - if (priv->usbaddrs) { - if (virDomainUSBAddressEnsure(priv->usbaddrs, hostdev->info) < 0) - goto cleanup; - releaseaddr = true; - } + if (!(usbaddrs = qemuDomainUSBAddrSetCreateFromDomain(vm->def))) + goto cleanup; + + if (virDomainUSBAddressEnsure(usbaddrs, hostdev->info) < 0) + goto cleanup; if (qemuHostdevPrepareUSBDevices(driver, vm->def->name, &hostdev, 1, 0) < 0) goto cleanup; @@ -2059,10 +2062,9 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, VIR_WARN("Unable to restore host device labelling on hotplug fail"); if (added) qemuHostdevReAttachUSBDevices(driver, vm->def->name, &hostdev, 1); - if (releaseaddr) - virDomainUSBAddressRelease(priv->usbaddrs, hostdev->info); } VIR_FREE(devstr); + virDomainUSBAddressSetFree(usbaddrs); return ret; } @@ -3103,8 +3105,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = disk; ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); - if (priv->usbaddrs) - virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); virDomainDiskDefFree(disk); return 0; -- 1.9.1
On Sat, Aug 20, 2016 at 04:53:04PM +0200, Tomasz Flendrich wrote:
Since we now have a way of relculating the usb address set, all the places that previously used the cached set now use the recalculated one. --- src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 51f06bc..078d936 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -738,13 +738,13 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, bool driveAdded = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); - bool releaseaddr = false; + virDomainUSBAddressSetPtr usbaddrs = NULL;
- if (priv->usbaddrs) {
This condition was only true when we assigned the domain addresses on domain startup (if it's a new domain, not migrated)...
- if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0) - goto cleanup; - releaseaddr = true; - } + if (!(usbaddrs = qemuDomainUSBAddrSetCreateFromDomain(vm->def))) + goto cleanup;
but this new helper does it unconditionally. Jan
There is no need to release addresses anymore, because the address sets are now recalculated. --- src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_domain.h | 2 -- src/qemu/qemu_domain_address.c | 31 ++--------------------- src/qemu/qemu_domain_address.h | 5 ---- src/qemu/qemu_hotplug.c | 57 ++++-------------------------------------- src/qemu/qemu_process.c | 6 ++--- tests/qemuhotplugtest.c | 2 +- 7 files changed, 12 insertions(+), 94 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 24e594b..424300a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1328,7 +1328,6 @@ qemuDomainObjPrivateFree(void *data) virObjectUnref(priv->qemuCaps); virCgroupFree(&priv->cgroup); - virDomainUSBAddressSetFree(priv->usbaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); VIR_FREE(priv->lockState); @@ -2660,7 +2659,7 @@ qemuDomainDefAssignAddresses(virDomainDef *def, def->emulator))) goto cleanup; - if (qemuDomainAssignAddresses(def, qemuCaps, NULL, newDomain) < 0) + if (qemuDomainAssignAddresses(def, qemuCaps, newDomain) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index af96d4a..21b4869 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -184,8 +184,6 @@ struct _qemuDomainObjPrivate { bool beingDestroyed; char *pidfile; - virDomainUSBAddressSetPtr usbaddrs; - virQEMUCapsPtr qemuCaps; char *lockState; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 99df92c..88c4ab9 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1780,22 +1780,14 @@ qemuDomainUSBAddrSetCreateFromDomain(virDomainDefPtr def) static int -qemuDomainAssignUSBAddresses(virDomainDefPtr def, - virDomainObjPtr obj) +qemuDomainAssignUSBAddresses(virDomainDefPtr def) { int ret = -1; virDomainUSBAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; if (!(addrs = qemuDomainUSBAddrSetCreateFromDomain(def))) goto cleanup; - if (obj && obj->privateData) { - priv = obj->privateData; - priv->usbaddrs = addrs; - addrs = NULL; - } - ret = 0; cleanup: @@ -1807,7 +1799,6 @@ qemuDomainAssignUSBAddresses(virDomainDefPtr def, int qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj, bool newDomain) { if (qemuDomainAssignVirtioSerialAddresses(def) < 0) @@ -1824,26 +1815,8 @@ qemuDomainAssignAddresses(virDomainDefPtr def, if (qemuDomainAssignPCIAddresses(def, qemuCaps) < 0) return -1; - if (newDomain && qemuDomainAssignUSBAddresses(def, obj) < 0) + if (newDomain && qemuDomainAssignUSBAddresses(def) < 0) return -1; return 0; } - - -void -qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, - virDomainDeviceInfoPtr info, - const char *devstr) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - - if (!devstr) - devstr = info->alias; - - if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && - priv->usbaddrs && - virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) - VIR_WARN("Unable to release USB address on %s", - NULLSTR(devstr)); -} diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index dfa76d2..adf163c 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -33,14 +33,9 @@ int qemuDomainSetSCSIControllerModel(const virDomainDef *def, int qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj, bool newDomain) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, - virDomainDeviceInfoPtr info, - const char *devstr); - virDomainCCWAddressSetPtr qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 078d936..6fb73d0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -309,14 +309,12 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, char *devstr = NULL; char *drivestr = NULL; char *drivealias = NULL; - bool releaseaddr = false; bool driveAdded = false; bool secobjAdded = false; bool encobjAdded = false; virDomainCCWAddressSetPtr ccwaddrs = NULL; virDomainPCIAddressSetPtr pciaddrs = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; qemuDomainDiskPrivatePtr diskPriv; @@ -352,7 +350,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (virDomainPCIAddressEnsureAddr(pciaddrs, &disk->info) < 0) goto error; } - releaseaddr = true; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; @@ -409,10 +406,8 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuMonitorAddDevice(priv->mon, devstr) < 0) goto exit_monitor; - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - releaseaddr = false; + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto error; - } virDomainAuditDisk(vm, NULL, disk->src, "attach", true); @@ -446,15 +441,11 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virFreeError(orig_err); } - if (qemuDomainObjExitMonitor(driver, vm) < 0) - releaseaddr = false; + ignore_value(qemuDomainObjExitMonitor(driver, vm) < 0); virDomainAuditDisk(vm, NULL, disk->src, "attach", false); error: - if (releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &disk->info, src); - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); goto cleanup; } @@ -470,7 +461,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; virDomainCCWAddressSetPtr ccwaddrs = NULL; virDomainPCIAddressSetPtr pciaddrs = NULL; - bool releaseaddr = false; if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -520,7 +510,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, !controller->info.addr.ccw.assigned) < 0) goto cleanup; } - releaseaddr = true; if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, controller) < 0) goto cleanup; @@ -533,7 +522,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorAddDevice(priv->mon, devstr); if (qemuDomainObjExitMonitor(driver, vm) < 0) { - releaseaddr = false; ret = -1; goto cleanup; } @@ -545,9 +533,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, } cleanup: - if (ret != 0 && releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); - VIR_FREE(devstr); virDomainCCWAddressSetFree(ccwaddrs); virDomainPCIAddressSetFree(pciaddrs); @@ -935,7 +920,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virNetDevVPortProfilePtr vport = NULL; int ret = -1; int vlan; - bool releaseaddr = false; bool iface_connected = false; int actualType; virNetDevBandwidthPtr actualBandwidth; @@ -1096,8 +1080,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - releaseaddr = true; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { vlan = -1; } else { @@ -1210,9 +1192,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (!ret) { vm->def->nets[vm->def->nnets++] = net; } else { - if (releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &net->info, NULL); - if (iface_connected) { virDomainConfNWFilterTeardown(net); @@ -1307,7 +1286,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, char *devstr = NULL; int configfd = -1; char *configfd_name = NULL; - bool releaseaddr = false; bool teardowncgroup = false; bool teardownlabel = false; int backend; @@ -1382,7 +1360,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, 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)) { configfd = qemuOpenPCIConfig(hostdev); @@ -1430,9 +1407,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); - if (releaseaddr) - qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); - qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1); VIR_FREE(devstr); @@ -1682,7 +1656,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, char *devstr = NULL; char *charAlias = NULL; bool chardevAttached = false; - bool need_release = false; if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && qemuDomainPrepareChannel(chr, priv->channelTargetDir) < 0) @@ -1693,8 +1666,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if ((rc = qemuDomainAttachChrDeviceAssignAddr(vm->def, priv, chr)) < 0) goto cleanup; - if (rc == 1) - need_release = true; if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; @@ -1723,8 +1694,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, cleanup: if (ret < 0 && virDomainObjIsActive(vm)) qemuDomainChrInsertPreAllocCleanup(vmdef, chr); - if (ret < 0 && need_release) - qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); VIR_FREE(charAlias); VIR_FREE(devstr); return ret; @@ -1754,7 +1723,6 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, char *devstr = NULL; char *charAlias = NULL; char *objAlias = NULL; - bool releaseaddr = false; bool chardevAdded = false; bool objAdded = false; virJSONValuePtr props = NULL; @@ -1783,7 +1751,6 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, rng->source.file)) return -1; } - releaseaddr = true; if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { @@ -1830,10 +1797,8 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (qemuMonitorAddDevice(priv->mon, devstr) < 0) goto exit_monitor; - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - releaseaddr = false; + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - } VIR_APPEND_ELEMENT_INPLACE(vm->def->rngs, vm->def->nrngs, rng); @@ -1843,8 +1808,6 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0); cleanup: virJSONValueFree(props); - if (ret < 0 && releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); VIR_FREE(charAlias); VIR_FREE(objAlias); VIR_FREE(devstr); @@ -1863,8 +1826,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virFreeError(orig_err); } - if (qemuDomainObjExitMonitor(driver, vm) < 0) - releaseaddr = false; + ignore_value(qemuDomainObjExitMonitor(driver, vm) < 0); goto audit; } @@ -3090,8 +3052,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } - qemuDomainReleaseDeviceAddress(vm, &disk->info, src); - if (virSecurityManagerRestoreDiskLabel(driver->securityManager, vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", src); @@ -3132,7 +3092,6 @@ qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver, } } - qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); virDomainControllerDefFree(controller); return 0; } @@ -3192,7 +3151,6 @@ qemuDomainRemovePCIHostDevice(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev) { qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1); - qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); } static void @@ -3201,7 +3159,6 @@ qemuDomainRemoveUSBHostDevice(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev) { qemuHostdevReAttachUSBDevices(driver, vm->def->name, &hostdev, 1); - qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); } static void @@ -3372,7 +3329,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, } } - qemuDomainReleaseDeviceAddress(vm, &net->info, NULL); virDomainConfNWFilterTeardown(net); if (cfg->macFilter && (net->ifname != NULL)) { @@ -3493,7 +3449,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, if ((idx = virDomainRNGFind(vm->def, rng)) >= 0) virDomainRNGRemove(vm->def, idx); - qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); virDomainRNGDefFree(rng); ret = 0; @@ -4349,10 +4304,8 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { - qemuDomainReleaseDeviceAddress(vm, &tmpChr->info, NULL); + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); - } cleanup: qemuDomainResetDeviceRemoval(vm); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 907da30..607d73c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3345,7 +3345,7 @@ qemuProcessReconnect(void *opaque) goto cleanup; } - if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj, false)) < 0) + if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, false)) < 0) goto error; /* if domain requests security driver we haven't loaded, report error, but @@ -4824,7 +4824,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, * use in hotplug */ VIR_DEBUG("Assigning domain PCI addresses"); - if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm, + if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, !!(flags & VIR_QEMU_PROCESS_START_NEW))) < 0) goto cleanup; @@ -6002,7 +6002,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, * use in hotplug */ VIR_DEBUG("Assigning domain PCI addresses"); - if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm, false)) < 0) + if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, false)) < 0) goto error; if ((timestamp = virTimeStringNow()) == NULL) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 0a5f068..e0a7d70 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -87,7 +87,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup; - if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm, true) < 0) + if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, true) < 0) goto cleanup; if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0) -- 1.9.1
We no longer cache address sets, so there's no use of a function that releases addresses. --- src/conf/domain_addr.c | 30 ------------------------------ src/conf/domain_addr.h | 5 ----- src/libvirt_private.syms | 1 - 3 files changed, 36 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index c1b5580..7918781 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1822,33 +1822,3 @@ virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs, return 0; } - - -int -virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs, - virDomainDeviceInfoPtr info) -{ - virDomainUSBAddressHubPtr targetHub = NULL; - char *portStr = NULL; - int targetPort; - int ret = -1; - - if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB || - !virDomainUSBAddressPortIsValid(info->addr.usb.port)) - return 0; - - portStr = virDomainUSBAddressPortFormat(info->addr.usb.port); - VIR_DEBUG("Releasing USB addr bus=%u port=%s", info->addr.usb.bus, portStr); - - if (!(targetHub = virDomainUSBAddressFindPort(addrs, info, &targetPort, - portStr))) - goto cleanup; - - ignore_value(virBitmapClearBit(targetHub->portmap, targetPort)); - - ret = 0; - - cleanup: - VIR_FREE(portStr); - return ret; -} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 596cd4c..15b45eb 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -304,9 +304,4 @@ int virDomainUSBAddressEnsure(virDomainUSBAddressSetPtr addrs, virDomainDeviceInfoPtr info) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - -int -virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs, - virDomainDeviceInfoPtr info) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a32ce1c..59febbe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -113,7 +113,6 @@ virDomainUSBAddressEnsure; virDomainUSBAddressPortFormat; virDomainUSBAddressPortFormatBuf; virDomainUSBAddressPortIsValid; -virDomainUSBAddressRelease; virDomainUSBAddressReserve; virDomainUSBAddressSetAddControllers; virDomainUSBAddressSetAddHub; -- 1.9.1
In order to test the recent changes, a testcase to attach a usb device after detaching one is added. It checks if addresses are properly reused. --- tests/qemuhotplugtest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index e0a7d70..2945fff 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -457,9 +457,12 @@ mymain(void) DO_TEST_ATTACH("base-live", "disk-usb", false, true, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-usb", false, false, + DO_TEST_DETACH("base-live", "disk-usb", false, true, "device_del", QMP_OK, "human-monitor-command", HMP("")); + DO_TEST_ATTACH("base-live", "disk-usb", false, false, + "human-monitor-command", HMP("OK\\r\\n"), + "device_add", QMP_OK); DO_TEST_ATTACH_EVENT("base-live", "disk-usb", false, true, "human-monitor-command", HMP("OK\\r\\n"), -- 1.9.1
On Sat, Aug 20, 2016 at 04:53:02PM +0200, Tomasz Flendrich wrote:
During my work on removing addresses caching, usb addresses were added :).
It should be applied on top of: https://www.redhat.com/archives/libvir-list/2016-August/msg00945.html or some small conflicts appear.
I took an approach where exactly the same function is used to both calculate and recalculate the address set. It forces us to keep all the usb address handling in such state that reusing that function doesn't perform any changes that aren't needed.
It might even be possible to unify the handling of usb devices into one function, "qemuDomainAssignUSBAddresses", and remove some code. For example, virDomainUSBAddressEnsure looks almost exactly like qemuDomainAssignUSBPorts plus virDomainUSBAddressReserve. The end goal would be simply calling qemuDomainAssignUSBAddresses() as many times as we want, for example when creating a new domain or after a hotplug.
I don't like calling functions that alter the whole domain definition after hotplugging one device. We should be hotplugging a device that is ready for the domain definition.
To see what I mean, please take a look at qemuDomainAttachUSBMassStorageDevice. If I remove the call to virDomainUSBAddressEnsure and move creating the USB address set somewhere down (after virDomainDiskInsertPreAlloced), it works - the address is assigned flawlessly. Does it make any sense?
Is there any reason why we don't add new usb hubs when hotplugging devices?
That kind of policy does not belong in libvirt, IMO. We have made the mistake of auto-adding it for SCSI disks. The problem is that the user might rather want to put it on a separate USB controller, or just fail. Jan
participants (2)
-
Ján Tomko -
Tomasz Flendrich