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

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.