On 04/03/2013 06:20 PM, Osier Yang wrote:
On 01/04/13 20:01, Han Cheng wrote:
And user should hotplug a virtio-scsi controller if doesn't exist.
I'm wondering if it could be implicitly added.
As I know, adding controller implicitly is for back compatibility. New codes don't need to do this. Am I right?
Usb hostdev related codes are in qemuDomainAttachHostDevice, push down to qemuDomainAttachHostUsbDevice.
Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- src/qemu/qemu_hotplug.c | 211 ++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 173 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b978b97..a78b410 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c + + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) + goto error; + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) + goto error; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) + goto error; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) {
vm->def->nhostdevs + 1
+ virReportOOMError(); + goto error; + }
vm->def->hostdevs[def->nhostdevs] is leaked if it errors out later.
I don't think so. vm->def->hostdevs[def->nhostdevs] is still tracked by vm->def->hostdevs. It is comsumed but without any data. It will be free when we free or realloc vm->def->hostdevs. And it is not big(sizeof(void *)). Besides, others code in this file deal with it in the same way. Actually, I find memory leak with usb code in the file. I fix it in this patch. I'll point it as following. Could you check it for me?
@@ -1123,6 +1215,26 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, int ret; qemuDomainObjPrivatePtr priv = vm->privateData; char *devstr = NULL; + virUSBDeviceList *list; + virUSBDevicePtr usb = NULL; + + + if (!(list = virUSBDeviceListNew())) + goto error; + + if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) + goto error; + + if (virUSBDeviceListAdd(list, usb) < 0) { + virUSBDeviceFree(usb); + usb = NULL; + goto error; + } + + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) { + usb = NULL; + goto error; + } if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) @@ -1138,7 +1250,6 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { virCgroupPtr cgroup = NULL; - virUSBDevicePtr usb; qemuCgroupData data; if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { @@ -1148,19 +1259,12 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, goto error; } - if ((usb = virUSBDeviceNew(hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device, - NULL)) == NULL) - goto error; - data.vm = vm; data.cgroup = cgroup; if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, &data) < 0) { - virUSBDeviceFree(usb); goto error; } - virUSBDeviceFree(usb); } qemuDomainObjEnterMonitor(driver, vm); @@ -1177,11 +1281,16 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + virUSBDeviceListSteal(list, usb); + virObjectUnref(list); VIR_FREE(devstr); return 0; error: + if (usb) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); + virObjectUnref(list); VIR_FREE(devstr); return -1; } @@ -1190,9 +1299,6 @@ 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"), @@ -1200,30 +1306,10 @@ 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);
We steal usb from list. *usb is tracked both by usb and driver->activeUsbHostdevs. If it errors later, we goto error: [1]
- } if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) - goto cleanup; + return -1; switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1238,6 +1324,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (qemuDomainAttachHostScsiDevice(driver, vm, + hostdev) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -1245,18 +1337,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; } - virObjectUnref(list); return 0; error: if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); - -cleanup: - virObjectUnref(list); - if (usb) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); [1] As *usb is not tracked by list, freeing list doesn't free *usb. We just steal not delete from driver->activeUsbHostdevs and don't free usb, so *usb leaks.
I fixed this by making stealing usb from list just before we return 0.
return -1; }