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

3 Jul 2012


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