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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org