[Libvir] [PATCH] Python bindings: Use virDomain/NetworkGetConnect instead of storing connection in domain/network object
5 p.m.
Previously[1] I patched libvirt to add the virDomainGetConnect and
virNetworkGetConnect functions. The rationale for these is explained
here[2].
The attached patch makes the Python bindings use these functions instead
of storing the connection object explicitly.
Rich.
[1]https://www.redhat.com/archives/libvir-list/2007-June/msg00365.html
[2]https://www.redhat.com/archives/libvir-list/2007-June/msg00352.html
5:16 p.m.
On Mon, Jul 23, 2007 at 04:00:59PM +0100, Richard W.M. Jones wrote:
Previously[1] I patched libvirt to add the virDomainGetConnect and
virNetworkGetConnect functions. The rationale for these is explained
here[2].
The attached patch makes the Python bindings use these functions instead
of storing the connection object explicitly.
Hum ... The problem was the following:
- you create a connection
- you get a domain pointer from the connection
- you delete the reference from the connection (e.g. by exiting
the method) but keep the domain object
- the connection count of the python object goes down to 0
- _del() is called on the object
- the connection is closed at the C level
- you have a dandling domain python object whose C pointer is dead
Are you sure your patch is safe ?
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:29 p.m.
Daniel Veillard wrote:
On Mon, Jul 23, 2007 at 04:00:59PM +0100, Richard W.M. Jones wrote:
> Previously[1] I patched libvirt to add the virDomainGetConnect and
> virNetworkGetConnect functions. The rationale for these is explained
> here[2].
>
> The attached patch makes the Python bindings use these functions instead
> of storing the connection object explicitly.
Hum ... The problem was the following:
- you create a connection
- you get a domain pointer from the connection
At the C level, conn->uses is incremented when we get a new domain object.
- you delete the reference from the connection (e.g. by exiting
the method) but keep the domain object
- the connection count of the python object goes down to 0
- _del() is called on the object
- the connection is closed at the C level
conn->uses should still be >= 1 (reflecting the outstanding domain
object), so the connection shouldn't be closed.
- you have a dandling domain python object whose C pointer is
dead
Are you sure your patch is safe ?
Having said that, I will now go and construct a test case to see if this
really is safe.
garbage.collection++
Rich.
6:11 p.m.
This is my test case:
------------------------------------------
#!/usr/bin/python
import libvirt
def get_dom():
conn = libvirt.open ("xen");
dom = conn.lookupByName ("debian32fv");
return dom
dom = get_dom()
print "name = %s\n" % dom.name()
------------------------------------------
I modified src/hash.c to add a little bit of debug of the state of
conn->uses in virGetConnect, virFreeConnect, virGetDomain,
virFreeDomain. When running the program I see:
------------------------------------------
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virInitialize ()
libvirt: virConnectOpen (name=xen)
conn(0x6748c0) uses = 1
libvirt: do_open: proceeding with name=xen:///
libvirt: do_open: trying driver 0 (Test) ...
libvirt: do_open: driver 0 returned DECLINED
libvirt: do_open: trying driver 1 (QEMU) ...
libvirt: do_open: driver 1 returned DECLINED
libvirt: do_open: trying driver 2 (Xen) ...
libvirt: do_open: driver 2 returned SUCCESS
libvir: Remote error : Connection refused
libvir: warning : Failed to find the network: Is the daemon running ?
libvirt: virDomainLookupByName (conn=0x6748c0, name=debian32fv)
new domain: conn(0x6748c0) uses = 2
libvirt: virConnectClose (conn=0x6748c0)
conn(0x6748c0) uses = 1
libvirt: virDomainGetName (domain=0x679e20)
name = debian32fv
libvirt: virDomainFree (domain=0x679e20)
free domain: conn(0x6748c0) uses = 0
------------------------------------------
I would say that it's working as intended.
Rich.
6:40 p.m.
On Mon, Jul 23, 2007 at 05:11:09PM +0100, Richard W.M. Jones wrote:
This is my test case:
------------------------------------------
#!/usr/bin/python
import libvirt
def get_dom():
conn = libvirt.open ("xen");
dom = conn.lookupByName ("debian32fv");
return dom
dom = get_dom()
print "name = %s\n" % dom.name()
------------------------------------------
I would say that it's working as intended.
In this case yes, but if you add one further line at the end of
this example
conn2 = dom.get_connection()
And then have python garbage collect 'dom' and the original 'conn'
you're not safe because 'conn2' didn't increment the reference
count:
/*
* virDomainGetConnect:
* @dom: pointer to a domain
*
* Provides the connection pointer associated with a domain. The
* reference counter on the connection is not increased by this
* call.
*/
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
4:19 p.m.
Daniel P. Berrange wrote:
On Mon, Jul 23, 2007 at 05:11:09PM +0100, Richard W.M. Jones wrote:
> This is my test case:
>
> ------------------------------------------
> #!/usr/bin/python
>
> import libvirt
>
> def get_dom():
> conn = libvirt.open ("xen");
> dom = conn.lookupByName ("debian32fv");
> return dom
>
> dom = get_dom()
> print "name = %s\n" % dom.name()
> ------------------------------------------
>
> I would say that it's working as intended.
In this case yes, but if you add one further line at the end of
this example
conn2 = dom.get_connection()
And then have python garbage collect 'dom' and the original 'conn'
you're not safe because 'conn2' didn't increment the reference
count:
I finally managed to reproduce this. For reference, the reproduction
code is below:
--------------------
#!/usr/bin/python
import libvirt
import libvirtmod
def get_dom():
conn = libvirt.open ("xen");
dom = conn.lookupByName ("debian32fv");
return dom
dom = get_dom()
print "name = %s" % dom.name()
conn2 =
libvirt.virConnect (_obj = libvirtmod.virDomainGetConnect (dom._o))
dom2 = conn2.lookupByName ("debian32fv")
--------------------
This crashes at the last line because virConnectClose was called in the
destructor of conn.
It doesn't seem like changing virDomainGetConnect to play with reference
counts will fix this. The damage was done when virConnectClose was
called, which happened much earlier. The only solution is to keep the
relation that domain contains a connection explicit in the Python code.
Same probably applies to OCaml bindings too.
/*
* virDomainGetConnect:
* @dom: pointer to a domain
*
* Provides the connection pointer associated with a domain. The
* reference counter on the connection is not increased by this
* call.
*/
For this reason also I suggest that we remove virDomainGetConnect and
virNetworkGetConnect calls entirely, to prevent anyone from thinking
they can use them in a safe way. Patch is attached, and yes I know in
this case it breaks strict ABI compatibility.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
4:45 p.m.
On Tue, Jul 24, 2007 at 03:19:23PM +0100, Richard W.M. Jones wrote:
It doesn't seem like changing virDomainGetConnect to play with
reference
counts will fix this. The damage was done when virConnectClose was
called, which happened much earlier. The only solution is to keep the
relation that domain contains a connection explicit in the Python code.
Same probably applies to OCaml bindings too.
>/*
> * virDomainGetConnect:
> * @dom: pointer to a domain
> *
> * Provides the connection pointer associated with a domain. The
> * reference counter on the connection is not increased by this
> * call.
> */
For this reason also I suggest that we remove virDomainGetConnect and
virNetworkGetConnect calls entirely, to prevent anyone from thinking
they can use them in a safe way. Patch is attached, and yes I know in
this case it breaks strict ABI compatibility.
I still find them useful in the context of C coding as you can
get the connextion without having to carry it in all the calls (assuming
you understand the reference counting associated to connections, this
allow for simpler/cleaner code).
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5 p.m.
Here is a patch which adds a large warning to those functions, and
removes a comment from the Python code.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6:28 p.m.
On Tue, Jul 24, 2007 at 04:00:25PM +0100, Richard W.M. Jones wrote:
Here is a patch which adds a large warning to those functions, and
removes a comment from the Python code.
Okay, I removed the generator comment, and added a part of the warning
to the 2 function descriptions (only the first part though) for the
release. If you really think one need to warn C users more then maybe
we can get less threatening wording.
thanks, and sorry I rushed this a bit,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6264
days inactive
6265
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones