[libvirt] [PATCH 2/2 v3] Python: modify other functions to use {get, set}PyVirTypedParameter

[Eric Blake]

On 01/28/2012 07:53 AM, Guannan Ren wrote:
>         *libvirt_virDomainBlockStatsFlags
>         *libvirt_virDomainGetSchedulerParameters
>         *libvirt_virDomainGetSchedulerParametersFlags
>         *libvirt_virDomainSetSchedulerParameters
>         *libvirt_virDomainSetSchedulerParametersFlags
>         *libvirt_virDomainSetBlkioParameters
>         *libvirt_virDomainGetBlkioParameters
>         *libvirt_virDomainSetMemoryParameters
>         *libvirt_virDomainGetMemoryParameters
>         *libvirt_virDomainSetBlockIoTune
>         *libvirt_virDomainGetBlockIoTune
> ---
>  python/libvirt-override-api.xml |   10 +-
>  python/libvirt-override.c       |  789 ++++++++++----------------------------
>  2 files changed, 213 insertions(+), 586 deletions(-)
> 
> diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml
> index 748aa17..934a832 100644
> --- a/python/libvirt-override-api.xml
> +++ b/python/libvirt-override-api.xml
> @@ -144,7 +144,7 @@
>      </function>
>      <function name='virDomainBlockStatsFlags' file='python'>
>        <info>Extracts block device statistics parameters of a running domain</info>
> -      <return type='virTypedParameterPtr' info='None in case of error, returns a dictionary of params'/>
> +      <return type='int' info='returns a dictionary of params in case of success, -1 in case of error'/>

type='int' is wrong; we're returning a dictionary on success, so I would
argue that we return None on error (and NULL if we want to raise a
python exception, but that doesn't have to be part of the python
self-documentation).  Same for all the getter functions.

> +++ b/python/libvirt-override.c
> @@ -304,11 +304,13 @@ libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
>  
>  static PyObject *
>  libvirt_virDomainBlockStatsFlags(PyObject *self ATTRIBUTE_UNUSED,
> -                                 PyObject *args) {
> +                                 PyObject *args)
> +{
>      virDomainPtr domain;
>      PyObject *pyobj_domain, *info;
> +    PyObject *ret = NULL;
>      int i_retval;
> -    int nparams = 0, i;
> +    int nparams = 0;
>      unsigned int flags;
>      virTypedParameterPtr params;
>      const char *path;
> @@ -323,69 +325,33 @@ libvirt_virDomainBlockStatsFlags(PyObject *self ATTRIBUTE_UNUSED,
>      LIBVIRT_END_ALLOW_THREADS;
>  
>      if (i_retval < 0)
> -        return VIR_PY_NONE;
> +        return VIR_PY_INT_FAIL;

This change doesn't make sense.

>  
>      if ((params = malloc(sizeof(*params)*nparams)) == NULL)
> -        return VIR_PY_NONE;
> +        return PyErr_NoMemory();

This one looks good.  But same comment as in 1/2 about skipping the
malloc() and just using 'return PyDict_New();' for an empty dictionary
(or python exception) if i_retval is 0.

>  
>      LIBVIRT_BEGIN_ALLOW_THREADS;
>      i_retval = virDomainBlockStatsFlags(domain, path, params, &nparams, flags);
>      LIBVIRT_END_ALLOW_THREADS;
>  
>      if (i_retval < 0) {
> -        free(params);
> -        return VIR_PY_NONE;
> -    }
> -
> -    /* convert to a Python tuple of long objects */
> -    if ((info = PyDict_New()) == NULL) {
> -        free(params);
> -        return VIR_PY_NONE;
> +        ret = VIR_PY_INT_FAIL;
> +        goto fail;

Should still return VIR_PY_NONE if i_retval is < 0.

> +    info = getPyVirTypedParameter(params, nparams);

Yay - the benefits of factoring common code into a helper method.

> +    if (!info)
> +        goto fail;
>  
> +    virTypedParameterArrayClear(params, nparams);
>      free(params);
> -    return(info);
> +    return info;
> +fail:
> +    virTypedParameterArrayClear(params, nparams);
> +    free(params);
> +    return ret;

You could share these, by using the label cleanup, and:

    ret = info;
cleanup;
    virTypedParameterArrayClear(params, nparams);
    VIR_FREE(params);
    return ret;

Similar comments on all the getter functions.

> @@ -683,85 +589,48 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self ATTRIBUTE_UNUSED,
>      free(c_retval);
>  
>      if ((params = malloc(sizeof(*params)*nparams)) == NULL)
> -        return VIR_PY_INT_FAIL;
> +        return PyErr_NoMemory();
>  
>      LIBVIRT_BEGIN_ALLOW_THREADS;
>      i_retval = virDomainGetSchedulerParameters(domain, params, &nparams);
>      LIBVIRT_END_ALLOW_THREADS;
>  
>      if (i_retval < 0) {
> -        free(params);
> -        return VIR_PY_INT_FAIL;
> +        ret = VIR_PY_INT_FAIL;
> +        goto fail;
>      }

Looks good.

> +    if (!setPyVirTypedParameter(info, params, nparams))
> +        goto fail;
>  
>      LIBVIRT_BEGIN_ALLOW_THREADS;
>      i_retval = virDomainSetSchedulerParameters(domain, params, nparams);

Same comments as on 1/2 - all the setter functions should be remembering
and using the new params array created by setPyVirtTypedParameter, and
not the original params from the getter function.  Probably not worth
rewriting this patch until we are happy with how the factoring for 1/2
works out, since it will be copying the same design pattern to multiple
functions.

I'll see about posting a patch that refactors the Makefile to let the
python glue code use libvirt_util, and we should get that in before
rebasing this series on top of that.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120202/744bf334/attachment-0001.sig>