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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list