[libvirt] [PATCH] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats

Peter Krempa pkrempa at redhat.com
Tue Feb 14 13:55:22 UTC 2012


On 02/14/2012 02:11 PM, Eric Blake wrote:
> On 02/14/2012 03:08 AM, Peter Krempa wrote:
>> On 02/14/2012 09:31 AM, ajia at redhat.com wrote:
>>> From: Alex Jia<ajia at redhat.com>
>>>
>>> Detected by valgrind. Leaks are introduced in commit 17c7795.
>>>
>>> * python/libvirt-override.c (libvirt_virNodeGetMemoryStats): fix
>>> memory leaks
>>> and improve codes return value.
>>>
>>> For details, please see the following link:
>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=770944
>>>
>>> Signed-off-by: Alex Jia<ajia at redhat.com>
>>> ---
>>>    python/libvirt-override.c |   41
>>> ++++++++++++++++++++++++++++++-----------
>>>    1 files changed, 30 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
>>> index 4e8a97e..ecb11ea 100644
>>> --- a/python/libvirt-override.c
>>> +++ b/python/libvirt-override.c
>>> @@ -2426,7 +2426,9 @@ libvirt_virNodeGetCPUStats(PyObject *self
>>> ATTRIBUTE_UNUSED, PyObject *args)
>>>    static PyObject *
>>>    libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED,
>>> PyObject *args)
>>>    {
>>> -    PyObject *ret;
>>> +    PyObject *info, *ret;
>>
>> I'd initialize ret to VIR_PY_NONE here instead of doing it multiple
>> times later on.
>
> I'd actually initialize to NULL, not VIR_PY_NONE.  Use of VIR_PY_NONE
> increments the reference count on the Python None object; and if you
> then later return anything else, you would also have to reduce that
> reference count to avoid a resource leak in the form of an un-freeable
> python object.

Yes I realized that later on :( (after some reading on creation of 
python bindings :/)

>
>>
>> This label gets called only on a error path, so I'd call it "error"
>> instead of cleanup.
>>
>>> +    VIR_FREE(stats);
>>> +    Py_XDECREF(key);
>>> +    Py_XDECREF(val);
>>> +    return NULL;
>>
>> You're returning NULL instead of VIR_PY_NONE. (or the variable err)
>
> And I think that's actually correct!  Returning VIR_PY_NONE is a _valid_
> python object, and library code interprets it as "I successfully called
> your function, but had no object to return; now you can check for the
> libvirt error class to see why, but there is no python exception".
> Returning NULL means "I encountered a python exception; you can now
> catch that exception".  They are handled quite differently in the
> calling code.  Returning VIR_PY_NONE should be reserved for the case
> where we called a libvirt API that failed (then libvirt did indeed
> populate a libvirt error, and there is no python exception); while
> returning NULL should be reserved for the case where a python glue code
> failed (such as inability to create a new python dictionary) or where we
> explicitly raised a python exception (such as when we detect an OOM
> situation and call PyErr_NoMemory()).
>

I didn't notice this difference :( My review for V2 has still some 
issues then.


Peter

>
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list