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

[Matthias Bolte]

2012/7/3 Doug Goldstein <cardoe at gentoo.org>:
> 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).

Thanks, I pushed this one.

-- 
Matthias Bolte
http://photron.blogspot.com