[libvirt] [PATCH 1/3] qemu: hotplug: Only label hostdev after checking device conflicts
Similar to what Jiri did for cgroup setup/teardown in 05e149f94, push it all into the device handler functions so we can do the necessary prep work before claiming the device. This also fixes hotplugging USB devices by product/vendor (virt-manager's default behavior): https://bugzilla.redhat.com/show_bug.cgi?id=1016511 --- src/qemu/qemu_hotplug.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f4fc723..bff9e23 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1153,6 +1153,7 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, char *configfd_name = NULL; bool releaseaddr = false; bool teardowncgroup = false; + bool teardownlabel = false; int backend = hostdev->source.subsys.u.pci.backend; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) @@ -1193,6 +1194,11 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, goto error; teardowncgroup = true; + if (virSecurityManagerSetHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + goto error; + teardownlabel = true; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto error; @@ -1250,6 +1256,10 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, error: if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (teardownlabel && + virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + VIR_WARN("Unable to restore host device labelling on hotplug fail"); if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); @@ -1445,6 +1455,7 @@ qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, char *devstr = NULL; bool added = false; bool teardowncgroup = false; + bool teardownlabel = false; int ret = -1; if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) @@ -1466,6 +1477,11 @@ qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, goto cleanup; teardowncgroup = true; + if (virSecurityManagerSetHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + goto cleanup; + teardownlabel = true; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; @@ -1492,10 +1508,14 @@ qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, ret = 0; cleanup: - if (ret < 0 && - teardowncgroup && - qemuTeardownHostdevCgroup(vm, hostdev) < 0) - VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (ret < 0) { + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (teardownlabel && + virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + VIR_WARN("Unable to restore host device labelling on hotplug fail"); + } if (added) virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); virUSBDeviceFree(usb); @@ -1515,6 +1535,7 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, char *devstr = NULL; char *drvstr = NULL; bool teardowncgroup = false; + bool teardownlabel = false; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) || @@ -1543,6 +1564,11 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, goto cleanup; teardowncgroup = true; + if (virSecurityManagerSetHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + goto cleanup; + teardownlabel = true; + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; @@ -1584,6 +1610,10 @@ cleanup: qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (teardownlabel && + virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + VIR_WARN("Unable to restore host device labelling on hotplug fail"); } VIR_FREE(drvstr); VIR_FREE(devstr); @@ -1601,10 +1631,6 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, return -1; } - if (virSecurityManagerSetHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) - return -1; - switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (qemuDomainAttachHostPciDevice(driver, vm, -- 1.8.4.2
If we hit a collision, we free the USB device while it is still part of our temporary USBDeviceList. When the list is unref'd, the device is free'd again. Make the initial device freeing dependent on whether it is present in the temporary list or not. --- src/qemu/qemu_hotplug.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bff9e23..b7512a7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1518,7 +1518,10 @@ cleanup: } if (added) virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); - virUSBDeviceFree(usb); + if (list && usb && + !virUSBDeviceListFind(list, usb) && + !virUSBDeviceListFind(driver->activeUsbHostdevs, usb)) + virUSBDeviceFree(usb); virObjectUnref(list); VIR_FREE(devstr); return ret; -- 1.8.4.2
On Thu, Dec 05, 2013 at 15:40:27 -0500, Cole Robinson wrote:
If we hit a collision, we free the USB device while it is still part of our temporary USBDeviceList. When the list is unref'd, the device is free'd again.
Make the initial device freeing dependent on whether it is present in the temporary list or not. --- src/qemu/qemu_hotplug.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bff9e23..b7512a7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1518,7 +1518,10 @@ cleanup: } if (added) virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); - virUSBDeviceFree(usb); + if (list && usb && + !virUSBDeviceListFind(list, usb) && + !virUSBDeviceListFind(driver->activeUsbHostdevs, usb)) + virUSBDeviceFree(usb); virObjectUnref(list); VIR_FREE(devstr); return ret;
ACK Jirka
We were unconditionally removing the device from the host list, when it should only be done on error. This fixes USB collision detection when hotplugging the same device to two guests. --- src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b7512a7..16b990d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1515,9 +1515,9 @@ cleanup: virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (added) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); } - if (added) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); if (list && usb && !virUSBDeviceListFind(list, usb) && !virUSBDeviceListFind(driver->activeUsbHostdevs, usb)) -- 1.8.4.2
On Thu, Dec 05, 2013 at 15:40:28 -0500, Cole Robinson wrote:
We were unconditionally removing the device from the host list, when it should only be done on error.
This fixes USB collision detection when hotplugging the same device to two guests. --- src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b7512a7..16b990d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1515,9 +1515,9 @@ cleanup: virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (added) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); } - if (added) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); if (list && usb && !virUSBDeviceListFind(list, usb) && !virUSBDeviceListFind(driver->activeUsbHostdevs, usb))
ACK Jirka
On 12/09/2013 07:09 AM, Jiri Denemark wrote:
On Thu, Dec 05, 2013 at 15:40:28 -0500, Cole Robinson wrote:
We were unconditionally removing the device from the host list, when it should only be done on error.
This fixes USB collision detection when hotplugging the same device to two guests. --- src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b7512a7..16b990d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1515,9 +1515,9 @@ cleanup: virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (added) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); } - if (added) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); if (list && usb && !virUSBDeviceListFind(list, usb) && !virUSBDeviceListFind(driver->activeUsbHostdevs, usb))
ACK
Jirka
Thanks, pushed these 3 now. - Cole
On Thu, Dec 05, 2013 at 15:40:26 -0500, Cole Robinson wrote:
Similar to what Jiri did for cgroup setup/teardown in 05e149f94, push it all into the device handler functions so we can do the necessary prep work before claiming the device.
This also fixes hotplugging USB devices by product/vendor (virt-manager's default behavior):
https://bugzilla.redhat.com/show_bug.cgi?id=1016511 --- src/qemu/qemu_hotplug.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)
ACK Jirka
participants (2)
-
Cole Robinson -
Jiri Denemark