9:53 a.m.
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
9:53 a.m.
New subject: [libvirt] [PATCH RFC 1/5] Add qemuDomainUSBAddrSetCreateFromDomain
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
9:53 a.m.
New subject: [libvirt] [PATCH RFC 2/5] qemu_hotplug: use a recalculated usb address set
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
8:48 a.m.
New subject: [libvirt] [PATCH RFC 2/5] qemu_hotplug: use a recalculated usb address set
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
9:53 a.m.
New subject: [libvirt] [PATCH RFC 3/5] remove qemuDomainReleaseDeviceAddress and its uses
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
9:53 a.m.
New subject: [libvirt] [PATCH RFC 4/5] remove unused virDomainUSBAddressRelease
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
9:53 a.m.
New subject: [libvirt] [PATCH RFC 5/5] Add a usb hotplug testcase for reattachment
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
8:59 a.m.
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
3013
days inactive
3016
days old
7 comments
2 participants
participants (2)
-
Ján Tomko -
Tomasz Flendrich