Re: [libvirt] [PATCH 1/3] esx: Wrap libcurl multi handle
2012/7/3 Doug Goldstein <cardoe(a)gentoo.org>:
On Mon, Jul 2, 2012 at 4:44 PM, Matthias Bolte
<matthias.bolte(a)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