[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]