[libvirt] [PATCH v2 09/10] qemu: hotplug support for scsi hostdev

[Han Cheng]

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 at 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;
>> }