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