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

[Alex Jia]

On 01/06/2012 05:39 PM, Osier Yang wrote:
> 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.
Oh, yeah, you're right, I will  send v3 patch according to your comment,
but not now, because I want to use 'virTypedParameterArrayClear' new
method to do this instead, you know I haven't resolved Makefile issue
at present, maybe, you may help me about this again, and I have raised
this issue to libvir-list, however, I haven't got a reply now.

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