[libvirt] [PATCHv2] python: Fix problems of virDomain{Set, Get}BlkioParameters bindings
From: Alex Jia <ajia@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@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); + 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); } + + for(i=0; i < nparams; i++) + if (params[i].type == VIR_TYPED_PARAM_STRING) + free(params[i].value.s); + free(params); return(info); } -- 1.7.1
On 2011年12月30日 14:57, ajia@redhat.com wrote:
From: Alex Jia<ajia@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@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.
+ 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"?
} + + 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.
free(params); return(info); }
On 2011年12月30日 14:57, ajia@redhat.com wrote:
From: Alex Jia<ajia@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@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
On 01/06/2012 04:36 PM, Osier Yang wrote: 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); }
On 2012年01月06日 17:11, Alex Jia wrote:
On 2011年12月30日 14:57, ajia@redhat.com wrote:
From: Alex Jia<ajia@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@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
On 01/06/2012 04:36 PM, Osier Yang wrote: 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); }
On 2012年01月06日 17:11, Alex Jia wrote:
On 2011年12月30日 14:57, ajia@redhat.com wrote:
From: Alex Jia<ajia@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@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
On 01/06/2012 04:36 PM, Osier Yang wrote: 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
On 01/06/2012 05:39 PM, Osier Yang wrote: 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); }
participants (3)
-
ajia＠redhat.com -
Alex Jia
-
Osier Yang