[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]