3:52 p.m.
I noticed these while working on my new list all snapshots code,
but these are fairly independent and can be applied in any order.
Eric Blake (2):
python: use simpler methods
python: fix snapshot listing bugs
python/libvirt-override.c | 70 ++++++++++++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 27 deletions(-)
--
1.7.10.2
3:52 p.m.
New subject: [libvirt] [PATCH 1/2] python: use simpler methods
* python/libvirt-override.c (libvirt_virDomainGetVcpus)
(libvirt_virDomainGetVcpuPinInfo): Use Py_XDECREF instead of
open-coding it.
---
python/libvirt-override.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 130e702..da5cb9a 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -1415,15 +1415,9 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
cleanup:
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
- /* NB, Py_DECREF is a badly defined macro, so we require
- * braces here to avoid 'ambiguous else' warnings from
- * the compiler.
- * NB. this comment is true at of time of writing wrt to
- * at least python2.5.
- */
- if (pyretval) { Py_DECREF(pyretval); }
- if (pycpuinfo) { Py_DECREF(pycpuinfo); }
- if (pycpumap) { Py_DECREF(pycpumap); }
+ Py_XDECREF(pyretval);
+ Py_XDECREF(pycpuinfo);
+ Py_XDECREF(pycpumap);
return VIR_PY_NONE;
}
@@ -1584,7 +1578,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED,
cleanup:
VIR_FREE(cpumaps);
- if (pycpumaps) { Py_DECREF(pycpumaps);}
+ Py_XDECREF(pycpumaps);
return VIR_PY_NONE;
}
--
1.7.10.2
3:52 p.m.
New subject: [libvirt] [PATCH 2/2] python: fix snapshot listing bugs
Python exceptions are different than libvirt errors, and we had
some corner case bugs on OOM situations.
* python/libvirt-override.c (libvirt_virDomainSnapshotListNames)
(libvirt_virDomainSnapshotListChildrenNames): Use correct error
returns, avoid segv on OOM, and avoid memory leaks on error.
---
python/libvirt-override.c | 56 +++++++++++++++++++++++++++++++--------------
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index da5cb9a..676002c 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -2025,15 +2025,18 @@ libvirt_virConnectListDefinedDomains(PyObject *self
ATTRIBUTE_UNUSED,
static PyObject *
libvirt_virDomainSnapshotListNames(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
PyObject *py_retval;
char **names = NULL;
int c_retval, i;
virDomainPtr dom;
PyObject *pyobj_dom;
+ PyObject *pyobj_snap;
unsigned int flags;
- if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListNames",
&pyobj_dom, &flags))
+ if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListNames",
+ &pyobj_dom, &flags))
return NULL;
dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom);
@@ -2045,7 +2048,7 @@ libvirt_virDomainSnapshotListNames(PyObject *self ATTRIBUTE_UNUSED,
if (c_retval) {
if (VIR_ALLOC_N(names, c_retval) < 0)
- return VIR_PY_NONE;
+ return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virDomainSnapshotListNames(dom, names, c_retval, flags);
LIBVIRT_END_ALLOW_THREADS;
@@ -2055,21 +2058,31 @@ libvirt_virDomainSnapshotListNames(PyObject *self
ATTRIBUTE_UNUSED,
}
}
py_retval = PyList_New(c_retval);
+ if (!py_retval)
+ goto cleanup;
- if (names) {
- for (i = 0;i < c_retval;i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
- VIR_FREE(names[i]);
+ for (i = 0; i < c_retval; i++) {
+ if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL ||
+ PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
+ Py_XDECREF(pyobj_snap);
+ Py_DECREF(py_retval);
+ py_retval = NULL;
+ goto cleanup;
}
- VIR_FREE(names);
+ VIR_FREE(names[i]);
}
+cleanup:
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
return py_retval;
}
static PyObject *
libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
PyObject *py_retval;
char **names = NULL;
int c_retval, i;
@@ -2077,7 +2090,8 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
PyObject *pyobj_snap;
unsigned int flags;
- if (!PyArg_ParseTuple(args, (char
*)"Oi:virDomainSnapshotListChildrenNames", &pyobj_snap, &flags))
+ if (!PyArg_ParseTuple(args, (char
*)"Oi:virDomainSnapshotListChildrenNames",
+ &pyobj_snap, &flags))
return NULL;
snap = (virDomainSnapshotPtr) PyvirDomainSnapshot_Get(pyobj_snap);
@@ -2089,9 +2103,10 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
if (c_retval) {
if (VIR_ALLOC_N(names, c_retval) < 0)
- return VIR_PY_NONE;
+ return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS;
- c_retval = virDomainSnapshotListChildrenNames(snap, names, c_retval, flags);
+ c_retval = virDomainSnapshotListChildrenNames(snap, names, c_retval,
+ flags);
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0) {
VIR_FREE(names);
@@ -2100,14 +2115,21 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
}
py_retval = PyList_New(c_retval);
- if (names) {
- for (i = 0;i < c_retval;i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
- VIR_FREE(names[i]);
+ for (i = 0; i < c_retval; i++) {
+ if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL ||
+ PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
+ Py_XDECREF(pyobj_snap);
+ Py_DECREF(py_retval);
+ py_retval = NULL;
+ goto cleanup;
}
- VIR_FREE(names);
+ VIR_FREE(names[i]);
}
+cleanup:
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
return py_retval;
}
--
1.7.10.2
4:12 a.m.
New subject: [libvirt] [PATCH 2/2] python: fix snapshot listing bugs
On 06/11/12 22:52, Eric Blake wrote:
Python exceptions are different than libvirt errors, and we had
some corner case bugs on OOM situations.
* python/libvirt-override.c (libvirt_virDomainSnapshotListNames)
(libvirt_virDomainSnapshotListChildrenNames): Use correct error
returns, avoid segv on OOM, and avoid memory leaks on error.
---
python/libvirt-override.c | 56 +++++++++++++++++++++++++++++++--------------
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index da5cb9a..676002c 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -2045,7 +2048,7 @@ libvirt_virDomainSnapshotListNames(PyObject
*self ATTRIBUTE_UNUSED,
if (c_retval) {
if (VIR_ALLOC_N(names, c_retval) < 0)
- return VIR_PY_NONE;
+ return PyErr_NoMemory();
I'll have to tweak my domain list python bindings to use this func too.
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virDomainSnapshotListNames(dom, names, c_retval, flags);
LIBVIRT_END_ALLOW_THREADS;
+cleanup:
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
Probably not worth optimizing this not to iterate the array on success.
return py_retval;
}
ACK.
Peter
7:46 a.m.
New subject: [libvirt] [PATCH 2/2] python: fix snapshot listing bugs
On 06/12/2012 03:12 AM, Peter Krempa wrote:
On 06/11/12 22:52, Eric Blake wrote:
> Python exceptions are different than libvirt errors, and we had
> some corner case bugs on OOM situations.
>
> * python/libvirt-override.c (libvirt_virDomainSnapshotListNames)
> (libvirt_virDomainSnapshotListChildrenNames): Use correct error
> returns, avoid segv on OOM, and avoid memory leaks on error.
> +cleanup:
> + for (i = 0; i < c_retval; i++)
> + VIR_FREE(names[i]);
> + VIR_FREE(names);
Probably not worth optimizing this not to iterate the array on success.
The old code had one iteration through the array, but if it failed
halfway through, leaked the other half of the array. The new code
separates the iterations on purpose, so that the cleanup happens
regardless of success or failure.
> return py_retval;
> }
>
ACK.
Thanks; series pushed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4547
days inactive
4548
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa