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

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(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 :) > Note that it's a loop.

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