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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list