Re: [libvirt] [libvirt-python PATCH] override: add virDomainFSFreeze and virDomainFSThaw API

12 May 2014

Thanks for your review, and a lot of fixes :) .

Just a comment...
...
On 10.05.2014 01:21, Tomoki Sekiyama wrote:

diff --git a/libvirt-override.c b/libvirt-override.c
index d0557c2..d08b271 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7564,7 +7564,7 @@ libvirt_virDomainFSFreeze(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
    PyObject *pyobj_list;
    unsigned int flags;
    unsigned int nmountpoints = 0;
-    const char **mountpoints = NULL;
+    char **mountpoints = NULL;
    size_t i = 0, j;
if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainFSFreeze",
@@ -7580,24 +7580,23 @@ libvirt_virDomainFSFreeze(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
for (i = 0; i < nmountpoints; i++) {
            if (libvirt_charPtrUnwrap(PyList_GetItem(pyobj_list, i),
-                                      (char **)mountpoints+i) < 0 ||
+                                      mountpoints+i) < 0 ||
                mountpoints[i] == NULL)
                goto cleanup;
        }
    }
LIBVIRT_BEGIN_ALLOW_THREADS;
-    c_retval = virDomainFSFreeze(domain, mountpoints, nmountpoints, flags);
+    c_retval = virDomainFSFreeze(domain, (const char **) mountpoints,
+                                 nmountpoints, flags);
    LIBVIRT_END_ALLOW_THREADS;
Maybe we should add here:

    if (c_retval >= 0)

to return None when API returns an error, so that it adopts to the description.
...
-    py_retval = libvirt_intWrap((int) c_retval);
+    py_retval = libvirt_intWrap(c_retval);
cleanup:
-    if (mountpoints) {
-        for (j = 0 ; j < i ; j++)
-            VIR_FREE(mountpoints[j]);
-        VIR_FREE(mountpoints);
-    }
-    return py_retval;
+    for (j = 0 ; j < i ; j++)
+        VIR_FREE(mountpoints[j]);
+    VIR_FREE(mountpoints);
+    return py_retval ? py_retval : VIR_PY_NONE;
}
@@ -7610,7 +7609,7 @@ libvirt_virDomainFSThaw(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
    PyObject *pyobj_list;
    unsigned int flags;
    unsigned int nmountpoints = 0;
-    const char **mountpoints = NULL;
+    char **mountpoints = NULL;
    size_t i = 0, j;
if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainFSThaw",
@@ -7626,24 +7625,23 @@ libvirt_virDomainFSThaw(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
for (i = 0; i < nmountpoints; i++) {
            if (libvirt_charPtrUnwrap(PyList_GetItem(pyobj_list, i),
-                                      (char **)mountpoints+i) < 0 ||
+                                      mountpoints+i) < 0 ||
                mountpoints[i] == NULL)
                goto cleanup;
        }
    }
LIBVIRT_BEGIN_ALLOW_THREADS;
-    c_retval = virDomainFSThaw(domain, mountpoints, nmountpoints, flags);
+    c_retval = virDomainFSThaw(domain, (const char **) mountpoints,
+                               nmountpoints, flags);
    LIBVIRT_END_ALLOW_THREADS;
And here too.
...
py_retval = libvirt_intWrap((int) c_retval);
cleanup:
-    if (mountpoints) {
-        for (j = 0 ; j < i ; j++)
-            VIR_FREE(mountpoints[j]);
-        VIR_FREE(mountpoints);
-    }
-    return py_retval;
+    for (j = 0 ; j < i ; j++)
+        VIR_FREE(mountpoints[j]);
+    VIR_FREE(mountpoints);
+    return py_retval ? py_retval : VIR_PY_NONE;
}
#endif /* LIBVIR_CHECK_VERSION(1, 2, 5) */
Having said that, I'll wait a few moments prior squashing that in and pushing just to allow anybody else to express their opinion. For example, why gcc doesn't like if char ** is passed to a const char ** argument...
Regards,
Tomoki Sekiyama

    