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

[Alex Jia]

On 02/14/2012 06:08 PM, 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.
>
>> +    PyObject *key = NULL;
>> +    PyObject *val = NULL;
>>       PyObject *pyobj_conn;
>>       virConnectPtr conn;
>>       unsigned int flags;
>> @@ -2446,28 +2448,45 @@ libvirt_virNodeGetMemoryStats(PyObject *self 
>> ATTRIBUTE_UNUSED, PyObject *args)
>>
>>       if (nparams) {
>>           if (VIR_ALLOC_N(stats, nparams)<  0)
>> -            return VIR_PY_NONE;
>> +            return PyErr_NoMemory();
>>
>>           LIBVIRT_BEGIN_ALLOW_THREADS;
>>           c_retval = virNodeGetMemoryStats(conn, cellNum, 
>> stats,&nparams, flags);
>>           LIBVIRT_END_ALLOW_THREADS;
>>           if (c_retval<  0) {
>> -            VIR_FREE(stats);
>> -            return VIR_PY_NONE;
>> +            ret = VIR_PY_NONE;
>
> Like here.
>
>> +            goto cleanup;
>>           }
>>       }
>> -    if (!(ret = PyDict_New())) {
>> -        VIR_FREE(stats);
>> -        return VIR_PY_NONE;
>> +    if (!(info = PyDict_New())) {
>> +        ret = VIR_PY_NONE;
>
> And here
>
>> +        goto cleanup;
>>       }
>> +
>>       for (i = 0; i<  nparams; i++) {
>> -        PyDict_SetItem(ret,
>> -                       libvirt_constcharPtrWrap(stats[i].field),
>> -                       libvirt_ulonglongWrap(stats[i].value));
>> +        key = libvirt_constcharPtrWrap(stats[i].field);
>> +        val = libvirt_ulonglongWrap(stats[i].value);
>> +
>> +        if (!key || !val)
>> +            goto cleanup;
>> +
>> +        if (PyDict_SetItem(info, key, val)<  0) {
>> +            Py_DECREF(info);
>> +            goto cleanup;
>> +        }
>> +
>> +        Py_DECREF(key);
>> +        Py_DECREF(val);
>>       }
>>
>>       VIR_FREE(stats);
>> -    return ret;
>> +    return info;
>> +
>> +cleanup:
>
> 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)
>
>>   }
>>
>>   static PyObject *
>
> With code like this, the variable err isn't used anywhere.
>
> Introduction of the new variable "info" is probably not needed, and 
> you may use "ret" for this purpose.
If so, the 'ret' will have 2 different function, the one is return 
value, the other is dictionary object,
I'm not sure it's a good idea to override previous variable 'ret', 
although the codes are quite simple.

Thanks for your comment,
Alex
>
> It should be possible to rewrite the code (especialy the cleanup 
> section) so that also the success path may pass that way. That would 
> simplify the code.
>
> Peter