12:45 p.m.
*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
12:45 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 | 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
12:49 p.m.
New subject: [libvirt] [PATCH 2/2] python: return error if PyObject obj is NULL for unwrapper helper functions
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:53 p.m.
New subject: [libvirt] [PATCH 2/2] python: return error if PyObject obj is NULL for unwrapper helper functions
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:55 p.m.
New subject: [libvirt] [PATCH 2/2] python: return error if PyObject obj is NULL for unwrapper helper functions
On 04/18/2012 01:53 AM, Eric Blake wrote:
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?
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.
1:05 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4602
days inactive
4603
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Guannan Ren