Re: [libvirt] [PATCH] Free object after *Destroy in python bindings

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