2012/7/3 Doug Goldstein <cardoe@xxxxxxxxxx>:
> On Tue, Jul 3, 2012 at 3:17 PM, Matthias Bolte
> <matthias.bolte@xxxxxxxxxxxxxx> wrote:
>> 2012/7/3 Doug Goldstein <cardoe@xxxxxxxxxx>:
>>> On Mon, Jul 2, 2012 at 4:44 PM, Matthias Bolte
>>> <matthias.bolte@xxxxxxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list