On 03/14/2012 07:03 AM, Guannan Ren wrote:
> dom.getCPUStats(True, 0)
> [{'cpu_time': 92913537401L, 'system_time': 5470000000L, 'user_time': 310000000L}]
>
> dom.getCPUStats(False, 0)
> [{'cpu_time': 39476858499L}, {'cpu_time': 10627048370L}, {'cpu_time': 21270945682L}, {'cpu_time': 21556420641L}]
>
> *generator.py Add a new naming rule
> *libvirt-override-api.xml The API function description
> *libvirt-override.c Implement it.
> ---
> python/generator.py | 5 +-
> python/libvirt-override-api.xml | 10 +++
> python/libvirt-override.c | 164 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 178 insertions(+), 1 deletions(-)
>
> +++ b/python/libvirt-override-api.xml
> @@ -149,6 +149,16 @@
> <arg name='path' type='char *' info='the path for the block device'/>
> <arg name='flags' type='int' info='flags (unused; pass 0)'/>
> </function>
> + <function name='virDomainGetCPUStats' file='python'>
> + <info>Extracts CPU statistics for a running domain, On success it will return a list of data of dictionary type.
s/, On/. On/
Long lines; can you wrap this to fit in 80 columns?
> + If boolean total is True, the first element of the list refers to CPU0 on the host, second element is CPU1, and so on.
s/total is True/total is False/
> + The format of data struct is like [{cpu_time:xxx},{cpu_time:xxx}, ...]
> + If it is False, it returns total domain CPU statistics like [{cpu_time:xxx, user_time:xxx, system_time:xxx}]</info>
s/False/True/
> +
> + if (!PyBool_Check(totalbool)) {
> + PyErr_Format(PyExc_TypeError,
> + "The \"total\" attribute must be bool");
> + return NULL;
> + }
> +
> + if ((ret = PyList_New(0)) == NULL)
> + return NULL;
> +
> + if (totalbool == Py_False) {
Per other code in libvirt-override.c, you can't compare totalbool (type
PyObject) with Py_False, at least not on all compilers. You need
something like this instead:
/* Hack - Python's definition of Py_True breaks strict
* aliasing rules, so can't directly compare
*/
if (PyBool_Check(value)) {
PyObject *hacktrue = PyBool_FromLong(1);
temp->value.b = hacktrue == value ? 1 : 0;
Py_DECREF(hacktrue);
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + ncpus = virDomainGetCPUStats(domain, NULL, 0, 0, 0, flags);
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (ncpus < 0) {
> + error = VIR_PY_NONE;
> + goto failed;
> + }
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + nparams = virDomainGetCPUStats(domain, NULL, 0, 0, 1, flags);
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (nparams < 0) {
> + error = VIR_PY_NONE;
> + goto failed;
> + }
So far, so good.
> +
> + 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
[{},{},{},{}].
> + }
> + 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;
> + }
> +
> + 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.
> +
> + cpuparams = ¶ms[i * nparams];
> + 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/
> + error = NULL;
> + goto cleanup;
> + }
> + if (PyList_Append(ret, cpu) < 0) {
> + Py_DECREF(cpu);
> + error = NULL;
> + goto cleanup;
> + }
> + Py_DECREF(cpu);
> + }
> +
> + virTypedParameterArrayClear(params, sumparams);
> + VIR_FREE(params);
> + return ret;
> + } else {
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + nparams = virDomainGetCPUStats(domain, NULL, 0, -1, 1, flags);
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (nparams < 0) {
> + error = VIR_PY_NONE;
> + goto failed;
> + }
> +
> + if (!nparams) {
> + if ((total = PyDict_New()) == NULL) {
> + error = NULL;
> + goto failed;
> + }
> + if (PyList_Append(ret, total) < 0) {
> + Py_DECREF(total);
> + error = NULL;
Again, several instances of error=NULL that could be avoided if you just
initialize it that way at the beginning.
> + goto failed;
> + }
> +
> + Py_DECREF(total);
> + return ret;
> + }
> + sumparams = nparams;
> +
> + if (VIR_ALLOC_N(params, nparams) < 0) {
> + error = PyErr_NoMemory();
> + goto failed;
> + }
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + i_retval = virDomainGetCPUStats(domain, params, nparams, -1, 1, flags);
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (i_retval < 0) {
> + error = VIR_PY_NONE;
> + goto cleanup;
> + }
> +
> + if ((total = getPyVirTypedParameter(params, nparams)) == NULL) {
again, s/nparams/i_retval/
> + error = NULL;
> + goto cleanup;
> + }
> + if (PyList_Append(ret, total) < 0) {
> + Py_DECREF(total);
> + error = NULL;
> + goto cleanup;
> + }
> + Py_DECREF(total);
> +
> + virTypedParameterArrayClear(params, sumparams);
> + VIR_FREE(params);
> + return ret;
These last three lines are common to both arms of the if/then; I'd
factor them out to occur after the } and before the cleanup: label.
> + }
> +
> +cleanup:
Actually, I'm not sure I like the name cleanup; we tend to use that when
the success case also uses the lable, but you only ever go to this label
on error conditions. Furthermore,
> + virTypedParameterArrayClear(params, sumparams);
> + VIR_FREE(params);
these two calls are always safe, if you initialize params to NULL at the
beginning of the method. Therefore, you only need one label instead of
two, and per HACKING, it should be named 'error:' not 'failed:'.
> +
> +failed:
> + Py_DECREF(ret);
> + return error;
> +}
Not quite ready, but I'm looking forward to v3.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list