[libvirt] [PATCHv2] python: Fix problems of virDomain{Set, Get}BlkioParameters bindings

[Osier Yang]

On 2011年12月30日 14:57, ajia at redhat.com wrote:
> From: Alex Jia<ajia at redhat.com>
>
> The parameter 'device_weight' is a string, however, the 'VIR_TYPED_PARAM_STRING'
> type condition is missed by libvirt_virDomain{Set, Get}BlkioParameters bindings,
> the result is we can't get or change 'device_weight' value.
>
> * python/libvirt-override.c: Add missing 'VIR_TYPED_PARAM_STRING' condition into
> libvirt_virDomain{Set, Get}BlkioParameters bindings and free allocated memory.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=770795
>
>
> Signed-off-by: Alex Jia<ajia at redhat.com>
> ---
>   python/libvirt-override.c |   20 ++++++++++++++++++++
>   1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> index d2aad0f..38a35b8 100644
> --- a/python/libvirt-override.c
> +++ b/python/libvirt-override.c
> @@ -726,6 +726,10 @@ libvirt_virDomainSetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
>               }
>               break;
>
> +        case VIR_TYPED_PARAM_STRING:
> +            params[i].value.s = PyString_AsString(val);
> +            break;
> +
>           default:
>               free(params);
>               return VIR_PY_INT_FAIL;
> @@ -735,6 +739,11 @@ libvirt_virDomainSetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
>       LIBVIRT_BEGIN_ALLOW_THREADS;
>       i_retval = virDomainSetBlkioParameters(domain, params, nparams, flags);
>       LIBVIRT_END_ALLOW_THREADS;
> +
> +    for(i=0; i<  nparams; i++)
> +        if (params[i].type == VIR_TYPED_PARAM_STRING)
> +            free(params[i].value.s);

IMHO this is not right, anything returned by PyString_AsString should
*not* be deallocated. It doesn't returns a copy but a pointer reference.
Note that Python still wants to see "params[i].value.s", and I guess
Python will be responsiable to do the cleanup. But I'm not quite
convinced about that, not formiliar with Python's memory management.

> +
>       if (i_retval<  0) {
>           free(params);
>           return VIR_PY_INT_FAIL;
> @@ -811,6 +820,10 @@ libvirt_virDomainGetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
>               val = PyBool_FromLong((long)params[i].value.b);
>               break;
>
> +        case VIR_TYPED_PARAM_STRING:
> +            val = PyString_FromString((char *)params[i].value.s);
> +            break;
> +
>           default:
>               free(params);
>               Py_DECREF(info);
> @@ -819,7 +832,14 @@ libvirt_virDomainGetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>           key = libvirt_constcharPtrWrap(params[i].field);
>           PyDict_SetItem(info, key, val);
> +        Py_DECREF(key);
> +        Py_DECREF(val);

Why it needs to descrease the reference count of "key" and "val"?

>       }
> +
> +    for(i=0; i<  nparams; i++)
> +        if (params[i].type == VIR_TYPED_PARAM_STRING)
> +            free(params[i].value.s);
> +

params[i].value.s also needs to be free()'ed in default branch, before
the function returns.

>       free(params);
>       return(info);
>   }