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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Eric,
Thanks for your comment on v1 and v2 patch, I will update v3 based on v1
according to your advise.

Regards,
Alex


----- Original Message -----
From: "Eric Blake" <eblake@xxxxxxxxxx>
To: "Peter Krempa" <pkrempa@xxxxxxxxxx>
Cc: ajia@xxxxxxxxxx, libvir-list@xxxxxxxxxx
Sent: Tuesday, February 14, 2012 9:11:47 PM
Subject: Re:  [PATCH] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats

On 02/14/2012 03:08 AM, Peter Krempa wrote:
> On 02/14/2012 09:31 AM, ajia@xxxxxxxxxx wrote:
>> From: Alex Jia<ajia@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>>   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.

> 
> 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()).

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]