On 10.05.2014 01:21, Tomoki Sekiyama wrote:
> Add binding for the new virDomainFSFreeze and virDomainFSThaw functions
> added in libvirt 1.2.5. These require override since these take a list
> of mountpoints path string. The methods are named 'fSFreeze' and 'fSThaw',
> like a existing 'fSTrim' method.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiy...@hds.com>
> ---
>   generator.py             |    2 +
>   libvirt-override-api.xml |   14 +++++++
>   libvirt-override.c       |   97 
> ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 113 insertions(+)
> 

Funny, I've started to write this prior to seeing your patch. I've ended up 
more or less with the same.

> diff --git a/generator.py b/generator.py
> index 05ec743..eec81b0 100755
> --- a/generator.py
> +++ b/generator.py
> @@ -462,6 +462,8 @@ skip_impl = (
>       'virDomainMigrate3',
>       'virDomainMigrateToURI3',
>       'virConnectGetCPUModelNames',
> +    'virDomainFSFreeze',
> +    'virDomainFSThaw',
>   )
>   
>   lxc_skip_impl = (
> diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
> index d5b25b5..a3db33e 100644
> --- a/libvirt-override-api.xml
> +++ b/libvirt-override-api.xml
> @@ -624,5 +624,19 @@
>         <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor 
> connection'/>
>         <arg name='flags' type='int' info='unused, pass 0'/>
>       </function>
> +    <function name='virDomainFSFreeze' file='python'>
> +      <info>Freeze specified filesystems within the guest</info>
> +      <return type='int' info='the number of frozen filesystems on success, 
> -1 otherwise.'/>
> +      <arg name='dom' type='virDomainPtr' info='a domain object'/>
> +      <arg name='mountpoints' type='const char **' info='list of mount 
> points to be frozen, or None'/>

1: ^^^

> +      <arg name='flags' type='unsigned int' info='unused, pass 0'/>
> +    </function>
> +    <function name='virDomainFSThaw' file='python'>
> +      <info>Thaw specified filesystems within the guest</info>
> +      <return type='int' info='the number of thawed filesystems on success, 
> -1 otherwise.'/>
> +      <arg name='dom' type='virDomainPtr' info='a domain object'/>
> +      <arg name='mountpoints' type='const char **' info='list of mount 
> points to be thawed, or None'/>

2: ^^^^

> +      <arg name='flags' type='unsigned int' info='unused, pass 0'/>
> +    </function>

In both [1] and [2] I suggest to add 'optional' somewhere in the @info 
attribute. It has a nice advantage (*) that one can simply call:
   dom.fsFreeze()
   dom.fsThaw()

to freeze/thaw all filesystems. 

* - while being a nice advantage, it's hackish too. Long story short, in the 
method definition, the generator will supply the implicit value to mountpoints 
argument:

    def fSFreeze(self, mountpoints=None, flags=0):
        """Freeze specified filesystems within the guest. """
        ret = libvirtmod.virDomainFSFreeze(self._o, mountpoints, flags)

>     </symbols>
>   </api>
> diff --git a/libvirt-override.c b/libvirt-override.c
> index 3fa9b9b..d0557c2 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -7554,6 +7554,99 @@ cleanup:
>   #endif /* LIBVIR_CHECK_VERSION(1, 1, 1) */
>   
>   
> +#if LIBVIR_CHECK_VERSION(1, 2, 5)
> +static PyObject *
> +libvirt_virDomainFSFreeze(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
> +    PyObject *py_retval = NULL;
> +    int c_retval;
> +    virDomainPtr domain;
> +    PyObject *pyobj_domain;
> +    PyObject *pyobj_list;
> +    unsigned int flags;
> +    unsigned int nmountpoints = 0;
> +    const char **mountpoints = NULL;
> +    size_t i = 0, j;
> +
> +    if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainFSFreeze",
> +                          &pyobj_domain, &pyobj_list, &flags))
> +        return NULL;
> +    domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
> +
> +    if (PyList_Check(pyobj_list)) {
> +        nmountpoints = PyList_Size(pyobj_list);
> +
> +        if (VIR_ALLOC_N(mountpoints, nmountpoints) < 0)
> +            return PyErr_NoMemory();
> +
> +        for (i = 0; i < nmountpoints; i++) {
> +            if (libvirt_charPtrUnwrap(PyList_GetItem(pyobj_list, i),
> +                                      (char **)mountpoints+i) < 0 ||
> +                mountpoints[i] == NULL)
> +                goto cleanup;
> +        }
> +    }
> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    c_retval = virDomainFSFreeze(domain, mountpoints, nmountpoints, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +    py_retval = libvirt_intWrap((int) c_retval);
> +
> +cleanup:
> +    if (mountpoints) {

This if is useless.

> +        for (j = 0 ; j < i ; j++)
> +            VIR_FREE(mountpoints[j]);
> +        VIR_FREE(mountpoints);

Ouch, freeing a const char? I know you are doing it to shut the gcc up, but I'd 
rather typecast @mountpoints in the API call a few lines above and satisfy 
const-correctness.

> +    }
> +    return py_retval;

No, you are saying in -api.xml that you'll return -1 on error. But you're 
returning NULL. Moreover, I think we should return None rather than -1 if 
that's the case.

> +}
> +
> +
> +static PyObject *
> +libvirt_virDomainFSThaw(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
> +    PyObject *py_retval = NULL;
> +    int c_retval;
> +    virDomainPtr domain;
> +    PyObject *pyobj_domain;
> +    PyObject *pyobj_list;
> +    unsigned int flags;
> +    unsigned int nmountpoints = 0;
> +    const char **mountpoints = NULL;
> +    size_t i = 0, j;
> +
> +    if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainFSThaw",
> +                          &pyobj_domain, &pyobj_list, &flags))
> +        return NULL;
> +    domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
> +
> +    if (PyList_Check(pyobj_list)) {
> +        nmountpoints = PyList_Size(pyobj_list);
> +
> +        if (VIR_ALLOC_N(mountpoints, nmountpoints) < 0)
> +            return PyErr_NoMemory();
> +
> +        for (i = 0; i < nmountpoints; i++) {
> +            if (libvirt_charPtrUnwrap(PyList_GetItem(pyobj_list, i),
> +                                      (char **)mountpoints+i) < 0 ||
> +                mountpoints[i] == NULL)
> +                goto cleanup;
> +        }
> +    }
> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    c_retval = virDomainFSThaw(domain, mountpoints, nmountpoints, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +    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;
> +}
> +#endif /* LIBVIR_CHECK_VERSION(1, 2, 5) */
> +
>   /************************************************************************
>    *                                                                  *
>    *                  The registration stuff                          *
> @@ -7729,6 +7822,10 @@ static PyMethodDef libvirtMethods[] = {
>       {(char *) "virDomainCreateXMLWithFiles", 
> libvirt_virDomainCreateXMLWithFiles, METH_VARARGS, NULL},
>       {(char *) "virDomainCreateWithFiles", libvirt_virDomainCreateWithFiles, 
> METH_VARARGS, NULL},
>   #endif /* LIBVIR_CHECK_VERSION(1, 1, 1) */
> +#if LIBVIR_CHECK_VERSION(1, 2, 5)
> +    {(char *) "virDomainFSFreeze", libvirt_virDomainFSFreeze, METH_VARARGS, 
> NULL},
> +    {(char *) "virDomainFSThaw", libvirt_virDomainFSThaw, METH_VARARGS, 
> NULL},
> +#endif /* LIBVIR_CHECK_VERSION(1, 2, 5) */
>       {NULL, NULL, 0, NULL}
>   };

