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

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(a)gmail.com&gt; > --- > 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).

+ deviceFilter->vtbl->GetVendorId(deviceFilter, &vendorIdUtf16); + deviceFilter->vtbl->GetProductId(deviceFilter, &productIdUtf16); + + VBOX_UTF16_TO_UTF8(vendorIdUtf16, &vendorIdUtf8); + VBOX_UTF16_TO_UTF8(productIdUtf16, &productIdUtf8); +

+ + VBOX_UTF16_FREE(vendorIdUtf16); + VBOX_UTF8_FREE(vendorIdUtf8); + + VBOX_UTF16_FREE(productIdUtf16); + VBOX_UTF8_FREE(productIdUtf8);

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list