[libvirt] [PATCH] Free object after *Destroy in python bindings
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. Thanks, Cole diff --git a/python/generator.py b/python/generator.py index cb57bff..9421981 100755 --- a/python/generator.py +++ b/python/generator.py @@ -629,10 +629,10 @@ function_classes = {} function_classes["None"] = [] function_post = { - 'virDomainDestroy': "self._o = None", - 'virNetworkDestroy': "self._o = None", - 'virStoragePoolDestroy': "self._o = None", - 'virStorageVolDestroy': "self._o = None", + 'virDomainDestroy': "del(self)", + 'virNetworkDestroy': "del(self)", + 'virStoragePoolDestroy': "del(self)", + 'virStorageVolDestroy': "del(self)", } # Functions returning an integral type which need special rules to
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. If the virDomainDestroy comment is correct, then the original python statement is sufficient, but the drivers need to have a few virDomainFree's added. Anybody have more info on this? Thanks, Cole
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 Regards, Dan -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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
Regards, Dan
Worth noting that there are actually places in the code that make the assumption that virDomainDestroy _does_ free the domain object, such as the remote driver and virsh. I'll send out a patch for those as well. Thanks, Cole
On Mon, May 19, 2008 at 04:31:36PM -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
Worth noting that there are actually places in the code that make the assumption that virDomainDestroy _does_ free the domain object, such as the remote driver and virsh. I'll send out a patch for those as well.
Yes, those would be bugs / memory leaks. Dan -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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_. 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. Thanks, Cole
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 -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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.
On Tue, May 20, 2008 at 02:43:42PM -0400, Cole Robinson wrote:
Here is the correct patch then: remove any special case cleanup for the destroy functions.
I've applied this to CVS Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, May 19, 2008 at 09:25:55PM +0100, Daniel P. Berrange wrote:
The docs are wrong. Destory merely hard-kills the object being managed. It does not free memory associated with the object.
No, the documentation says it frees the objects (and has done forever), so it should free them. I have code which depends on this behaviour. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On Tue, May 20, 2008 at 10:51:43AM +0100, Richard W.M. Jones wrote:
On Mon, May 19, 2008 at 09:25:55PM +0100, Daniel P. Berrange wrote:
The docs are wrong. Destory merely hard-kills the object being managed. It does not free memory associated with the object.
No, the documentation says it frees the objects (and has done forever), so it should free them. I have code which depends on this behaviour.
It depends on behaviour which does *not* exist so is already broken. With inactive domains free'ing the object after destroy is non-sensical because the domain still exists, merely in the shutoff state. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, May 20, 2008 at 12:58:15PM +0100, Daniel P. Berrange wrote:
On Tue, May 20, 2008 at 10:51:43AM +0100, Richard W.M. Jones wrote:
On Mon, May 19, 2008 at 09:25:55PM +0100, Daniel P. Berrange wrote:
The docs are wrong. Destory merely hard-kills the object being managed. It does not free memory associated with the object.
No, the documentation says it frees the objects (and has done forever), so it should free them. I have code which depends on this behaviour.
It depends on behaviour which does *not* exist so is already broken. With inactive domains free'ing the object after destroy is non-sensical because the domain still exists, merely in the shutoff state.
If 'implements correctly the documented API' is 'broken', well then ... I looked back at an old implementation of libvirt (June '07) just to see what the behaviour used to be. Note virDomainUndefine & virNetworkUndefine (oops!): xen qemu test ---------------------------------------------------------------------- virDomainDestroy no no no virNetworkDestroy no no no virDomainUndefine no frees no virNetworkDestroy no no no virNetworkUndefine no frees no For comparison in current CVS none of those calls free the object. So this change is safe. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On Tue, May 20, 2008 at 01:20:17PM +0100, Richard W.M. Jones wrote:
On Tue, May 20, 2008 at 12:58:15PM +0100, Daniel P. Berrange wrote:
On Tue, May 20, 2008 at 10:51:43AM +0100, Richard W.M. Jones wrote:
On Mon, May 19, 2008 at 09:25:55PM +0100, Daniel P. Berrange wrote:
The docs are wrong. Destory merely hard-kills the object being managed. It does not free memory associated with the object.
No, the documentation says it frees the objects (and has done forever), so it should free them. I have code which depends on this behaviour.
It depends on behaviour which does *not* exist so is already broken. With inactive domains free'ing the object after destroy is non-sensical because the domain still exists, merely in the shutoff state.
If 'implements correctly the documented API' is 'broken', well then ...
I looked back at an old implementation of libvirt (June '07) just to see what the behaviour used to be. Note virDomainUndefine & virNetworkUndefine (oops!):
xen qemu test ---------------------------------------------------------------------- virDomainDestroy no no no virNetworkDestroy no no no virDomainUndefine no frees no virNetworkDestroy no no no virNetworkUndefine no frees no
For comparison in current CVS none of those calls free the object.
Fortunately, the QEMU driver is never invoked directly by end user apps, only ever called via the daemon, so those broken semantics in the QEMU driver didn't leak out into the public users. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Richard W.M. Jones