2:31 a.m.
From: Alex Jia <ajia(a)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(a)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;
+ 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;
+ goto cleanup;
}
}
- if (!(ret = PyDict_New())) {
- VIR_FREE(stats);
- return VIR_PY_NONE;
+ if (!(info = PyDict_New())) {
+ ret = VIR_PY_NONE;
+ 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:
+ VIR_FREE(stats);
+ Py_XDECREF(key);
+ Py_XDECREF(val);
+ return NULL;
}
static PyObject *
--
1.7.1
4:08 a.m.
On 02/14/2012 09:31 AM, ajia(a)redhat.com wrote:
From: Alex Jia<ajia(a)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(a)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
4:48 a.m.
On 02/14/2012 06:08 PM, Peter Krempa wrote:
On 02/14/2012 09:31 AM, ajia(a)redhat.com wrote:
> From: Alex Jia<ajia(a)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(a)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
4:52 a.m.
On 02/14/2012 11:48 AM, Alex Jia wrote:
On 02/14/2012 06:08 PM, Peter Krempa wrote:
> On 02/14/2012 09:31 AM, ajia(a)redhat.com wrote:
>> From: Alex Jia<ajia(a)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(a)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
Relevant function header
>> static PyObject *
>> libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED,
>> PyObject *args)
>
> 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.
The return value of this function in case everything went OK actualy is
the dictionary Python object we construct in the code. Your "info"
variable actualy copies the function of ret.
Well, looking back at the code now, the simplification of cleanup
section probably won't be that easy, so you probably will have to use it
as an error section.
Peter
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
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:11 a.m.
On 02/14/2012 03:08 AM, Peter Krempa wrote:
On 02/14/2012 09:31 AM, ajia(a)redhat.com wrote:
> From: Alex Jia<ajia(a)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(a)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.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:55 a.m.
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(a)redhat.com wrote:
>> From: Alex Jia<ajia(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:50 a.m.
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(a)redhat.com>
To: "Peter Krempa" <pkrempa(a)redhat.com>
Cc: ajia(a)redhat.com, libvir-list(a)redhat.com
Sent: Tuesday, February 14, 2012 9:11:47 PM
Subject: Re: [libvirt] [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(a)redhat.com wrote:
> From: Alex Jia<ajia(a)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(a)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.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4695
days inactive
4695
days old
6 comments
4 participants
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Peter Krempa