Daniel P. Berrange wrote:
> On Mon, May 19, 2008 at 05:17:07PM -0400, Cole Robinson wrote:
>> Daniel P. Berrange wrote:
>>> On Mon, May 19, 2008 at 04:21:55PM -0400, Cole Robinson wrote:
>>>> Cole Robinson wrote:
>>>>> The patch below fixes an issue in the python bindings with the
>>>>> vir*Destroy methods. After the object is successfully destroyed,
>>>>> the payload is cleared, using 'self._o = None'. This unfortunately
>>>>> screws up virt object reference counts, as the payload should be
>>>>> free'd using the appropriate vir*Free function.
>>>>>
>>>> Hmm, I might be wrong about this. Reading the virDomainDestroy
>>>> description in libvirt.c, destroy is supposed to free the
>>>> domain object if the operation is successful. The qemu driver
>>>> currently does not do this. In fact, from what I gather, none
>>>> of the drivers do this.
>>> The docs are wrong. Destory merely hard-kills the object being managed.
>>> It does not free memory associated with the object.
>>>
>>>> If the virDomainDestroy comment is correct, then the original
>>>> python statement is sufficient, but the drivers need to have
>>>> a few virDomainFree's added.
>>> I believe your patch is correct - python should not be dropping its
>>> reference to the underlying C object, afte invoking the destroy method.
>>> It should be only be dropped after free is called
>> I guess it should also be asked if the python bindings should be doing
>> anything to the domain/network/... object after a destroy _at_all_.
>
> I think destroy should work in same way as any other operation. The
> only method call which is special is the virDomainFree().
>
>> Shutdown and reboot don't alter the object, even undefine doesn't. The
>> original statement just seems to be compensating for the original idea
>> that Destroy was freeing the underlying object.
>>
>> Taking the statement out entirely would be a change in existing behavior,
>> but wouldn't break any existing use of the bindings and would most likely
>> prevent more memory leaks, since the automatic garbage collection would
>> now actually be able to free the object appropriately.
>
> It would actually resolve bugs if we fixed the python objects behaviour
> here. As you say the garbage collector will then 'do the right thing'
> so it should not have any negative impact on apps to fix this issue.
>
> Dan
Here is the correct patch then: remove any special case cleanup for the
destroy functions.
Thanks,
Cole
diff --git a/python/generator.py b/python/generator.py
index cb57bff..68a7203 100755
--- a/python/generator.py
+++ b/python/generator.py
@@ -628,12 +628,7 @@ function_classes = {}
function_classes["None"] = []
-function_post = {
- 'virDomainDestroy': "self._o = None",
- 'virNetworkDestroy': "self._o = None",
- 'virStoragePoolDestroy': "self._o = None",
- 'virStorageVolDestroy': "self._o = None",
-}
+function_post = {}
# Functions returning an integral type which need special rules to
# check for errors and raise exceptions.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list