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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list