Re: [libvirt] [PATCH 2/5] python: add API exports for virConnectListAllDomains()
On 05/20/2012 09:56 AM, Peter Krempa wrote:
This patch adds export of the new API function
virConnectListAllDomains() to the libvirt-python bindings. The
virConnect object now has method "listAllDomains" that takes only the
flags parameter and returns a python list of virDomain object
corresponding to virDomainPtrs returned by the underlying api.
The implementation is done manually as the generator does not support
wrapping list of virDomainPtrs into virDomain objects.
---
python/libvirt-override-api.xml | 12 ++++++--
python/libvirt-override-virConnect.py | 12 ++++++++
python/libvirt-override.c | 49 ++++++++++++++++++++++++++++++++-
3 files changed, 69 insertions(+), 4 deletions(-)
Yep, definitely has to be an override function.
- <return type='str *' info='the list of Names of
None in case of error'/>
+ <return type='str *' info='the list of Names or None in case of
error'/>
Cute typo fix. Took me a while to spot it.
+
+ def listAllDomains(self, flags):
+ """List all domains and returns a list of domain
objects"""
+ ret = libvirtmod.virConnectListAllDomains(self._o, flags)
+ if ret is None:
+ raise libvirtError("virConnectListAllDomains() failed",
conn=self)
+
+ retlist = list()
+ for domptr in ret:
+ retlist.append(virDomain(self, _obj=domptr))
I'm not familiar enough with python to know if this does the right
thing, but I hope it does. You got further than I did in my simple RFC :)
static PyObject *
+libvirt_virConnectListAllDomains(PyObject *self ATTRIBUTE_UNUSED,
+ PyObject *args)
+{
+ PyObject *pyobj_conn;
+ PyObject *py_retval = NULL;
+ virConnectPtr conn;
+ virDomainPtr *doms = NULL;
+ int c_retval = 0;
+ int i;
+ unsigned int flags;
+
+ if (!PyArg_ParseTuple(args, (char *)"Oi:virConnectListAllDomains",
+ &pyobj_conn, &flags))
+ return NULL;
+ conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
What happens if PyvirConnect_Get() fails? Can it return NULL? While
virConnectListAllDomains happens to deal with NULL (by failing the API),
I can't help but wonder if we should be more careful here. Then again,
that's probably a common problem in the python bindings, where you are
just copying from other clients doing the same. I guess nothing needs
to change here.
+
+ LIBVIRT_BEGIN_ALLOW_THREADS;
+ c_retval = virConnectListAllDomains(conn, &doms, -1, flags);
+ LIBVIRT_END_ALLOW_THREADS;
+ if (c_retval < 0)
+ return VIR_PY_NONE;
+
+ if (!(py_retval = PyList_New(c_retval)))
+ goto error;
+
+ for (i = 0; i < c_retval; i++) {
+ if (PyList_SetItem(py_retval, i,
+ libvirt_virDomainPtrWrap(doms[i])) < 0)
Does libvirt_virDomainPtrWrap create a new object from scratch, or is it
taking a reference to doms[i]? If the former, then we need to call
virDomainFree(doms[i]) unconditionally; if the latter, then we need to
set doms[i] to NULL on success. [Looks like the latter.]
+ goto error;
+ }
+
+ return py_retval;
Wrong. This leaks memory, of at least doms. Instead, you need skip to:
+
+error:
+ Py_XDECREF(py_retval);
+
a cleanup label here.
+ if (doms && c_retval >= 0)
We already guaranteed doms was non-NULL and c_retval >= 0 if we get here.
+ for (i = 0; i < c_retval; i++)
+ if (doms[i])
+ virDomainFree(doms[i]);
+
Memory leak: you are missing a VIR_FREE(doms).
+
+ return NULL;
I think this control flow solves it:
if (!(py_retval = PyList_New(c_retval)))
goto cleanup;
for (i = 0; i < c_retval; i++) {
if (PyList_SetItem(py_retval, i,
libvirt_virDomainPtrWrap(doms[i])) < 0) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
doms[i] = NULL;
}
cleanup:
for (i = 0; i < c_retval; i++)
if (doms[i])
virDomainFree(doms[i]);
VIR_FREE(doms).
return py_retval;
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)