[libvirt] [PATCH 2/5] python: add API exports for virConnectListAllDomains()

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120521/6772a5b2/attachment-0001.sig>