[libvirt] [libvirt-python PATCH] override: add virDomainFSFreeze and virDomainFSThaw API
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.sekiyama@hds.com> --- generator.py | 2 + libvirt-override-api.xml | 14 +++++++ libvirt-override.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+) 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'/> + <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'/> + <arg name='flags' type='unsigned int' info='unused, pass 0'/> + </function> </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) { + for (j = 0 ; j < i ; j++) + VIR_FREE(mountpoints[j]); + VIR_FREE(mountpoints); + } + return py_retval; +} + + +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} };
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.sekiyama@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
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
On 12.05.2014 17:13, Tomoki Sekiyama wrote:
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)
Ah, you're obviously right. I'd just rather do: if (c_retval < 0) goto cleanup; Anyway, fixed in my local repo. Michal
On 05/12/2014 07:22 AM, Michal Privoznik wrote:
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.
Eww. I'd rather 'fsFreeze' and 'fsThaw' - how baked in is the 'fSTrim' naming?
+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;
This should be '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 ||
...so you can drop the cast here...
+ mountpoints[i] == NULL) + goto cleanup; + } + } + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainFSFreeze(domain, mountpoints, nmountpoints, flags);
...at the expense of needing one here.
+ 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.
It's not just gcc, but ALL C compilers. The C language mandates this to be an error, although it drives me crazy. Quoting from C99 6.5.16.1 para 6: EXAMPLE 3 Consider the fragment: const char **cpp; char *p; const char c = 'A'; cpp = &p; //constraint violation *cpp = &c; //valid *p = 0; //valid The first assignment is unsafe because it would allow the following valid code to attempt to change the value of the const object c.
+ } + 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.
The C code returns an integer, and the python code should do likewise. But remember, in python, you return either 'NULL' (witness that a python exception needs to be raised) or a pointer to a python integer object. The return looks correct to me.
+++ 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.'/>
NACK to this hunk. If you return an integer on success, you must return -1 on failure. The generator does not do well with a mix of python integer vs. non-integer python struct - None is only paired with non-integer returns.
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...
(Any reason you didn't have your mailer wrap lines?) Probably best to send a v2 at this point. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, May 12, 2014 at 05:12:45PM -0600, Eric Blake wrote:
On 05/12/2014 07:22 AM, Michal Privoznik wrote:
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.
Eww. I'd rather 'fsFreeze' and 'fsThaw' - how baked in is the 'fSTrim' naming?
The existing APIs are broken since the generator doesn't cope with the input parameters, so there's no issue changing the name. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Tomoki Sekiyama