On 07/05/13 20:23, John Ferlan wrote:
On 05/03/2013 02:07 PM, Osier Yang wrote:
It's better to put the usb related codes into qemuDomainAttachHostUsbDevice
instead of qemuDomainAttachHostDevice.
And in the old qemuDomainAttachHostDevice, just stealing the "usb" from
driver->activeUsbHostdevs leaks the memory.
Seems this is a bug fix that potentially could be separated from this
patch stream... Right?
Yeah, I found it where trying to refacor the code, but my initial purpose
was to prepare for later patch, so I think it's fine to be in this set.
---
src/qemu/qemu_hotplug.c | 77 +++++++++++++++++++++----------------------------
1 file changed, 33 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a4f48b0..422d336 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1136,20 +1136,38 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
- int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virUSBDeviceList *list = NULL;
+ virUSBDevicePtr usb = NULL;
char *devstr = NULL;
+ bool added = false;
+ int ret = -1;
+
+ if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0)
+ return -1;
+
+ if (!(list = virUSBDeviceListNew()))
+ goto cleanup;
+
+ if (virUSBDeviceListAdd(list, usb) < 0)
+ goto cleanup;
How is cleanup affected if the next call fails? That is we have
something on 'list', but not on the hostdev, so wouldn't the device need
to be taken from there... Perhaps it's just Unref of list takes care of
that - something I'm not yet clear on.
Yes, Unref will take care of it.
+
+ if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0)
+ goto cleanup;
+
+ added = true;
+ virUSBDeviceListSteal(list, usb);
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0)
- goto error;
+ goto cleanup;
if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, priv->qemuCaps)))
- goto error;
+ goto cleanup;
}
if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) {
virReportOOMError();
- goto error;
+ goto cleanup;
}
qemuDomainObjEnterMonitor(driver, vm);
@@ -1162,26 +1180,24 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver,
qemuDomainObjExitMonitor(driver, vm);
virDomainAuditHostdev(vm, hostdev, "attach", ret == 0);
if (ret < 0)
- goto error;
+ goto cleanup;
vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
+ ret = 0;
+cleanup:
+ if (added)
+ virUSBDeviceListSteal(driver->activeUsbHostdevs, usb);
+ virUSBDeviceFree(usb);
+ virObjectUnref(list);
VIR_FREE(devstr);
-
- return 0;
-
-error:
- VIR_FREE(devstr);
- return -1;
+ return ret;
}
int qemuDomainAttachHostDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
- virUSBDeviceList *list;
- virUSBDevicePtr usb = NULL;
-
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hostdev mode '%s' not supported"),
@@ -1189,33 +1205,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver,
return -1;
}
- if (!(list = virUSBDeviceListNew()))
- goto cleanup;
-
- if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
- if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0)
- goto cleanup;
-
- if (virUSBDeviceListAdd(list, usb) < 0) {
- virUSBDeviceFree(usb);
- usb = NULL;
- goto cleanup;
- }
-
- if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) {
- usb = NULL;
- goto cleanup;
- }
-
- virUSBDeviceListSteal(list, usb);
- }
-
Moving the above hunk causes no "ordering" issues with the following two
calls, right? Since you're placing USB devices on the hostdev list in
the hunk and that won't happen until afterwards now thus the CGroup and
HostdevLabel adjustments below will possibly have different results, right?
I'm not quite sure if I got your meaning completely. But the following
2 calls are not related with the list, their argument "hostdev" are passed
from the parent call.
Osier
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list