Re: [libvirt] [libvirt-python PATCH] override: add virDomainFSFreeze and virDomainFSThaw API
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)