Re: [libvirt] [PATCH 1/3] esx: Wrap libcurl multi handle
On Tue, Jul 3, 2012 at 3:17 PM, Matthias Bolte
<matthias.bolte(a)googlemail.com> wrote:
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
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