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

[Osier Yang]

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