8:03 a.m.
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
10:40 p.m.
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},
12:26 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:42 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:38 a.m.
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
5:41 a.m.
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
12:36 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:35 a.m.
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
7:07 a.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:22 a.m.
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.
8:23 a.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:47 a.m.
On 03/21/2012 09:23 PM, Eric Blake wrote:
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.
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.
9:01 a.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:28 a.m.
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
**
4629
days inactive
4636
days old
13 comments
2 participants
participants (2)
-
Eric Blake -
Guannan Ren