4:04 p.m.
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
3:21 p.m.
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
3:25 p.m.
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 :|
3:31 p.m.
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
3:32 p.m.
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 :|
4:17 p.m.
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
4:27 p.m.
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 :|
1:43 p.m.
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.
4:15 p.m.
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 :|
4:51 a.m.
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
6:58 a.m.
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 :|
7:20 a.m.
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
8:16 a.m.
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 :|
6029
days inactive
6034
days old
12 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Richard W.M. Jones