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).
--
Doug Goldstein
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list