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

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

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]