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

On 01/06/2012 04:36 PM, Osier Yang wrote: > On 2011年12月30日 14:57, ajia(a)redhat.com wrote: >> From: Alex Jia&lt;ajia(a)redhat.com&gt; >> >> 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&lt;ajia(a)redhat.com&gt; >> --- >> 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); >> } >