So I guess it's ACK with this squashed in:


diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
index a3db33e..f6af2ed 100644
--- a/libvirt-override-api.xml
+++ b/libvirt-override-api.xml
@@ -626,17 +626,17 @@
     </function>
     <function name='virDomainFSFreeze' file='python'>
       <info>Freeze specified filesystems within the guest</info>
-      <return type='int' info='the number of frozen filesystems on success, -1 
otherwise.'/>
+      <return type='int' info='the number of frozen filesystems on success, 
None otherwise.'/>
       <arg name='dom' type='virDomainPtr' info='a domain object'/>
-      <arg name='mountpoints' type='const char **' info='list of mount points 
to be frozen, or None'/>
-      <arg name='flags' type='unsigned int' info='unused, pass 0'/>
+      <arg name='mountpoints' type='const char **' info='optional list of 
mount points to be frozen'/>
+      <arg name='flags' type='unsigned int' info='extra flags'/>
     </function>
     <function name='virDomainFSThaw' file='python'>
       <info>Thaw specified filesystems within the guest</info>
-      <return type='int' info='the number of thawed filesystems on success, -1 
otherwise.'/>
+      <return type='int' info='the number of thawed filesystems on success, 
None otherwise.'/>
       <arg name='dom' type='virDomainPtr' info='a domain object'/>
-      <arg name='mountpoints' type='const char **' info='list of mount points 
to be thawed, or None'/>
-      <arg name='flags' type='unsigned int' info='unused, pass 0'/>
+      <arg name='mountpoints' type='const char **' info='optional list of 
mount points to be thawed'/>
+      <arg name='flags' type='unsigned int' info='extra flags'/>
     </function>
   </symbols>
 </api>
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;
-    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;
     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...

Michal

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

Reply via email to