Hi Laine,
On Mon, Nov 25, 2013 at 8:26 PM, Laine Stump <laine@xxxxxxxxx> wrote:
> On 11/21/2013 04:41 PM, Ryota Ozaki wrote:
>> The USB-related code in vboxDomainGetXMLDesc is deeply nested and
>> difficult to add new code. So flatten it. To do so, the code is
>> pulled out from vboxDomainGetXMLDesc to make the function short
>> and to leaverage early return and goto for error handling.
>>
>> Signed-off-by: Ryota Ozaki <ozaki.ryota@xxxxxxxxx>
>> ---
>> src/vbox/vbox_tmpl.c | 183 +++++++++++++++++++++++++++------------------------
>> 1 file changed, 97 insertions(+), 86 deletions(-)
>>
>> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
>> index 67dd23a..95a04b1 100644
>> --- a/src/vbox/vbox_tmpl.c
>> +++ b/src/vbox/vbox_tmpl.c
>> @@ -2206,6 +2206,102 @@ vboxDomainGetMaxVcpus(virDomainPtr dom)
>> VIR_DOMAIN_VCPU_MAXIMUM));
>> }
>>
>> +static void vboxHostDeviceGetXMLDesc(vboxGlobalData *data, virDomainDefPtr def, IMachine *machine)
>> +{
>> + IUSBController *USBController = NULL;
>> + PRBool enabled = PR_FALSE;
>> + vboxArray deviceFilters = VBOX_ARRAY_INITIALIZER;
>> + size_t i;
>> + PRUint32 USBFilterCount = 0;
>> +
>> + def->nhostdevs = 0;
>> + machine->vtbl->GetUSBController(machine, &USBController);
>> +
>> + if (!USBController)
>> + return;
>> +
>> + USBController->vtbl->GetEnabled(USBController, &enabled);
>> + if (!enabled)
>> + goto release_controller;
>> +
>> + vboxArrayGet(&deviceFilters, USBController,
>> + USBController->vtbl->GetDeviceFilters);
>> +
>> + if (deviceFilters.count <= 0)
>> + goto release_filters;
>> +
>> + /* check if the filters are active and then only
>> + * alloc mem and set def->nhostdevs
>> + */
>> +
>> + for (i = 0; i < deviceFilters.count; i++) {
>> + PRBool active = PR_FALSE;
>> + IUSBDeviceFilter *deviceFilter = deviceFilters.items[i];
>> +
>> + deviceFilter->vtbl->GetActive(deviceFilter, &active);
>> + if (active) {
>> + def->nhostdevs++;
>> + }
>> + }
>> +
>> + if (def->nhostdevs == 0)
>> + goto release_filters;
>> +
>> + /* Alloc mem needed for the filters now */
>> + if (VIR_ALLOC_N(def->hostdevs, def->nhostdevs) < 0)
>> + goto release_filters;
>> +
>> + for (i = 0; (USBFilterCount < def->nhostdevs) || (i < deviceFilters.count); i++) {
>> + PRBool active = PR_FALSE;
>> + IUSBDeviceFilter *deviceFilter = deviceFilters.items[i];
>> + PRUnichar *vendorIdUtf16 = NULL;
>> + char *vendorIdUtf8 = NULL;
>> + unsigned vendorId = 0;
>> + PRUnichar *productIdUtf16 = NULL;
>> + char *productIdUtf8 = NULL;
>> + unsigned productId = 0;
>> + char *endptr = NULL;
>
>
> Again, very mechanical. ACK and pushed.
>
> I just want to make sure of one thing though - there are several char*'s
> here that are set by calling some other function unknown to me, so I
> don't know if those functions are returning a pointer to a pre-existing
> string, or allocating a new string. (The standard practice in libvirt
> code is to declare things that are simply pointing to pre-existing
> buffers as "const char *" to avoid confusion). Can you verify that we're
> not leaking anything here? (Again, this is something that you didn't
> change, so has no bearing on whether or not the current patch should go in).
I looked more and confirmed that the below code
allocates and frees strings correctly.
> + deviceFilter->vtbl->GetVendorId(deviceFilter, &vendorIdUtf16);
> + deviceFilter->vtbl->GetProductId(deviceFilter, &productIdUtf16);
> +
> + VBOX_UTF16_TO_UTF8(vendorIdUtf16, &vendorIdUtf8);
> + VBOX_UTF16_TO_UTF8(productIdUtf16, &productIdUtf8);
> +
(snip)
> +
> + VBOX_UTF16_FREE(vendorIdUtf16);
> + VBOX_UTF8_FREE(vendorIdUtf8);
> +
> + VBOX_UTF16_FREE(productIdUtf16);
> + VBOX_UTF8_FREE(productIdUtf8);
VBOX_UTF16_TO_UTF8 is a wrapper of pfnUtf16ToUtf8()
and VBOX_UTF{16,8}_FREE are pfnUtf{16,8}Free().
And this is a code snippet from SDK:
PRUnichar *uuidUtf16 = NULL;
char *uuidUtf8 = NULL;
machine->vtbl->GetId(machine, &uuidUtf16);
g_pVBoxFuncs->pfnUtf16ToUtf8(uuidUtf16, &uuidUtf8);
printf("\tUUID: %s\n", uuidUtf8);
g_pVBoxFuncs->pfnUtf8Free(uuidUtf8);
g_pVBoxFuncs->pfnUtf16Free(uuidUtf16);
So the usage of the functions in vbox_tmpl.c looks ok for me.
Thanks,
ozaki-r
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list