[libvirt] [PATCHv2 10/13] list: provide python bindings for snapshots

[Peter Krempa]

On 06/15/12 06:18, Eric Blake wrote:
> This adds support for the new virDomainListAllSnapshots (a domain
> function) and virDomainSnapshotListAllChildren (a snapshot function)
> to the libvirt-python bindings.  The implementation is done manually
> as the generator does not support wrapping lists of C pointers into
> python objects.
>
> * python/libvirt-override.c (libvirt_virDomainListAllSnapshots)
> (libvirt_virDomainSnapshotListAllChildren): New functions.
> * python/libvirt-override-api.xml: Document them.
> * python/libvirt-override-virDomain.py (listAllSnapshots): New
> file.
> * python/libvirt-override-virDomainSnapshot.py (listAllChildren):
> Likewise.
> * python/Makefile.am (CLASSES_EXTRA): Ship them.
> ---

> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> index 8d1ede1..4370045 100644
> --- a/python/libvirt-override.c
> +++ b/python/libvirt-override.c
> @@ -2127,6 +2127,51 @@ cleanup:
>   }
>
>   static PyObject *
> +libvirt_virDomainListAllSnapshots(PyObject *self ATTRIBUTE_UNUSED,
> +                                  PyObject *args)
> +{
> +    PyObject *py_retval = NULL;
> +    virDomainSnapshotPtr *snaps = NULL;
> +    int c_retval, i;
> +    virDomainPtr dom;
> +    PyObject *pyobj_dom;
> +    unsigned int flags;
> +    PyObject *pyobj_snap;
> +
> +    if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainListAllSnapshots",
> +                          &pyobj_dom, &flags))
> +        return NULL;
> +    dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom);
> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    c_retval = virDomainListAllSnapshots(dom, &snaps, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +    if (c_retval < 0)
> +        return VIR_PY_NONE;
> +
> +    if (!(py_retval = PyList_New(c_retval)))
> +        goto cleanup;
> +
> +    for (i = 0; i < c_retval; i++) {
> +        if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL ||
> +            PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
> +            Py_XDECREF(pyobj_snap);
> +            Py_DECREF(py_retval);
> +            py_retval = NULL;
> +            goto cleanup;
> +        }
> +        snaps[i] = NULL;
> +    }
> +
> +cleanup:
> +    for (i = 0; i < c_retval; i++)
> +        if (snaps[i])
> +            virDomainSnapshotFree(snaps[i]);
> +    VIR_FREE(snaps);
> +    return py_retval;
> +}
> +
> +static PyObject *
>   libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED,
>                                              PyObject *args)
>   {
> @@ -2181,6 +2226,51 @@ cleanup:
>   }
>
>   static PyObject *
> +libvirt_virDomainSnapshotListAllChildren(PyObject *self ATTRIBUTE_UNUSED,
> +                                         PyObject *args)
> +{
> +    PyObject *py_retval = NULL;
> +    virDomainSnapshotPtr *snaps = NULL;
> +    int c_retval, i;
> +    virDomainSnapshotPtr parent;
> +    PyObject *pyobj_parent;
> +    unsigned int flags;
> +    PyObject *pyobj_snap;
> +
> +    if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListAllChildren",
> +                          &pyobj_parent, &flags))
> +        return NULL;
> +    parent = (virDomainSnapshotPtr) PyvirDomainSnapshot_Get(pyobj_parent);
> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    c_retval = virDomainSnapshotListAllChildren(parent, &snaps, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +    if (c_retval < 0)
> +        return VIR_PY_NONE;
> +
> +    if (!(py_retval = PyList_New(c_retval)))
> +        goto cleanup;
> +
> +    for (i = 0; i < c_retval; i++) {
> +        if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL ||
> +            PyList_SetItem(py_retval, i, pyobj_snap) < 0) {

Sadly, this code pattern doesn't save us from leaking memory: The 
PyCapsule objects, that are used to wrap libvirt pointers lack a 
destructor, so if this whole function fails, calling Py_DECREF() on the 
resulting output list doesn't save us from leaking all the processed 
references that were stored in the List. Later on, when the list of 
PyCapsules is converted into actual python-libvirt objects, those 
objects contain destructors that dispose the pointers properly.

A workaround would be not to NULL members of the snap array and on 
successful end of the loop just set c_retval to 0 so that the cleanup 
loop is not called. But this still is not perfect.

To fix all possibilities of leaking these pointers, we will need to 
provide destructor callbacks for PyCapsules that wrap libvirt pointers 
and increase the reference count, when the Capsule object is created. 
This will allow to call free on all pointers and those that are stored 
in python structures will remain in memory thanks to the reference 
count. This will probably require to drop destructors on the 
libvirt-python wrapping objects, so when they get deleted, they use the 
destructor of the PyCapsule holding the actual pointer instead of the 
wrapping object's destructor that cleans the reference to 
domains/snapshots/...

This does not apply to libvirt_virDomainSnapshotListNames and others as 
strings are wrapped by python wrappers that (I assume) have destructors, 
but does apply on my domain listing bindings.

> +            Py_XDECREF(pyobj_snap);
> +            Py_DECREF(py_retval);
> +            py_retval = NULL;
> +            goto cleanup;
> +        }
> +        snaps[i] = NULL;
> +    }
> +
> +cleanup:
> +    for (i = 0; i < c_retval; i++)
> +        if (snaps[i])
> +            virDomainSnapshotFree(snaps[i]);
> +    VIR_FREE(snaps);
> +    return py_retval;
> +}
> +
> +static PyObject *
>   libvirt_virDomainRevertToSnapshot(PyObject *self ATTRIBUTE_UNUSED,
>                                     PyObject *args) {
>       int c_retval;
> @@ -5763,7 +5853,9 @@ static PyMethodDef libvirtMethods[] = {
>       {(char *) "virConnectBaselineCPU", libvirt_virConnectBaselineCPU, METH_VARARGS, NULL},
>       {(char *) "virDomainGetJobInfo", libvirt_virDomainGetJobInfo, METH_VARARGS, NULL},
>       {(char *) "virDomainSnapshotListNames", libvirt_virDomainSnapshotListNames, METH_VARARGS, NULL},
> +    {(char *) "virDomainListAllSnapshots", libvirt_virDomainListAllSnapshots, METH_VARARGS, NULL},
>       {(char *) "virDomainSnapshotListChildrenNames", libvirt_virDomainSnapshotListChildrenNames, METH_VARARGS, NULL},
> +    {(char *) "virDomainSnapshotListAllChildren", libvirt_virDomainSnapshotListAllChildren, METH_VARARGS, NULL},
>       {(char *) "virDomainRevertToSnapshot", libvirt_virDomainRevertToSnapshot, METH_VARARGS, NULL},
>       {(char *) "virDomainGetBlockJobInfo", libvirt_virDomainGetBlockJobInfo, METH_VARARGS, NULL},
>       {(char *) "virDomainSetBlockIoTune", libvirt_virDomainSetBlockIoTune, METH_VARARGS, NULL},
>

ACK, as the code follows a pattern, but memory leaks are possible yet 
very unlikely.

Peter