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

Peter Krempa pkrempa at redhat.com
Tue Feb 14 13:45:27 UTC 2012


On 02/14/2012 12:19 PM, 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>
> ---

--- see end of message for patch of suggested changes ---

>   python/libvirt-override.c |   43 +++++++++++++++++++++++++++++--------------
>   1 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> index 4e8a97e..594aae6 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;

I realized, that "VIR_PY_NONE" actualy increases the reference count on 
the "none" object. We should not do it unless we need the reference. Set 
ret to NULL please.

> +    PyObject *ret = VIR_PY_NONE;
> +    PyObject *key = NULL;
> +    PyObject *val = NULL;
>       PyObject *pyobj_conn;
>       virConnectPtr conn;
>       unsigned int flags;
> @@ -2442,32 +2444,45 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args)
>       c_retval = virNodeGetMemoryStats(conn, cellNum, NULL,&nparams, flags);
>       LIBVIRT_END_ALLOW_THREADS;
>       if (c_retval<  0)
> -        return VIR_PY_NONE;
> +        return ret;

Use "goto error;" here for consistency.

>
>       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;
> -        }
> -    }
> -    if (!(ret = PyDict_New())) {
> -        VIR_FREE(stats);
> -        return VIR_PY_NONE;
> +        if (c_retval<  0)
> +            goto error;
>       }
> +    if (!(ret = PyDict_New()))
> +        goto error;
> +
>       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 error;
> +
> +        if (PyDict_SetItem(ret, key, val)<  0) {
> +            Py_DECREF(ret);

Move this statement to the error section to free the returned dictionary 
even in case of other errors.

> +            goto error;
> +        }
> +
> +        Py_DECREF(key);
> +        Py_DECREF(val);
>       }
>
>       VIR_FREE(stats);
>       return ret;
> +
> +error:
> +    VIR_FREE(stats);
> +    Py_XDECREF(key);
> +    Py_XDECREF(val);
> +    return ret;

You'll need to return VIR_PY_NONE here as the

>   }
>
>   static PyObject *

ACK with those changes.

Here is a patch of changes that I suggest:

diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 594aae6..0409dd4 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -2426,7 +2426,7 @@ libvirt_virNodeGetCPUStats(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args)
  static PyObject *
  libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, 
PyObject *args)
  {
-    PyObject *ret = VIR_PY_NONE;
+    PyObject *ret = NULL;
      PyObject *key = NULL;
      PyObject *val = NULL;
      PyObject *pyobj_conn;
@@ -2444,7 +2444,7 @@ libvirt_virNodeGetMemoryStats(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args)
      c_retval = virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, 
flags);
      LIBVIRT_END_ALLOW_THREADS;
      if (c_retval < 0)
-        return ret;
+        goto error;

      if (nparams) {
          if (VIR_ALLOC_N(stats, nparams) < 0)
@@ -2466,10 +2466,8 @@ libvirt_virNodeGetMemoryStats(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args)
          if (!key || !val)
              goto error;

-        if (PyDict_SetItem(ret, key, val) < 0) {
-            Py_DECREF(ret);
+        if (PyDict_SetItem(ret, key, val) < 0)
              goto error;
-        }

          Py_DECREF(key);
          Py_DECREF(val);
@@ -2482,7 +2480,8 @@ error:
      VIR_FREE(stats);
      Py_XDECREF(key);
      Py_XDECREF(val);
-    return ret;
+    Py_XDECREF(ret);
+    return VIR_PY_NONE;
  }

  static PyObject *




More information about the libvir-list mailing list