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 =¶ms[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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list