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

Peter Krempa pkrempa at redhat.com
Tue Feb 14 10:08:42 UTC 2012


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.

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




More information about the libvir-list mailing list