On 07/05/13 20:23, John Ferlan 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
On 05/03/2013 02:07 PM, Osier Yang wrote: 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