[libvirt] [PATCH 2/4] vbox: pull vboxHostDeviceGetXMLDesc out from vboxDomainGetXMLDesc

[Ryota Ozaki]

Hi Laine,

On Mon, Nov 25, 2013 at 8:26 PM, Laine Stump <laine at laine.org> 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 at gmail.com>
>> ---
>>  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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list