[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 vcpus which are list in tuple pycpumap, trun off the rest in cpumap. The original way ignored the error info from PyTuple_GetItem if pos is out of bounds. "IndexError: tuple index out of range" The error message will only be raised on next command in interactive mode. --- python/libvirt-override.c | 173 +++++++++++++++++++++++++++++--------------- 1 files changed, 114 insertions(+), 59 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 56f96ba..2788546 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) @@ -1388,54 +1394,80 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, PyObject *info = PyTuple_New(4); 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 (PyTuple_SetItem(info, 0, PyInt_FromLong((long)cpuinfo[i].number)) < 0) + { + Py_DECREF(info); + goto cleanup; + } + + if (PyTuple_SetItem(info, 1, PyInt_FromLong((long)cpuinfo[i].state)) < 0) + { + Py_DECREF(info); + goto cleanup; + } + + if (PyTuple_SetItem(info, 2, PyLong_FromLongLong((long long)cpuinfo[i].cpuTime)) < 0) + { + Py_DECREF(info); + goto cleanup; + } + + if (PyTuple_SetItem(info, 3, PyInt_FromLong((long)cpuinfo[i].cpu)) < 0) + { + Py_DECREF(info); + goto cleanup; + } + if (PyList_SetItem(pycpuinfo, i, info) < 0) + goto cleanup; } + for (i = 0 ; i < dominfo.nrVirtCpu ; i++) { PyObject *info = PyTuple_New(VIR_NODEINFO_MAXCPUS(nodeinfo)); - int j; if (info == NULL) goto cleanup; + + int j; for (j = 0 ; j < VIR_NODEINFO_MAXCPUS(nodeinfo) ; j++) { - PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))); + if (PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))) < 0) + { + Py_DECREF(info); + goto cleanup; + } } - PyList_SetItem(pycpumap, i, info); + if (PyList_SetItem(pycpumap, i, info) < 0) + goto cleanup; } - 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); - /* NB, Py_DECREF is a badly defined macro, so we require - * braces here to avoid 'ambiguous else' warnings from - * the compiler. - * NB. this comment is true at of time of writing wrt to - * at least python2.5. - */ - if (pyretval) { Py_DECREF(pyretval); } - if (pycpuinfo) { Py_DECREF(pycpuinfo); } - if (pycpumap) { Py_DECREF(pycpumap); } - return VIR_PY_NONE; + Py_XDECREF(pyretval); + Py_XDECREF(pycpuinfo); + Py_XDECREF(pycpumap); + 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", @@ -1451,37 +1483,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(); - truth = PyBool_FromLong(1); - for (i = 0 ; i < VIR_NODEINFO_MAXCPUS(nodeinfo) ; i++) { + tuple_size = PyTuple_Size(pycpumap); + if (tuple_size == -1) + goto cleanup; + + 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; @@ -1498,27 +1543,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.7.5
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 | 26 +++++++++++++++++++++++--- 1 files changed, 23 insertions(+), 3 deletions(-) diff --git a/python/typewrappers.c b/python/typewrappers.c index c525e59..58770be 100644 --- a/python/typewrappers.c +++ b/python/typewrappers.c @@ -124,6 +124,9 @@ libvirt_intUnwrap(PyObject *obj, int *val) { long long_val; + if (!obj) + 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 +154,9 @@ libvirt_uintUnwrap(PyObject *obj, unsigned int *val) { long long_val; + if (!obj) + return -1; + long_val = PyInt_AsLong(obj); if ((long_val == -1) && PyErr_Occurred()) return -1; @@ -170,6 +176,9 @@ libvirt_longUnwrap(PyObject *obj, long *val) { long long_val; + if (!obj) + return -1; + long_val = PyInt_AsLong(obj); if ((long_val == -1) && PyErr_Occurred()) return -1; @@ -183,6 +192,9 @@ libvirt_ulongUnwrap(PyObject *obj, unsigned long *val) { long long_val; + if (!obj) + return -1; + long_val = PyInt_AsLong(obj); if ((long_val == -1) && PyErr_Occurred()) return -1; @@ -202,6 +214,9 @@ libvirt_longlongUnwrap(PyObject *obj, long long *val) { long long llong_val; + if (!obj) + return -1; + /* If obj is of PyInt_Type, PyLong_AsLongLong * will call PyInt_AsLong() to handle it automatically. */ @@ -219,6 +234,9 @@ libvirt_ulonglongUnwrap(PyObject *obj, unsigned long long *val) unsigned long long ullong_val = -1; long long llong_val; + if (!obj) + 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 +265,9 @@ libvirt_doubleUnwrap(PyObject *obj, double *val) { double double_val; + if (!obj) + return -1; + double_val = PyFloat_AsDouble(obj); if ((double_val == -1) && PyErr_Occurred()) return -1; @@ -260,9 +281,8 @@ libvirt_boolUnwrap(PyObject *obj, bool *val) { int ret; - ret = PyObject_IsTrue(obj); - if (ret < 0) - return ret; + if (!obj || ((ret = PyObject_IsTrue(obj)) < 0)) + return -1; *val = ret > 0; return 0; -- 1.7.7.5
On 04/17/2012 11:45 AM, 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 | 26 +++++++++++++++++++++++--- 1 files changed, 23 insertions(+), 3 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/17/2012 11:49 AM, Eric Blake wrote:
On 04/17/2012 11:45 AM, 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 | 26 +++++++++++++++++++++++--- 1 files changed, 23 insertions(+), 3 deletions(-)
ACK.
I spoke too soon. On looking at it more, I'm wondering if all callers do the right thing in this case. It would probably be better if we explicitly raised a python exception before returning -1, so that callers can blindly assume that a return of -1 means that the correct python error is already present. That is, instead of: if (!obj) return -1; should we instead be doing something like: if (!obj) { PyErr_SetString(PyExc_TypeError, "unexpected type"); return -1; } Or are we guaranteed that the only way obj is NULL is if the caller already encountered an error, so there is already a python error set? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/18/2012 01:53 AM, Eric Blake wrote:
On 04/17/2012 11:45 AM, 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 | 26 +++++++++++++++++++++++--- 1 files changed, 23 insertions(+), 3 deletions(-)
ACK. I spoke too soon. On looking at it more, I'm wondering if all callers do the right thing in this case. It would probably be better if we explicitly raised a python exception before returning -1, so that callers can blindly assume that a return of -1 means that the correct
On 04/17/2012 11:49 AM, Eric Blake wrote: python error is already present.
That is, instead of:
if (!obj) return -1;
should we instead be doing something like:
if (!obj) { PyErr_SetString(PyExc_TypeError, "unexpected type"); return -1; }
Or are we guaranteed that the only way obj is NULL is if the caller already encountered an error, so there is already a python error set?
Generally, when an obj is set to NULL by C/Python function such as PyTuple_GetItem, the error has been set, we just need a return value NULL in python bindings to user python caller to trigger it out. But if the value is set by ourselves( a little crazy :) ), then no error is set. The negative value could let the python binding API return NULL to user python caller for error showing. For the NULL set by ourselves, we could try to avoid it in codes I think.
On 04/17/2012 11:45 AM, Guannan Ren wrote:
*libvirt_virDomainGetVcpus: add error handling, return -1 instead of None *libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags: make use of libvirt_boolUnwrap
set vcpus which are list in tuple pycpumap, trun off the rest in cpumap. The original way ignored the error info from PyTuple_GetItem if pos is out of bounds. "IndexError: tuple index out of range" The error message will only be raised on next command in interactive mode. --- python/libvirt-override.c | 173 +++++++++++++++++++++++++++++--------------- 1 files changed, 114 insertions(+), 59 deletions(-)
@@ -1388,54 +1394,80 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, PyObject *info = PyTuple_New(4); 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 (PyTuple_SetItem(info, 0, PyInt_FromLong((long)cpuinfo[i].number)) < 0)
Ouch. PyInt_FromLong can return NULL (on OOM situations), but PyTuple_SetItem() doesn't take too kindly to NULL arguments. You need to use temporary variables through here.
for (i = 0 ; i < dominfo.nrVirtCpu ; i++) { PyObject *info = PyTuple_New(VIR_NODEINFO_MAXCPUS(nodeinfo)); - int j; if (info == NULL) goto cleanup; + + int j;
Why did you sink the declaration of j? We require C99, so it works, but it's not our usual style.
for (j = 0 ; j < VIR_NODEINFO_MAXCPUS(nodeinfo) ; j++) { - PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))); + if (PyTuple_SetItem(info, j, PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen, i, j))) < 0)
Again, this needs a temporary to avoid calling PyTuple_SetItem(NULL).
+ + if ((!flag) || (libvirt_boolUnwrap(flag, &b) < 0))
Redundant parenthesis. This could be written: if (!flag || libvirt_boolUnwrap(flag, &b) < 0) Not quite ready, but I like the direction that your headed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Guannan Ren