12:33 p.m.
The first version
https://www.redhat.com/archives/libvir-list/2012-April/msg00848.html
Sorry for so late.
I will go on adding error handling and make it strong
for python binding APIs if necessary
Guannan Ren
python: cleanup vcpu related binding APIs
python: return error if PyObject obj is NULL for unwrapper helper functions
python/libvirt-override.c | 171 ++++++++++++++++++++++++++++++++++++++--------------
python/typewrappers.c | 43 +++++++++++++-
2 files changed, 164 insertions(+), 50 deletions(-)
12:33 p.m.
New subject: [libvirt] [PATCH 1/2] python: cleanup vcpu related binding APIs
libvirt_virDomainGetVcpus: add error handling, return -1 instead of None
libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
make use of libvirt_boolUnwrap
Set bitmap according to these values which are contained in given
argument of vcpu tuple and turn off these bit corresponding to
missing vcpus in argument tuple
The original way ignored the error info from PyTuple_GetItem
if index is out of range.
"IndexError: tuple index out of range"
The error message will only be raised on next command in interactive mode.
---
python/libvirt-override.c | 171 +++++++++++++++++++++++++++++++++-------------
1 file changed, 123 insertions(+), 48 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 25f9d3f..b69f5cf 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -1333,9 +1333,11 @@ cleanup:
static PyObject *
libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
PyObject *pyobj_domain, *pyretval = NULL, *pycpuinfo = NULL, *pycpumap = NULL;
+ PyObject *error = NULL;
virNodeInfo nodeinfo;
virDomainInfo dominfo;
virVcpuInfoPtr cpuinfo = NULL;
@@ -1352,29 +1354,33 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
i_retval = virNodeGetInfo(virDomainGetConnect(domain), &nodeinfo);
LIBVIRT_END_ALLOW_THREADS;
if (i_retval < 0)
- return VIR_PY_NONE;
+ return VIR_PY_INT_FAIL;
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainGetInfo(domain, &dominfo);
LIBVIRT_END_ALLOW_THREADS;
if (i_retval < 0)
- return VIR_PY_NONE;
+ return VIR_PY_INT_FAIL;
if (VIR_ALLOC_N(cpuinfo, dominfo.nrVirtCpu) < 0)
- return VIR_PY_NONE;
+ return PyErr_NoMemory();
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (xalloc_oversized(dominfo.nrVirtCpu, cpumaplen) ||
- VIR_ALLOC_N(cpumap, dominfo.nrVirtCpu * cpumaplen) < 0)
+ VIR_ALLOC_N(cpumap, dominfo.nrVirtCpu * cpumaplen) < 0) {
+ error = PyErr_NoMemory();
goto cleanup;
+ }
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainGetVcpus(domain,
cpuinfo, dominfo.nrVirtCpu,
cpumap, cpumaplen);
LIBVIRT_END_ALLOW_THREADS;
- if (i_retval < 0)
+ if (i_retval < 0) {
+ error = VIR_PY_INT_FAIL;
goto cleanup;
+ }
/* convert to a Python tuple of long objects */
if ((pyretval = PyTuple_New(2)) == NULL)
@@ -1386,13 +1392,43 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) {
PyObject *info = PyTuple_New(4);
+ PyObject *item = NULL;
+ bool itemError = true;
+
if (info == NULL)
goto cleanup;
- PyTuple_SetItem(info, 0, PyInt_FromLong((long)cpuinfo[i].number));
- PyTuple_SetItem(info, 1, PyInt_FromLong((long)cpuinfo[i].state));
- PyTuple_SetItem(info, 2, PyLong_FromLongLong((long long)cpuinfo[i].cpuTime));
- PyTuple_SetItem(info, 3, PyInt_FromLong((long)cpuinfo[i].cpu));
- PyList_SetItem(pycpuinfo, i, info);
+ do {
+ if ((item = PyInt_FromLong((long)cpuinfo[i].number)) == NULL)
+ break;
+ else if ((PyTuple_SetItem(info, 0, item)) < 0)
+ break;
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].state)) == NULL)
+ break;
+ else if (PyTuple_SetItem(info, 1, item) < 0)
+ break;
+
+ if ((item = PyLong_FromLongLong((long long)cpuinfo[i].cpuTime)) == NULL)
+ break;
+ else if (PyTuple_SetItem(info, 2, item) < 0)
+ break;
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].cpu)) == NULL)
+ break;
+ else if (PyTuple_SetItem(info, 3, item) < 0)
+ break;
+
+ if (PyList_SetItem(pycpuinfo, i, info) < 0)
+ break;
+
+ itemError = false;
+ } while (0);
+
+ if (itemError) {
+ Py_DECREF(info);
+ Py_XDECREF(item);
+ goto cleanup;
+ }
}
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) {
PyObject *info = PyTuple_New(VIR_NODEINFO_MAXCPUS(nodeinfo));
@@ -1400,36 +1436,52 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
if (info == NULL)
goto cleanup;
for (j = 0 ; j < VIR_NODEINFO_MAXCPUS(nodeinfo) ; j++) {
- PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i,
j)));
+ PyObject *item = NULL;
+ if ((item = PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))) ==
NULL) {
+ Py_DECREF(info);
+ goto cleanup;
+ } else if (PyTuple_SetItem(info, j, item) < 0) {
+ Py_DECREF(info);
+ Py_DECREF(item);
+ goto cleanup;
+ }
+ }
+ if (PyList_SetItem(pycpumap, i, info) < 0) {
+ Py_DECREF(info);
+ goto cleanup;
}
- PyList_SetItem(pycpumap, i, info);
}
- PyTuple_SetItem(pyretval, 0, pycpuinfo);
- PyTuple_SetItem(pyretval, 1, pycpumap);
+ if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0)
+ goto cleanup;
+
+ if (PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
+ goto cleanup;
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
return pyretval;
- cleanup:
+cleanup:
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
Py_XDECREF(pyretval);
Py_XDECREF(pycpuinfo);
Py_XDECREF(pycpumap);
- return VIR_PY_NONE;
+ return error;
}
static PyObject *
libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
- PyObject *pyobj_domain, *pycpumap, *truth;
+ PyObject *pyobj_domain, *pycpumap;
+ PyObject *ret = NULL;
virNodeInfo nodeinfo;
unsigned char *cpumap;
- int cpumaplen, i, vcpu;
+ int cpumaplen, i, j, vcpu, tuple_size;
int i_retval;
if (!PyArg_ParseTuple(args, (char *)"OiO:virDomainPinVcpu",
@@ -1445,37 +1497,50 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
- return VIR_PY_INT_FAIL;
+ return PyErr_NoMemory();
+
+ tuple_size = PyTuple_Size(pycpumap);
+ if (tuple_size == -1)
+ goto cleanup;
- truth = PyBool_FromLong(1);
- for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) {
+ for (i = 0; i < tuple_size; i++) {
PyObject *flag = PyTuple_GetItem(pycpumap, i);
- if (flag == truth)
- VIR_USE_CPU(cpumap, i);
- else
- VIR_UNUSE_CPU(cpumap, i);
+ bool b;
+
+ if (!flag || libvirt_boolUnwrap(flag, &b) < 0)
+ goto cleanup;
+
+ (b) ? VIR_USE_CPU(cpumap, i) : VIR_UNUSE_CPU(cpumap, i);
}
+ for (j = 0; j < VIR_NODEINFO_MAXCPUS(nodeinfo) - i; j++)
+ VIR_UNUSE_CPU(cpumap, i + j);
+
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainPinVcpu(domain, vcpu, cpumap, cpumaplen);
LIBVIRT_END_ALLOW_THREADS;
- Py_DECREF(truth);
- VIR_FREE(cpumap);
- if (i_retval < 0)
- return VIR_PY_INT_FAIL;
+ if (i_retval < 0) {
+ ret = VIR_PY_INT_FAIL;
+ goto cleanup;
+ }
+ ret = VIR_PY_INT_SUCCESS;
- return VIR_PY_INT_SUCCESS;
+cleanup:
+ VIR_FREE(cpumap);
+ return ret;
}
static PyObject *
libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
- PyObject *pyobj_domain, *pycpumap, *truth;
+ PyObject *pyobj_domain, *pycpumap;
+ PyObject *ret = NULL;
virNodeInfo nodeinfo;
unsigned char *cpumap;
- int cpumaplen, i, vcpu;
+ int cpumaplen, i, j, vcpu, tuple_size;
unsigned int flags;
int i_retval;
@@ -1492,27 +1557,37 @@ libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED,
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
- return VIR_PY_INT_FAIL;
+ return PyErr_NoMemory();
+
+ tuple_size = PyTuple_Size(pycpumap);
+ if (tuple_size == -1)
+ goto cleanup;
- truth = PyBool_FromLong(1);
- for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) {
+ for (i = 0; i < tuple_size; i++) {
PyObject *flag = PyTuple_GetItem(pycpumap, i);
- if (flag == truth)
- VIR_USE_CPU(cpumap, i);
- else
- VIR_UNUSE_CPU(cpumap, i);
+ bool b;
+
+ if (!flag || libvirt_boolUnwrap(flag, &b) < 0)
+ goto cleanup;
+
+ (b) ? VIR_USE_CPU(cpumap, i) : VIR_UNUSE_CPU(cpumap, i);
}
+ for (j = 0; j < VIR_NODEINFO_MAXCPUS(nodeinfo) - i; j++)
+ VIR_UNUSE_CPU(cpumap, i + j);
+
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainPinVcpuFlags(domain, vcpu, cpumap, cpumaplen, flags);
LIBVIRT_END_ALLOW_THREADS;
- Py_DECREF(truth);
- VIR_FREE(cpumap);
-
- if (i_retval < 0)
- return VIR_PY_INT_FAIL;
+ if (i_retval < 0) {
+ ret = VIR_PY_INT_FAIL;
+ goto cleanup;
+ }
+ ret = VIR_PY_INT_SUCCESS;
- return VIR_PY_INT_SUCCESS;
+cleanup:
+ VIR_FREE(cpumap);
+ return ret;
}
static PyObject *
--
1.7.11.2
8:51 a.m.
New subject: [libvirt] [PATCH 1/2] python: cleanup vcpu related binding APIs
On 26.09.2012 19:33, Guannan Ren wrote:
libvirt_virDomainGetVcpus: add error handling, return -1 instead of
None
libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
make use of libvirt_boolUnwrap
Set bitmap according to these values which are contained in given
argument of vcpu tuple and turn off these bit corresponding to
missing vcpus in argument tuple
The original way ignored the error info from PyTuple_GetItem
if index is out of range.
"IndexError: tuple index out of range"
The error message will only be raised on next command in interactive mode.
---
python/libvirt-override.c | 171 +++++++++++++++++++++++++++++++++-------------
1 file changed, 123 insertions(+), 48 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 25f9d3f..b69f5cf 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -1333,9 +1333,11 @@ cleanup:
static PyObject *
libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
PyObject *pyobj_domain, *pyretval = NULL, *pycpuinfo = NULL, *pycpumap = NULL;
+ PyObject *error = NULL;
You are not setting this variable before every 'goto cleanup;' and I
think it should be done. Or is it okay to return NULL?
virNodeInfo nodeinfo;
virDomainInfo dominfo;
virVcpuInfoPtr cpuinfo = NULL;
@@ -1352,29 +1354,33 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
i_retval = virNodeGetInfo(virDomainGetConnect(domain), &nodeinfo);
LIBVIRT_END_ALLOW_THREADS;
if (i_retval < 0)
- return VIR_PY_NONE;
+ return VIR_PY_INT_FAIL;
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainGetInfo(domain, &dominfo);
LIBVIRT_END_ALLOW_THREADS;
if (i_retval < 0)
- return VIR_PY_NONE;
+ return VIR_PY_INT_FAIL;
if (VIR_ALLOC_N(cpuinfo, dominfo.nrVirtCpu) < 0)
- return VIR_PY_NONE;
+ return PyErr_NoMemory();
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (xalloc_oversized(dominfo.nrVirtCpu, cpumaplen) ||
- VIR_ALLOC_N(cpumap, dominfo.nrVirtCpu * cpumaplen) < 0)
+ VIR_ALLOC_N(cpumap, dominfo.nrVirtCpu * cpumaplen) < 0) {
+ error = PyErr_NoMemory();
goto cleanup;
+ }
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainGetVcpus(domain,
cpuinfo, dominfo.nrVirtCpu,
cpumap, cpumaplen);
LIBVIRT_END_ALLOW_THREADS;
- if (i_retval < 0)
+ if (i_retval < 0) {
+ error = VIR_PY_INT_FAIL;
goto cleanup;
+ }
/* convert to a Python tuple of long objects */
if ((pyretval = PyTuple_New(2)) == NULL)
@@ -1386,13 +1392,43 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) {
PyObject *info = PyTuple_New(4);
+ PyObject *item = NULL;
+ bool itemError = true;
+
if (info == NULL)
goto cleanup;
- PyTuple_SetItem(info, 0, PyInt_FromLong((long)cpuinfo[i].number));
- PyTuple_SetItem(info, 1, PyInt_FromLong((long)cpuinfo[i].state));
- PyTuple_SetItem(info, 2, PyLong_FromLongLong((long long)cpuinfo[i].cpuTime));
- PyTuple_SetItem(info, 3, PyInt_FromLong((long)cpuinfo[i].cpu));
- PyList_SetItem(pycpuinfo, i, info);
+ do {
+ if ((item = PyInt_FromLong((long)cpuinfo[i].number)) == NULL)
+ break;
+ else if ((PyTuple_SetItem(info, 0, item)) < 0)
+ break;
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].state)) == NULL)
+ break;
+ else if (PyTuple_SetItem(info, 1, item) < 0)
+ break;
+
+ if ((item = PyLong_FromLongLong((long long)cpuinfo[i].cpuTime)) == NULL)
+ break;
+ else if (PyTuple_SetItem(info, 2, item) < 0)
+ break;
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].cpu)) == NULL)
+ break;
+ else if (PyTuple_SetItem(info, 3, item) < 0)
+ break;
+
+ if (PyList_SetItem(pycpuinfo, i, info) < 0)
+ break;
+
+ itemError = false;
+ } while (0);
+
+ if (itemError) {
+ Py_DECREF(info);
+ Py_XDECREF(item);
+ goto cleanup;
+ }
No. I know python bindings code are mess but this won't make it any
better. First of all:
if (cond1)
code_block;
else if (cond2)
the_very_same_codeblock;
can be joined like this:
if (cond1 || cond2)
code_block;
Second - drop the do { } while(0) and use a label instead:
if (cond1 || cond2)
goto label;
This label can in this special case be at the end of the parent for
loop; However, there needs to be 'continue' just before the label so the
for loop doesn't execute the label itself.
}
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) {
PyObject *info = PyTuple_New(VIR_NODEINFO_MAXCPUS(nodeinfo));
@@ -1400,36 +1436,52 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
if (info == NULL)
goto cleanup;
for (j = 0 ; j < VIR_NODEINFO_MAXCPUS(nodeinfo) ; j++) {
- PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen,
i, j)));
+ PyObject *item = NULL;
+ if ((item = PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))) ==
NULL) {
+ Py_DECREF(info);
+ goto cleanup;
+ } else if (PyTuple_SetItem(info, j, item) < 0) {
+ Py_DECREF(info);
+ Py_DECREF(item);
+ goto cleanup;
Again, join this if () else if; you probably need to use
Py_XDECREF(item) then.
+ }
+ }
+ if (PyList_SetItem(pycpumap, i, info) < 0) {
+ Py_DECREF(info);
+ goto cleanup;
}
- PyList_SetItem(pycpumap, i, info);
}
- PyTuple_SetItem(pyretval, 0, pycpuinfo);
- PyTuple_SetItem(pyretval, 1, pycpumap);
+ if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0)
+ goto cleanup;
+
+ if (PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
+ goto cleanup;
And again a candidate for joining.
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
return pyretval;
- cleanup:
+cleanup:
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
Py_XDECREF(pyretval);
Py_XDECREF(pycpuinfo);
Py_XDECREF(pycpumap);
- return VIR_PY_NONE;
+ return error;
}
static PyObject *
libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
- PyObject *pyobj_domain, *pycpumap, *truth;
+ PyObject *pyobj_domain, *pycpumap;
+ PyObject *ret = NULL;
virNodeInfo nodeinfo;
unsigned char *cpumap;
- int cpumaplen, i, vcpu;
+ int cpumaplen, i, j, vcpu, tuple_size;
int i_retval;
if (!PyArg_ParseTuple(args, (char *)"OiO:virDomainPinVcpu",
@@ -1445,37 +1497,50 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
- return VIR_PY_INT_FAIL;
+ return PyErr_NoMemory();
+
+ tuple_size = PyTuple_Size(pycpumap);
+ if (tuple_size == -1)
+ goto cleanup;
- truth = PyBool_FromLong(1);
- for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) {
+ for (i = 0; i < tuple_size; i++) {
PyObject *flag = PyTuple_GetItem(pycpumap, i);
- if (flag == truth)
- VIR_USE_CPU(cpumap, i);
- else
- VIR_UNUSE_CPU(cpumap, i);
+ bool b;
+
+ if (!flag || libvirt_boolUnwrap(flag, &b) < 0)
+ goto cleanup;
+
+ (b) ? VIR_USE_CPU(cpumap, i) : VIR_UNUSE_CPU(cpumap, i);
Well, in libvirt we prefer 'if (b) ...' rather than this.
}
+ for (j = 0; j < VIR_NODEINFO_MAXCPUS(nodeinfo) - i; j++)
+ VIR_UNUSE_CPU(cpumap, i + j);
+
This would be more readable as:
for (; i < VIR_NODEINFO_MAXCPUS(nodeinfo); i++)
VIR_UNUSE_CPU(cpumap, i);
The 'j' variable can be then dropped.
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainPinVcpu(domain, vcpu, cpumap, cpumaplen);
LIBVIRT_END_ALLOW_THREADS;
- Py_DECREF(truth);
- VIR_FREE(cpumap);
- if (i_retval < 0)
- return VIR_PY_INT_FAIL;
+ if (i_retval < 0) {
+ ret = VIR_PY_INT_FAIL;
+ goto cleanup;
+ }
+ ret = VIR_PY_INT_SUCCESS;
- return VIR_PY_INT_SUCCESS;
+cleanup:
+ VIR_FREE(cpumap);
+ return ret;
}
static PyObject *
libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
- PyObject *pyobj_domain, *pycpumap, *truth;
+ PyObject *pyobj_domain, *pycpumap;
+ PyObject *ret = NULL;
virNodeInfo nodeinfo;
unsigned char *cpumap;
- int cpumaplen, i, vcpu;
+ int cpumaplen, i, j, vcpu, tuple_size;
unsigned int flags;
int i_retval;
@@ -1492,27 +1557,37 @@ libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED,
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
- return VIR_PY_INT_FAIL;
+ return PyErr_NoMemory();
+
+ tuple_size = PyTuple_Size(pycpumap);
+ if (tuple_size == -1)
+ goto cleanup;
- truth = PyBool_FromLong(1);
- for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) {
+ for (i = 0; i < tuple_size; i++) {
PyObject *flag = PyTuple_GetItem(pycpumap, i);
- if (flag == truth)
- VIR_USE_CPU(cpumap, i);
- else
- VIR_UNUSE_CPU(cpumap, i);
+ bool b;
+
+ if (!flag || libvirt_boolUnwrap(flag, &b) < 0)
+ goto cleanup;
+
+ (b) ? VIR_USE_CPU(cpumap, i) : VIR_UNUSE_CPU(cpumap, i);
same applies here
}
+ for (j = 0; j < VIR_NODEINFO_MAXCPUS(nodeinfo) - i; j++)
+ VIR_UNUSE_CPU(cpumap, i + j);
+
and here
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainPinVcpuFlags(domain, vcpu, cpumap, cpumaplen, flags);
LIBVIRT_END_ALLOW_THREADS;
- Py_DECREF(truth);
- VIR_FREE(cpumap);
-
- if (i_retval < 0)
- return VIR_PY_INT_FAIL;
+ if (i_retval < 0) {
+ ret = VIR_PY_INT_FAIL;
+ goto cleanup;
+ }
+ ret = VIR_PY_INT_SUCCESS;
- return VIR_PY_INT_SUCCESS;
+cleanup:
+ VIR_FREE(cpumap);
+ return ret;
}
static PyObject *
Michal
1:38 a.m.
New subject: [libvirt] [PATCH 1/2] python: cleanup vcpu related binding APIs
On 09/27/2012 09:51 PM, Michal Privoznik wrote:
On 26.09.2012 19:33, Guannan Ren wrote:
> libvirt_virDomainGetVcpus: add error handling, return -1 instead of None
> libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
> make use of libvirt_boolUnwrap
>
> Set bitmap according to these values which are contained in given
> argument of vcpu tuple and turn off these bit corresponding to
> missing vcpus in argument tuple
>
> The original way ignored the error info from PyTuple_GetItem
> if index is out of range.
> "IndexError: tuple index out of range"
> The error message will only be raised on next command in interactive mode.
> ---
> python/libvirt-override.c | 171 +++++++++++++++++++++++++++++++++-------------
> 1 file changed, 123 insertions(+), 48 deletions(-)
>
> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> index 25f9d3f..b69f5cf 100644
> --- a/python/libvirt-override.c
> +++ b/python/libvirt-override.c
> @@ -1333,9 +1333,11 @@ cleanup:
>
> static PyObject *
> libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
> - PyObject *args) {
> + PyObject *args)
> +{
> virDomainPtr domain;
> PyObject *pyobj_domain, *pyretval = NULL, *pycpuinfo = NULL, *pycpumap = NULL;
> + PyObject *error = NULL;
You are not setting this variable before every 'goto cleanup;' and I
think it should be done. Or is it okay to return NULL?
we set error variable in two places.
error = PyErr_NoMemory();
error = VIR_PY_INT_FAIL;
For the other places before goto cleanup, we don't need to set
specific error because
the cpython function did it already. what left to me is to return
NULL to show these
error messages to user.
In common, if cpython function(e.g PyTuple_New()) reports errors,
we just return NULL.
No. I know python bindings code are mess but this won't make it any
better. First of all:
if (cond1)
code_block;
else if (cond2)
the_very_same_codeblock;
can be joined like this:
if (cond1 || cond2)
code_block;
Thanks I will change to this.
Second - drop the do { } while(0) and use a label instead:
if (cond1 || cond2)
goto label;
without do () while, we have to have two labels
I think it will be a little messy.
I tried the following before, it is ok to walk back.
if (cond1 || cond2)
goto label;
continue;
label:
Py_DECREF(info);
..
goto cleanup
This label can in this special case be at the end of the parent for
loop; However, there needs to be 'continue' just before the label so the
for loop doesn't execute the label itself.
For the following, I will change according to your comments.
Thanks for this review. :)
Guannan
3:27 a.m.
New subject: [libvirt] [PATCH v3] python: cleanup vcpu related binding APIs
libvirt_virDomainGetVcpus: add error handling, return -1 instead of None
libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
check the type of argument
make use of libvirt_boolUnwrap
Set bitmap according to these values which are contained in given
argument of vcpu tuple and turn off these bit corresponding to
missing vcpus in argument tuple
The original way ignored the error info from PyTuple_GetItem
if index is out of range.
"IndexError: tuple index out of range"
The error message will only be raised on next command in interactive mode.
---
python/libvirt-override.c | 163 ++++++++++++++++++++++++++++++++++------------
1 file changed, 121 insertions(+), 42 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 25f9d3f..81099b1 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -1333,9 +1333,11 @@ cleanup:
static PyObject *
libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
PyObject *pyobj_domain, *pyretval = NULL, *pycpuinfo = NULL, *pycpumap = NULL;
+ PyObject *error = NULL;
virNodeInfo nodeinfo;
virDomainInfo dominfo;
virVcpuInfoPtr cpuinfo = NULL;
@@ -1352,29 +1354,33 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
i_retval = virNodeGetInfo(virDomainGetConnect(domain), &nodeinfo);
LIBVIRT_END_ALLOW_THREADS;
if (i_retval < 0)
- return VIR_PY_NONE;
+ return VIR_PY_INT_FAIL;
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainGetInfo(domain, &dominfo);
LIBVIRT_END_ALLOW_THREADS;
if (i_retval < 0)
- return VIR_PY_NONE;
+ return VIR_PY_INT_FAIL;
if (VIR_ALLOC_N(cpuinfo, dominfo.nrVirtCpu) < 0)
- return VIR_PY_NONE;
+ return PyErr_NoMemory();
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (xalloc_oversized(dominfo.nrVirtCpu, cpumaplen) ||
- VIR_ALLOC_N(cpumap, dominfo.nrVirtCpu * cpumaplen) < 0)
+ VIR_ALLOC_N(cpumap, dominfo.nrVirtCpu * cpumaplen) < 0) {
+ error = PyErr_NoMemory();
goto cleanup;
+ }
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainGetVcpus(domain,
cpuinfo, dominfo.nrVirtCpu,
cpumap, cpumaplen);
LIBVIRT_END_ALLOW_THREADS;
- if (i_retval < 0)
+ if (i_retval < 0) {
+ error = VIR_PY_INT_FAIL;
goto cleanup;
+ }
/* convert to a Python tuple of long objects */
if ((pyretval = PyTuple_New(2)) == NULL)
@@ -1386,13 +1392,35 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) {
PyObject *info = PyTuple_New(4);
+ PyObject *item = NULL;
+
if (info == NULL)
goto cleanup;
- PyTuple_SetItem(info, 0, PyInt_FromLong((long)cpuinfo[i].number));
- PyTuple_SetItem(info, 1, PyInt_FromLong((long)cpuinfo[i].state));
- PyTuple_SetItem(info, 2, PyLong_FromLongLong((long long)cpuinfo[i].cpuTime));
- PyTuple_SetItem(info, 3, PyInt_FromLong((long)cpuinfo[i].cpu));
- PyList_SetItem(pycpuinfo, i, info);
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].number)) == NULL ||
+ PyTuple_SetItem(info, 0, item) < 0)
+ goto itemError;
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].state)) == NULL ||
+ PyTuple_SetItem(info, 1, item) < 0)
+ goto itemError;
+
+ if ((item = PyLong_FromLongLong((long long)cpuinfo[i].cpuTime)) == NULL ||
+ PyTuple_SetItem(info, 2, item) < 0)
+ goto itemError;
+
+ if ((item = PyInt_FromLong((long)cpuinfo[i].cpu)) == NULL ||
+ PyTuple_SetItem(info, 3, item) < 0)
+ goto itemError;
+
+ if (PyList_SetItem(pycpuinfo, i, info) < 0)
+ goto itemError;
+
+ continue;
+ itemError:
+ Py_DECREF(info);
+ Py_XDECREF(item);
+ goto cleanup;
}
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) {
PyObject *info = PyTuple_New(VIR_NODEINFO_MAXCPUS(nodeinfo));
@@ -1400,36 +1428,48 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
if (info == NULL)
goto cleanup;
for (j = 0 ; j < VIR_NODEINFO_MAXCPUS(nodeinfo) ; j++) {
- PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i,
j)));
+ PyObject *item = NULL;
+ if ((item = PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))) == NULL
||
+ PyTuple_SetItem(info, j, item) < 0) {
+ Py_DECREF(info);
+ Py_XDECREF(item);
+ goto cleanup;
+ }
+ }
+ if (PyList_SetItem(pycpumap, i, info) < 0) {
+ Py_DECREF(info);
+ goto cleanup;
}
- PyList_SetItem(pycpumap, i, info);
}
- PyTuple_SetItem(pyretval, 0, pycpuinfo);
- PyTuple_SetItem(pyretval, 1, pycpumap);
+ if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0 ||
+ PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
+ goto cleanup;
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
return pyretval;
- cleanup:
+cleanup:
VIR_FREE(cpuinfo);
VIR_FREE(cpumap);
Py_XDECREF(pyretval);
Py_XDECREF(pycpuinfo);
Py_XDECREF(pycpumap);
- return VIR_PY_NONE;
+ return error;
}
static PyObject *
libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
- PyObject *pyobj_domain, *pycpumap, *truth;
+ PyObject *pyobj_domain, *pycpumap;
+ PyObject *ret = NULL;
virNodeInfo nodeinfo;
unsigned char *cpumap;
- int cpumaplen, i, vcpu;
+ int cpumaplen, i, vcpu, tuple_size;
int i_retval;
if (!PyArg_ParseTuple(args, (char *)"OiO:virDomainPinVcpu",
@@ -1443,39 +1483,60 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
if (i_retval < 0)
return VIR_PY_INT_FAIL;
+ if (PyTuple_Check(pycpumap)) {
+ tuple_size = PyTuple_Size(pycpumap);
+ if (tuple_size == -1)
+ return ret;
+ } else {
+ PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required");
+ return ret;
+ }
+
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
- return VIR_PY_INT_FAIL;
+ return PyErr_NoMemory();
- truth = PyBool_FromLong(1);
- for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) {
+ for (i = 0; i < tuple_size; i++) {
PyObject *flag = PyTuple_GetItem(pycpumap, i);
- if (flag == truth)
+ bool b;
+
+ if (!flag || libvirt_boolUnwrap(flag, &b) < 0)
+ goto cleanup;
+
+ if (b)
VIR_USE_CPU(cpumap, i);
else
VIR_UNUSE_CPU(cpumap, i);
}
+ for (; i < VIR_NODEINFO_MAXCPUS(nodeinfo); i++)
+ VIR_UNUSE_CPU(cpumap, i);
+
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainPinVcpu(domain, vcpu, cpumap, cpumaplen);
LIBVIRT_END_ALLOW_THREADS;
- Py_DECREF(truth);
- VIR_FREE(cpumap);
- if (i_retval < 0)
- return VIR_PY_INT_FAIL;
+ if (i_retval < 0) {
+ ret = VIR_PY_INT_FAIL;
+ goto cleanup;
+ }
+ ret = VIR_PY_INT_SUCCESS;
- return VIR_PY_INT_SUCCESS;
+cleanup:
+ VIR_FREE(cpumap);
+ return ret;
}
static PyObject *
libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED,
- PyObject *args) {
+ PyObject *args)
+{
virDomainPtr domain;
- PyObject *pyobj_domain, *pycpumap, *truth;
+ PyObject *pyobj_domain, *pycpumap;
+ PyObject *ret = NULL;
virNodeInfo nodeinfo;
unsigned char *cpumap;
- int cpumaplen, i, vcpu;
+ int cpumaplen, i, vcpu, tuple_size;
unsigned int flags;
int i_retval;
@@ -1490,29 +1551,47 @@ libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED,
if (i_retval < 0)
return VIR_PY_INT_FAIL;
+ if (PyTuple_Check(pycpumap)) {
+ tuple_size = PyTuple_Size(pycpumap);
+ if (tuple_size == -1)
+ return ret;
+ } else {
+ PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required");
+ return ret;
+ }
+
cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
- return VIR_PY_INT_FAIL;
+ return PyErr_NoMemory();
- truth = PyBool_FromLong(1);
- for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) {
+ for (i = 0; i < tuple_size; i++) {
PyObject *flag = PyTuple_GetItem(pycpumap, i);
- if (flag == truth)
+ bool b;
+
+ if (!flag || libvirt_boolUnwrap(flag, &b) < 0)
+ goto cleanup;
+
+ if (b)
VIR_USE_CPU(cpumap, i);
else
VIR_UNUSE_CPU(cpumap, i);
}
+ for (; i < VIR_NODEINFO_MAXCPUS(nodeinfo); i++)
+ VIR_UNUSE_CPU(cpumap, i);
+
LIBVIRT_BEGIN_ALLOW_THREADS;
i_retval = virDomainPinVcpuFlags(domain, vcpu, cpumap, cpumaplen, flags);
LIBVIRT_END_ALLOW_THREADS;
- Py_DECREF(truth);
- VIR_FREE(cpumap);
-
- if (i_retval < 0)
- return VIR_PY_INT_FAIL;
+ if (i_retval < 0) {
+ ret = VIR_PY_INT_FAIL;
+ goto cleanup;
+ }
+ ret = VIR_PY_INT_SUCCESS;
- return VIR_PY_INT_SUCCESS;
+cleanup:
+ VIR_FREE(cpumap);
+ return ret;
}
static PyObject *
--
1.7.11.2
7:17 a.m.
New subject: [libvirt] [PATCH v3] python: cleanup vcpu related binding APIs
On 28.09.2012 10:27, Guannan Ren wrote:
libvirt_virDomainGetVcpus: add error handling, return -1 instead of
None
libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
check the type of argument
make use of libvirt_boolUnwrap
Set bitmap according to these values which are contained in given
argument of vcpu tuple and turn off these bit corresponding to
missing vcpus in argument tuple
The original way ignored the error info from PyTuple_GetItem
if index is out of range.
"IndexError: tuple index out of range"
The error message will only be raised on next command in interactive mode.
---
python/libvirt-override.c | 163 ++++++++++++++++++++++++++++++++++------------
1 file changed, 121 insertions(+), 42 deletions(-)
ACK you've addressed all my concerns.
Michal
8:50 a.m.
New subject: [libvirt] [PATCH v3] python: cleanup vcpu related binding APIs
On 10/08/2012 08:17 PM, Michal Privoznik wrote:
On 28.09.2012 10:27, Guannan Ren wrote:
> libvirt_virDomainGetVcpus: add error handling, return -1 instead of None
> libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
> check the type of argument
> make use of libvirt_boolUnwrap
>
> Set bitmap according to these values which are contained in given
> argument of vcpu tuple and turn off these bit corresponding to
> missing vcpus in argument tuple
>
> The original way ignored the error info from PyTuple_GetItem
> if index is out of range.
> "IndexError: tuple index out of range"
> The error message will only be raised on next command in interactive mode.
> ---
> python/libvirt-override.c | 163 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 121 insertions(+), 42 deletions(-)
>
ACK you've addressed all my concerns.
Michal
Thanks and pushed.
Guannan
12:33 p.m.
New subject: [libvirt] [PATCH 2/2] python: return error if PyObject obj is NULL for unwrapper helper functions
The result is indeterminate for NULL argument to python
functions as follows. It's better to return negative value in
these situations.
PyObject_IsTrue will segfault if the argument is NULL
PyFloat_AsDouble(NULL) is -1.000000
PyLong_AsUnsignedLongLong(NULL) is 0.000000
---
python/typewrappers.c | 43 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/python/typewrappers.c b/python/typewrappers.c
index c525e59..7580689 100644
--- a/python/typewrappers.c
+++ b/python/typewrappers.c
@@ -124,6 +124,11 @@ libvirt_intUnwrap(PyObject *obj, int *val)
{
long long_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
/* If obj is type of PyInt_Type, PyInt_AsLong converts it
* to C long type directly. If it is of PyLong_Type, PyInt_AsLong
* will call PyLong_AsLong() to deal with it automatically.
@@ -151,6 +156,11 @@ libvirt_uintUnwrap(PyObject *obj, unsigned int *val)
{
long long_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
long_val = PyInt_AsLong(obj);
if ((long_val == -1) && PyErr_Occurred())
return -1;
@@ -170,6 +180,11 @@ libvirt_longUnwrap(PyObject *obj, long *val)
{
long long_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
long_val = PyInt_AsLong(obj);
if ((long_val == -1) && PyErr_Occurred())
return -1;
@@ -183,6 +198,11 @@ libvirt_ulongUnwrap(PyObject *obj, unsigned long *val)
{
long long_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
long_val = PyInt_AsLong(obj);
if ((long_val == -1) && PyErr_Occurred())
return -1;
@@ -202,6 +222,11 @@ libvirt_longlongUnwrap(PyObject *obj, long long *val)
{
long long llong_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
/* If obj is of PyInt_Type, PyLong_AsLongLong
* will call PyInt_AsLong() to handle it automatically.
*/
@@ -219,6 +244,11 @@ libvirt_ulonglongUnwrap(PyObject *obj, unsigned long long *val)
unsigned long long ullong_val = -1;
long long llong_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
/* The PyLong_AsUnsignedLongLong doesn't check the type of
* obj, only accept argument of PyLong_Type, so we check it instead.
*/
@@ -247,6 +277,11 @@ libvirt_doubleUnwrap(PyObject *obj, double *val)
{
double double_val;
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
double_val = PyFloat_AsDouble(obj);
if ((double_val == -1) && PyErr_Occurred())
return -1;
@@ -260,8 +295,12 @@ libvirt_boolUnwrap(PyObject *obj, bool *val)
{
int ret;
- ret = PyObject_IsTrue(obj);
- if (ret < 0)
+ if (!obj) {
+ PyErr_SetString(PyExc_TypeError, "unexpected type");
+ return -1;
+ }
+
+ if ((ret = PyObject_IsTrue(obj)) < 0)
return ret;
*val = ret > 0;
--
1.7.11.2
8:51 a.m.
New subject: [libvirt] [PATCH 2/2] python: return error if PyObject obj is NULL for unwrapper helper functions
On 26.09.2012 19:33, Guannan Ren wrote:
The result is indeterminate for NULL argument to python
functions as follows. It's better to return negative value in
these situations.
PyObject_IsTrue will segfault if the argument is NULL
PyFloat_AsDouble(NULL) is -1.000000
PyLong_AsUnsignedLongLong(NULL) is 0.000000
---
python/typewrappers.c | 43 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
ACK
Michal
3:28 a.m.
New subject: [libvirt] [PATCH 2/2] python: return error if PyObject obj is NULL for unwrapper helper functions
On 09/27/2012 09:51 PM, Michal Privoznik wrote:
On 26.09.2012 19:33, Guannan Ren wrote:
> The result is indeterminate for NULL argument to python
> functions as follows. It's better to return negative value in
> these situations.
>
> PyObject_IsTrue will segfault if the argument is NULL
> PyFloat_AsDouble(NULL) is -1.000000
> PyLong_AsUnsignedLongLong(NULL) is 0.000000
> ---
> python/typewrappers.c | 43 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
ACK
Michal
Thanks and pushed
Guannan
4463
days inactive
4475
days old
9 comments
2 participants
participants (2)
-
Guannan Ren -
Michal Privoznik