[libvirt] [libvirt-python PATCH] override: add virDomainFSFreeze and virDomainFSThaw API

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140512/17331ab0/attachment-0001.sig>