[libvirt] [PATCH v2] python: add virDomainGetCPUStats python binding API

[Eric Blake]

On 03/16/2012 11:26 AM, Eric Blake wrote:
> 
>> +
>> +        if (!nparams) {
>> +            if ((cpu = PyDict_New()) == NULL) {
>> +                error = NULL;
> 
> Initialize error to NULL at the front, and you won't have to set it here.
> 
>> +                goto failed;
>> +            }
>> +
>> +            if (PyList_Append(ret, cpu) < 0) {
>> +                Py_DECREF(cpu);
>> +                error = NULL;
>> +                goto failed;
>> +            }
>> +
>> +            Py_DECREF(cpu);
>> +            return ret;
> 
> So why are we populating the list with a single empty dictionary?
> Shouldn't we instead be populating it with one empty dictionary per cpu?
>  That is, this code returns [{}], but if ncpus is 4, it should return
> [{},{},{},{}].

In fact, instead of returning early here, you can just say:

if (nparams) {

// get parameters, via for loop over...

> 
>> +        }
>> +        sumparams = nparams * ncpus;
>> +
>> +        if (VIR_ALLOC_N(params, sumparams) < 0) {
>> +            error = PyErr_NoMemory();
>> +            goto failed;
>> +        }
>> +
>> +        LIBVIRT_BEGIN_ALLOW_THREADS;
>> +        i_retval = virDomainGetCPUStats(domain, params, nparams, 0, ncpus, flags);
> 
> This will fail if ncpus > 128.  You have to do this in a for loop,
> grabbing only 128 cpus per iteration.
> 
>> +        LIBVIRT_END_ALLOW_THREADS;
>> +
>> +        if (i_retval < 0) {
>> +            error = VIR_PY_NONE;
>> +            goto cleanup;
>> +        }

} else {
    i_retval = 0;
}

>> +
>> +        for (i = 0; i < ncpus; i++) {
>> +            if (params[i * nparams].type == 0)
>> +                continue;
> 
> Technically, this is only possible if you called virDomainGetCPUStats
> with ncpus larger than what the hypervisor already told you to use.  But
> I guess it doesn't hurt to leave these two lines in.

then drop these two lines after all (since in the i_retval==0 case,
params might be NULL and this would be a NULL deref),

> 
>> +
>> +            cpuparams = &params[i * nparams];

This is just an address computation, so it doesn't matter if params is NULL.

>> +            if ((cpu = getPyVirTypedParameter(cpuparams, nparams)) == NULL) {
> 
> Here, you want to iterate over the number of returned parameters, not
> the number of array slots you passed in.  s/nparams/i_retval/

and this will properly create your empty dictionary, since
getPyVirTypedParameter does the right thing if i_retval is 0.  That way,
your PyList_Append() code can be reused to populate the correct return
value for ncpus regardless of whether nparams was 0.

-- 
Eric Blake   eblake at 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: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120316/debdc87d/attachment-0001.sig>