[libvirt] [PATCH v2] python: add virDomainGetCPUStats python binding API
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(-) diff --git a/python/generator.py b/python/generator.py index 98072f0..4f95cc9 100755 --- a/python/generator.py +++ b/python/generator.py @@ -423,7 +423,7 @@ skip_impl = ( 'virDomainGetBlockIoTune', 'virDomainSetInterfaceParameters', 'virDomainGetInterfaceParameters', - 'virDomainGetCPUStats', # not implemented now. + 'virDomainGetCPUStats', 'virDomainGetDiskErrors', ) @@ -966,6 +966,9 @@ def nameFixup(name, classe, type, file): elif name[0:19] == "virStorageVolLookup": func = name[3:] func = string.lower(func[0:1]) + func[1:] + elif name[0:20] == "virDomainGetCPUStats": + func = name[9:] + func = string.lower(func[0:1]) + func[1:] elif name[0:12] == "virDomainGet": func = name[12:] func = string.lower(func[0:1]) + func[1:] diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index ab8f33a..c906cc3 100644 --- a/python/libvirt-override-api.xml +++ 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. + If boolean total is True, the first element of the list refers to CPU0 on the host, second element is CPU1, and so on. + 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> + <return type='str *' info='returns a list of dictionary in case of success, None in case of error'/> + <arg name='domain' type='virDomainPtr' info='pointer to domain object'/> + <arg name='total' type='bool' info='on true, return total domain CPU statistics, false return per-cpu info'/> + <arg name='flags' type='int' info='flags (unused; pass 0)'/> + </function> <function name='virDomainInterfaceStats' file='python'> <info>Extracts interface device statistics for a domain</info> <return type='virDomainInterfaceStats' info='a tuple of statistics'/> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 792cfa3..eb24c60 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -378,6 +378,169 @@ cleanup: } static PyObject * +libvirt_virDomainGetCPUStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) +{ + virDomainPtr domain; + PyObject *pyobj_domain, *totalbool; + PyObject *ret = NULL; + PyObject *cpu, *total; + PyObject *error; + int i, i_retval; + int ncpus = -1; + int sumparams, nparams = -1; + unsigned int flags; + virTypedParameterPtr params, cpuparams; + + if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainGetNumaParameters", + &pyobj_domain, &totalbool, &flags)) + return NULL; + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + 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) { + 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; + } + + if (!nparams) { + if ((cpu = PyDict_New()) == NULL) { + error = NULL; + goto failed; + } + + if (PyList_Append(ret, cpu) < 0) { + Py_DECREF(cpu); + error = NULL; + goto failed; + } + + Py_DECREF(cpu); + return ret; + } + 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); + 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; + + cpuparams = ¶ms[i * nparams]; + if ((cpu = getPyVirTypedParameter(cpuparams, nparams)) == NULL) { + 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; + 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) { + 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; + } + +cleanup: + virTypedParameterArrayClear(params, sumparams); + VIR_FREE(params); + +failed: + Py_DECREF(ret); + return error; +} + +static PyObject * libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { virDomainPtr domain; PyObject *pyobj_domain; @@ -5366,6 +5529,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virNetworkGetAutostart", libvirt_virNetworkGetAutostart, METH_VARARGS, NULL}, {(char *) "virDomainBlockStats", libvirt_virDomainBlockStats, METH_VARARGS, NULL}, {(char *) "virDomainBlockStatsFlags", libvirt_virDomainBlockStatsFlags, METH_VARARGS, NULL}, + {(char *) "virDomainGetCPUStats", libvirt_virDomainGetCPUStats, METH_VARARGS, NULL}, {(char *) "virDomainInterfaceStats", libvirt_virDomainInterfaceStats, METH_VARARGS, NULL}, {(char *) "virDomainMemoryStats", libvirt_virDomainMemoryStats, METH_VARARGS, NULL}, {(char *) "virNodeGetCellsFreeMemory", libvirt_virNodeGetCellsFreeMemory, METH_VARARGS, NULL}, -- 1.7.7.5
Is there anyone who would like to review this :) On 03/14/2012 09:03 PM, 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(-)
diff --git a/python/generator.py b/python/generator.py index 98072f0..4f95cc9 100755 --- a/python/generator.py +++ b/python/generator.py @@ -423,7 +423,7 @@ skip_impl = ( 'virDomainGetBlockIoTune', 'virDomainSetInterfaceParameters', 'virDomainGetInterfaceParameters', - 'virDomainGetCPUStats', # not implemented now. + 'virDomainGetCPUStats', 'virDomainGetDiskErrors', )
@@ -966,6 +966,9 @@ def nameFixup(name, classe, type, file): elif name[0:19] == "virStorageVolLookup": func = name[3:] func = string.lower(func[0:1]) + func[1:] + elif name[0:20] == "virDomainGetCPUStats": + func = name[9:] + func = string.lower(func[0:1]) + func[1:] elif name[0:12] == "virDomainGet": func = name[12:] func = string.lower(func[0:1]) + func[1:] diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index ab8f33a..c906cc3 100644 --- a/python/libvirt-override-api.xml +++ 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. + If boolean total is True, the first element of the list refers to CPU0 on the host, second element is CPU1, and so on. + 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> +<return type='str *' info='returns a list of dictionary in case of success, None in case of error'/> +<arg name='domain' type='virDomainPtr' info='pointer to domain object'/> +<arg name='total' type='bool' info='on true, return total domain CPU statistics, false return per-cpu info'/> +<arg name='flags' type='int' info='flags (unused; pass 0)'/> +</function> <function name='virDomainInterfaceStats' file='python'> <info>Extracts interface device statistics for a domain</info> <return type='virDomainInterfaceStats' info='a tuple of statistics'/> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 792cfa3..eb24c60 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -378,6 +378,169 @@ cleanup: }
static PyObject * +libvirt_virDomainGetCPUStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) +{ + virDomainPtr domain; + PyObject *pyobj_domain, *totalbool; + PyObject *ret = NULL; + PyObject *cpu, *total; + PyObject *error; + int i, i_retval; + int ncpus = -1; + int sumparams, nparams = -1; + unsigned int flags; + virTypedParameterPtr params, cpuparams; + + if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainGetNumaParameters", +&pyobj_domain,&totalbool,&flags)) + return NULL; + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + 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) { + 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; + } + + if (!nparams) { + if ((cpu = PyDict_New()) == NULL) { + error = NULL; + goto failed; + } + + if (PyList_Append(ret, cpu)< 0) { + Py_DECREF(cpu); + error = NULL; + goto failed; + } + + Py_DECREF(cpu); + return ret; + } + 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); + 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; + + cpuparams =¶ms[i * nparams]; + if ((cpu = getPyVirTypedParameter(cpuparams, nparams)) == NULL) { + 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; + 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) { + 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; + } + +cleanup: + virTypedParameterArrayClear(params, sumparams); + VIR_FREE(params); + +failed: + Py_DECREF(ret); + return error; +} + +static PyObject * libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { virDomainPtr domain; PyObject *pyobj_domain; @@ -5366,6 +5529,7 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virNetworkGetAutostart", libvirt_virNetworkGetAutostart, METH_VARARGS, NULL}, {(char *) "virDomainBlockStats", libvirt_virDomainBlockStats, METH_VARARGS, NULL}, {(char *) "virDomainBlockStatsFlags", libvirt_virDomainBlockStatsFlags, METH_VARARGS, NULL}, + {(char *) "virDomainGetCPUStats", libvirt_virDomainGetCPUStats, METH_VARARGS, NULL}, {(char *) "virDomainInterfaceStats", libvirt_virDomainInterfaceStats, METH_VARARGS, NULL}, {(char *) "virDomainMemoryStats", libvirt_virDomainMemoryStats, METH_VARARGS, NULL}, {(char *) "virNodeGetCellsFreeMemory", libvirt_virNodeGetCellsFreeMemory, METH_VARARGS, NULL},
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 03/17/2012 01:26 AM, Eric Blake wrote:
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);
Yes, it did report warning in compiling as follows due to the case from PyIntObject* to PyObject* warning :dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing] GCC command line to reproduce the error: gcc -Wstrict-aliasing=1 -O2 cpythonexample.c Actually PyObject_IsTrue() is a more light-weight approach to do the checking instead of creating a intermediate PyObject * for the compare. But for the more portability, It is still better to choose the above comparing approach for boolean value. Guannan Ren
On 03/18/2012 04:41 AM, Guannan Ren wrote:
+ 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);
Yes, it did report warning in compiling as follows due to the case from PyIntObject* to PyObject* warning :dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing]
And that would trip up a -Werror compilation, so I'm glad to see you changed it in v3.
GCC command line to reproduce the error: gcc -Wstrict-aliasing=1 -O2 cpythonexample.c
Actually PyObject_IsTrue() is a more light-weight approach to do the checking instead of creating a intermediate PyObject * for the compare.
Is PyObject_IsTrue() available in the version of python present on RHEL 5? If so, I'd be in favor of a followup cleanup patch that removes all our hacks in favor of the python glue code that does the same thing. And even if not, we should write a decent wrapper in our own typewrappers.c, so that the rest of our code doesn't have to look so ugly with so much copy-and-paste. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/20/2012 01:36 AM, Eric Blake wrote:
On 03/18/2012 04:41 AM, Guannan Ren wrote:
+ 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); Yes, it did report warning in compiling as follows due to the case from PyIntObject* to PyObject* warning :dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing] And that would trip up a -Werror compilation, so I'm glad to see you changed it in v3.
GCC command line to reproduce the error: gcc -Wstrict-aliasing=1 -O2 cpythonexample.c
Actually PyObject_IsTrue() is a more light-weight approach to do the checking instead of creating a intermediate PyObject * for the compare.
Is PyObject_IsTrue() available in the version of python present on RHEL 5? If so, I'd be in favor of a followup cleanup patch that removes all our hacks in favor of the python glue code that does the same thing. And even if not, we should write a decent wrapper in our own typewrappers.c, so that the rest of our code doesn't have to look so ugly with so much copy-and-paste.
On RHEL5.7 Python 2.4.3, the PyObject_IsTrue() is available to use. so, the original comparing from if (PyBool_Check(value)) { PyObject *hacktrue = PyBool_FromLong(1); temp->value.b = hacktrue == value ? 1 : 0; Py_DECREF(hacktrue); } else { PyErr_Format(PyExc_TypeError, "The value type of " "attribute \"%s\" must be bool", keystr); goto cleanup; } will become: if (PyBool_Check(value)) { temp->value.b = PyObject_IsTrue(value) ? 1 : 0; } else { PyErr_Format(PyExc_TypeError, "The value type of " "attribute \"%s\" must be bool", keystr); goto cleanup; } Guannan Ren
On 03/21/2012 01:35 AM, Guannan Ren wrote:
will become: if (PyBool_Check(value)) {
Why do we have to require a PyBool? My reading of PyObject_IsTrue is that it can convert other python objects to a boolean truth value, which is more flexible.
temp->value.b = PyObject_IsTrue(value) ? 1 : 0;
PyObject_IsTrue is tri-state; it can return failure. You don't want to map failure to true.
} else { PyErr_Format(PyExc_TypeError, "The value type of " "attribute \"%s\" must be bool", keystr); goto cleanup; }
Maybe we need a wrapper: int libvirt_boolUnwrap(PyObject *obj, bool *value) { int ret = PyObject_IsTrue(obj); if (ret < 0) return ret; *value = ret > 0; return 0; } and then callers become: if (libvirt_boolUnwrap(value, &temp->value.b) < 0) goto cleanup; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/21/2012 08:07 PM, Eric Blake wrote:
Maybe we need a wrapper:
int libvirt_boolUnwrap(PyObject *obj, bool *value) { int ret = PyObject_IsTrue(obj); if (ret< 0) return ret; *value = ret> 0; return 0; }
and then callers become:
if (libvirt_boolUnwrap(value,&temp->value.b)< 0) goto cleanup;
The libvirt_boolUnwrap is okay to be flexible like this, we still need the caller to check whether or not the value given from user python code is boolean argument, we need to do the data type checking in python C API before invoking the wrapper.
On 03/21/2012 07:22 AM, Guannan Ren wrote:
On 03/21/2012 08:07 PM, Eric Blake wrote:
Maybe we need a wrapper:
int libvirt_boolUnwrap(PyObject *obj, bool *value) { int ret = PyObject_IsTrue(obj); if (ret< 0) return ret; *value = ret> 0; return 0; }
and then callers become:
if (libvirt_boolUnwrap(value,&temp->value.b)< 0) goto cleanup;
The libvirt_boolUnwrap is okay to be flexible like this, we still need the caller to check whether or not the value given from user python code is boolean argument, we need to do the data type checking in python C API before invoking the wrapper.
But that's my point - the wrapper should be doing the type checking (PyObject_IsTrue already _does_ some type checking - that's why it can fail with -1 with an already decent python exception created). Callers shouldn't have to duplicate the work which can be done in just one place, but instead just pass in an object of unknown type and let the wrapper do the work of both type validation and conversion if the type was usable as a boolean. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/21/2012 09:23 PM, Eric Blake wrote:
On 03/21/2012 08:07 PM, Eric Blake wrote:
Maybe we need a wrapper:
int libvirt_boolUnwrap(PyObject *obj, bool *value) { int ret = PyObject_IsTrue(obj); if (ret< 0) return ret; *value = ret> 0; return 0; }
and then callers become:
if (libvirt_boolUnwrap(value,&temp->value.b)< 0) goto cleanup;
The libvirt_boolUnwrap is okay to be flexible like this, we still need the caller to check whether or not the value given from user python code is boolean argument, we need to do the data type checking in python C API before invoking the wrapper. But that's my point - the wrapper should be doing the type checking (PyObject_IsTrue already _does_ some type checking - that's why it can fail with -1 with an already decent python exception created). Callers shouldn't have to duplicate the work which can be done in just one
On 03/21/2012 07:22 AM, Guannan Ren wrote: place, but instead just pass in an object of unknown type and let the wrapper do the work of both type validation and conversion if the type was usable as a boolean.
I get your points, sorry I don't express my idea clearly. According to my experience, for PyObject_IsTrue, all of non-null objects belongs to True, null objects like {}, ""(str) ,None, False counts as False, it's good way to work with Pybool_check to ensure the type of value given by upper python code is right type. The wrapper should do the job.
On 03/21/2012 07:47 AM, Guannan Ren wrote:
I get your points, sorry I don't express my idea clearly. According to my experience, for PyObject_IsTrue, all of non-null objects belongs to True, null objects like {}, ""(str) ,None, False counts as False,
Correct.
it's good way to work with Pybool_check to ensure the type of value given by upper python code is right type. The wrapper should do the job.
Only if we want to be sticklers. For reference, in C, void foo(bool x); can be called with a bool (foo(true) or foo(false)), an int (foo(1) or foo(0)), a pointer (foo("") or foo(NULL)). That is, C gives you automatic conversion. In Java, you _have_ to pass a boolean, where void foo(boolean x) {} must be called as foo(true), foo(i != 0), foo(str != null), and so forth, which puts more burden on the caller to do the type conversion up front. Which style describes python code? Is python type-strict, where you have to manually convert to bool in contexts that demand a PyBool, or do you get automatic conversion where the empty string, None, integer 0, and other python objects can be implicitly used in place of the actual False object? Depending on that answer determines whether you should be using PyBool_Check and rejecting invalid types, or whether you allow all python objects with their normal conversion to boolean. And my limited understanding (given that I have not done much python coding) is that python is relaxed like C, not strict like Java. In other words, I think we should allow users to call dom.getCPUStats(0, 0) rather than forcing them to call dom.getCPUStats(False, 0), if that is the convention allowed elsewhere in native python code. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/21/2012 10:01 PM, Eric Blake wrote:
Only if we want to be sticklers. For reference, in C,
void foo(bool x);
can be called with a bool (foo(true) or foo(false)), an int (foo(1) or foo(0)), a pointer (foo("") or foo(NULL)). That is, C gives you automatic conversion.
In Java, you _have_ to pass a boolean, where
void foo(boolean x) {}
must be called as foo(true), foo(i != 0), foo(str != null), and so forth, which puts more burden on the caller to do the type conversion up front.
Which style describes python code? Is python type-strict, where you have to manually convert to bool in contexts that demand a PyBool, or do you get automatic conversion where the empty string, None, integer 0, and other python objects can be implicitly used in place of the actual False object? Depending on that answer determines whether you should be using PyBool_Check and rejecting invalid types, or whether you allow all python objects with their normal conversion to boolean. And my limited understanding (given that I have not done much python coding) is that python is relaxed like C, not strict like Java. In other words, I think we should allow users to call dom.getCPUStats(0, 0) rather than forcing them to call dom.getCPUStats(False, 0), if that is the convention allowed elsewhere in native python code.
Thanks for these ideas I totally agree with them. Guannan Ren **
participants (2)
-
Eric Blake -
Guannan Ren