Re: [libvirt] [PATCH 1/3] esx: Wrap libcurl multi handle

On Tue, Jul 3, 2012 at 3:17 PM, Matthias Bolte <matthias.bolte(a)googlemail.com&gt; wrote: > 2012/7/3 Doug Goldstein <cardoe(a)gentoo.org&gt;: >> On Mon, Jul 2, 2012 at 4:44 PM, Matthias Bolte >> <matthias.bolte(a)googlemail.com&gt; wrote: >>> --- >>> src/esx/esx_vi.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> src/esx/esx_vi.h | 18 +++++++++ >>> 2 files changed, 129 insertions(+), 0 deletions(-) > >>> +/* esxVI_MultiCURL_Alloc */ >>> +ESX_VI__TEMPLATE__ALLOC(MultiCURL) >>> + >>> +/* esxVI_MultiCURL_Free */ >>> +ESX_VI__TEMPLATE__FREE(MultiCURL, >>> +{ >>> + if (item->count > 0) { >>> + /* Better leak than crash */ >>> + VIR_ERROR(_("Trying to free MultiCURL object that is still in use")); >>> + return; >>> + } >>> + >>> + if (item->handle != NULL) { >>> + curl_multi_cleanup(item->handle); >>> + } >> >> Since its a double pointer maybe setting the passed in value to NULL >> to prevent a double free situations? > > Actually that's what already happens here but hidden inside the > ESX_VI__TEMPLATE__FREE macro. Expanded esxVI_MultiCURL_Free looks like > this > > void esxVI_MultiCURL_Free(esxVI_MultiCURL **multi) > { > esxVI_MultiCURL *item; > > if (multi == NULL || *multi == NULL) { > return; > } > > item = *multi; > > if (item->count > 0) { > /* Better leak than crash */ > VIR_ERROR(_("Trying to free MultiCURL object that is still in use")); > return; > } > > if (item->handle != NULL) { > curl_multi_cleanup(item->handle); > } > > VIR_FREE(*multi); > } > > As VIR_FREE sets its argument to NULL after freeing it a double free > is not possible here. > >>> +int >>> +esxVI_MultiCURL_Remove(esxVI_MultiCURL *multi, esxVI_CURL *curl) >>> +{ >>> + if (curl->handle == NULL) { >>> + ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", >>> + _("Cannot remove uninitialized CURL handle from a " >>> + "multi handle")); >>> + return -1; >>> + } >>> + >>> + if (curl->multi == NULL) { >>> + ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", >>> + _("Cannot remove CURL handle from a multi handle when it " >>> + "wasn't added before")); >>> + return -1; >>> + } >>> + >>> + if (curl->multi != multi) { >>> + ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("CURL (multi) mismatch")); >>> + return -1; >>> + } >>> + >>> + virMutexLock(&curl->lock); >>> + >>> + curl_multi_remove_handle(multi->handle, curl->handle); >>> + >>> + curl->multi = NULL; >>> + --multi->count; >>> + >>> + virMutexUnlock(&curl->lock); >> >> Maybe add your free code here when count is 0? That way you wouldn't >> have to contend with a potential memory leak case when the free is >> called when its still ref'd. > > count is not meant as a refcount here. It's sole purpose is to avoid > freeing a multi handle that still has easy handles attached to it. > Also calling free one count == 0 here would not allow a call sequence > such as add, remove, add, remove. > > -- > Matthias Bolte > http://photron.blogspot.com Ok good. Just was double checking. Then ACK from me on this patch (its not affected by the multi-CONTENT issue mentioned in the other e-mail).