On 10.05.2014 01:21, Tomoki Sekiyama wrote: Funny, I've started to write this prior to seeing your patch. I've ended up more or less with the same. 1: ^^^ 2: ^^^^ 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) This if is useless. 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. 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) { > +} > +#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

On 12.05.2014 17:13, Tomoki Sekiyama wrote: Ah, you're obviously right. I'd just rather do: if (c_retval < 0) goto cleanup; Anyway, fixed in my local repo. Michal