Re: [libvirt] [PATCH 11/25] qemu: Refactor helpers for USB device attachment

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?

> --- > 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.

> + > + 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?