[libvirt] [PATCH v2 python 1/2] minor clean-up for libvirt_virDomainPin*

[Peter Krempa]

On Fri, Oct 28, 2016 at 13:41:09 +0300, Konstantin Neumoin wrote:
> All libvirt_virDomainPin* functions do the same thing for convert
> pycpumap to cpumap, so this patch moves all common logic to new
> helper - virPyCpuMapToChar.
> 
> Signed-off-by: Konstantin Neumoin <kneumoin at virtuozzo.com>
> ---
>  libvirt-override.c | 131 +++++------------------------------------------------
>  libvirt-utils.c    |  60 ++++++++++++++++++++++++
>  libvirt-utils.h    |   5 ++
>  3 files changed, 76 insertions(+), 120 deletions(-)

Nice cleanup.

> 
> diff --git a/libvirt-override.c b/libvirt-override.c
> index fa3e2ca..ba0d87c 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c

The usage here looks good to me.

> diff --git a/libvirt-utils.c b/libvirt-utils.c
> index 2bf7519..aaf4bea 100644
> --- a/libvirt-utils.c
> +++ b/libvirt-utils.c
> @@ -586,3 +586,63 @@ virPyDictToTypedParams(PyObject *dict,
>      return ret;
>  }
>  #endif /* LIBVIR_CHECK_VERSION(1, 1, 0) */
> +
> +
> +/* virPyCpuMapToChar

virPyCpumapConvert perhaps?


> + * @cpunum: the number of cpus

This is the number of physical cpus of the host we are talking to. It
should be noted.

> + * @pycpumap: source Py cpu map

You should note that this should be a python tuple of bools.

> + * @cpumapptr: destination cpu map
> + * @cpumaplen: destination cpu map length
> + *
> + * Helper function to convert a pycpumap to char*
> + *
> + * return -1 on error.

Success case is not documented. Also the fact that the proper python
error is set in cases when it makes sense is not documented.

> + */
> +int
> +virPyCpuMapToChar(int cpunum,
> +                  PyObject *pycpumap,
> +                  unsigned char **cpumapptr,
> +                  int *cpumaplen)
> +{
> +    int tuple_size;
> +    size_t i;
> +    int i_retval = -1;

We usually call this "ret".

> +    *cpumapptr = NULL;
> +
> +    if (!PyTuple_Check(pycpumap)) {
> +        PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required");
> +        goto exit;
> +    }
> +
> +    *cpumaplen = VIR_CPU_MAPLEN(cpunum);
> +
> +    if ((tuple_size = PyTuple_Size(pycpumap)) == -1)
> +        goto exit;
> +
> +    if (VIR_ALLOC_N(*cpumapptr, *cpumaplen) < 0) {
> +        PyErr_NoMemory();
> +        goto exit;
> +    }
> +
> +    for (i = 0; i < tuple_size; i++) {
> +        PyObject *flag = PyTuple_GetItem(pycpumap, i);
> +        bool b;
> +
> +        if (!flag || libvirt_boolUnwrap(flag, &b) < 0) {
> +            VIR_FREE(*cpumapptr);
> +            goto exit;
> +        }
> +
> +        if (b)
> +            VIR_USE_CPU(*cpumapptr, i);
> +        else
> +            VIR_UNUSE_CPU(*cpumapptr, i);
> +    }
> +
> +    for (; i < cpunum; i++)
> +        VIR_UNUSE_CPU(*cpumapptr, i);
> +
> +    i_retval = 0;
> +exit:

We indent labels by one space in libvirt so that git grep -p works well.
Also we have a naming convention for labels passed both on success and
error and call them "cleanup".


Finally the label is not necessary in this case as you are cleaning up
the pointer in the code and thus the label can be avoided completely and
you can return -1 directly. This will also avoid the "ret" or "i_retval"
variable.

> +    return i_retval;
> +}

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161101/bf1f7c50/attachment-0001.sig>