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

[Guannan Ren]

On 03/17/2012 01:42 AM, Eric Blake wrote:
> 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.
>

      Thanks for these good suggested points, the v3 has been sent out 
based on them.

      Guannan Ren