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

[Alex Jia]

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 :)

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120106/d8529c88/attachment-0001.htm>