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

[Doug Goldstein]

On Tue, Jul 3, 2012 at 3:17 PM, Matthias Bolte
<matthias.bolte at googlemail.com> wrote:
> 2012/7/3 Doug Goldstein <cardoe at gentoo.org>:
>> On Mon, Jul 2, 2012 at 4:44 PM, Matthias Bolte
>> <matthias.bolte at googlemail.com> 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).


-- 
Doug Goldstein