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

[Osier Yang]

On 2012年01月06日 17:11, Alex Jia wrote:
> On 01/06/2012 04:36 PM, Osier Yang wrote:
>> 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.
> Yeah, I know python.org says "PyString_AsString() must not be
> deallocated, because its
> pointer refers to the internal buffer of string//, not a copy." in here,
> I want o free the contents
> of string, may be free(val) is more clear.
>>
>>> +
>>>       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"?
> PyDict_SetItem() puts something in a dictionary is "storing" it,
> therefore PyDict_SetItem() INCREFs both the key and the value.
>>
>>>       }
>>> +
>>> +    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.
> I don't think so,  if 'default' branch is hit, it means "params[i].type
> != VIR_TYPED_PARAM_STRING", in other words, codes haven't called
> 'PyString_FromString()' method, hence we don't need to free memory,
> please correct me if I'm wrong :)
>

Note that it's a loop.

> Thanks for your comments again,
> Alex
> ||||
>>
>>>       free(params);
>>>       return(info);
>>>   }
>>
>