[Libvir] PATCH: Fix and cleanup ref counting/ domain/network object release
The referencing counting code for Connect/Domain/Network objects has many repeated codepaths, not all of which are correct. eg, the virFreeDomain method forgets to release networks when garbage collecting a virConnectPtr, and the virFreeNetwork method forgets to release domains. I've also found it very confusing having both virFreeDomain and virDomainFree. So I've changed the impl of the cleanup functions in hash.c to use a slightly different structure. There are now 6 functions: - virUnrefConnect - virReleaseConnect - virUnrefDomain - virReleaseDomain - virUnrefNetwork The 'virUnref*' APIs are intended for use by the drivers to decrement the ref count on objects they no loner need. In the virUnref* method, if the ref count hits zero, then it calls the corresponding virRelease* method to actually free the object's memory. The virUnref* methods are responsible for acquiring the connection mutex. The virRelease* method mandate that5D the mutex is already held, and will releae it as the very last step when deleting the object. The virReleaseDomain/virReleaeNetwork methods will also derecement the ref count of the connection, and call virReleaseConnection if appropriate. The patch for all this looks horrific but its not as bad as it looks, so I'd recommend reviewing the final code in hash.c, rather than the patch. I have also switched from xmlMutex to the POSIX standard pthread_mutex_t object. In theory the former may give more portability, but the patches which follow are also going to require actual pthread_t objects, for which libxml has no wrappers. Thus it is pointless using the xmlMutex abstraction. The added plus, is that pthread_mutex_t objects do not require any memory allocation, merely initialization of their pre-allocated contents so it simplifies a little code. Finally, the code in hash.c for dealing with ref counting seriously abused the 'goto' statement with multiple jumps overlapping each other in a single method. This made it very hard to follow. So I removed the use of goto in most places so its only used in error cleanup paths, and not for regular flow control. Oh, and in cleaning up internal.h I found an odd declaration of some functions from xs_internal.h which could have been made static. So I made them static. In this version I removed the call to 'virUnrefDomain' from the libvirt.c file's virDomainDestroy call, and likewise in virNetworkDestroy. 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 -=|
On Sat, Jan 19, 2008 at 07:18:03PM +0000, Daniel P. Berrange wrote:
The referencing counting code for Connect/Domain/Network objects has many repeated codepaths, not all of which are correct. eg, the virFreeDomain method forgets to release networks when garbage collecting a virConnectPtr, and the virFreeNetwork method forgets to release domains. I've also found it very confusing having both virFreeDomain and virDomainFree.
So I've changed the impl of the cleanup functions in hash.c to use a slightly different structure. There are now 6 functions:
- virUnrefConnect - virReleaseConnect - virUnrefDomain - virReleaseDomain - virUnrefNetwork
The 'virUnref*' APIs are intended for use by the drivers to decrement the ref count on objects they no loner need. In the virUnref* method, if the ref count hits zero, then it calls the corresponding virRelease* method to actually free the object's memory. The virUnref* methods are responsible for acquiring the connection mutex. The virRelease* method mandate that5D the mutex is already held, and will releae it as the very last step when deleting the object. The virReleaseDomain/virReleaeNetwork methods will also derecement the ref count of the connection, and call virReleaseConnection if appropriate. The patch for all this looks horrific but its not as bad as it looks, so I'd recommend reviewing the final code in hash.c, rather than the patch.
Sounds okay,
I have also switched from xmlMutex to the POSIX standard pthread_mutex_t object. In theory the former may give more portability, but the patches which follow are also going to require actual pthread_t objects, for which libxml has no wrappers. Thus it is pointless using the xmlMutex abstraction. The added plus, is that pthread_mutex_t objects do not require any memory allocation, merely initialization of their pre-allocated contents so it simplifies a little code.
That I found worrying. What is the added benefit for the loss of portability? You now reference 2 relatively system specific kind of data structure, where we used to reference only the xmlMutex one which was portable. And i don't see why, you need to do this for the goal stated before.
Finally, the code in hash.c for dealing with ref counting seriously abused the 'goto' statement with multiple jumps overlapping each other in a single method. This made it very hard to follow. So I removed the use of goto in most places so its only used in error cleanup paths, and not for regular flow control.
Sounds right
Oh, and in cleaning up internal.h I found an odd declaration of some functions from xs_internal.h which could have been made static. So I made them static.
Sounds good too, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Jan 21, 2008 at 03:54:49AM -0500, Daniel Veillard wrote:
On Sat, Jan 19, 2008 at 07:18:03PM +0000, Daniel P. Berrange wrote:
The referencing counting code for Connect/Domain/Network objects has many repeated codepaths, not all of which are correct. eg, the virFreeDomain method forgets to release networks when garbage collecting a virConnectPtr, and the virFreeNetwork method forgets to release domains. I've also found it very confusing having both virFreeDomain and virDomainFree.
So I've changed the impl of the cleanup functions in hash.c to use a slightly different structure. There are now 6 functions:
- virUnrefConnect - virReleaseConnect - virUnrefDomain - virReleaseDomain - virUnrefNetwork
The 'virUnref*' APIs are intended for use by the drivers to decrement the ref count on objects they no loner need. In the virUnref* method, if the ref count hits zero, then it calls the corresponding virRelease* method to actually free the object's memory. The virUnref* methods are responsible for acquiring the connection mutex. The virRelease* method mandate that5D the mutex is already held, and will releae it as the very last step when deleting the object. The virReleaseDomain/virReleaeNetwork methods will also derecement the ref count of the connection, and call virReleaseConnection if appropriate. The patch for all this looks horrific but its not as bad as it looks, so I'd recommend reviewing the final code in hash.c, rather than the patch.
Sounds okay,
I have also switched from xmlMutex to the POSIX standard pthread_mutex_t object. In theory the former may give more portability, but the patches which follow are also going to require actual pthread_t objects, for which libxml has no wrappers. Thus it is pointless using the xmlMutex abstraction. The added plus, is that pthread_mutex_t objects do not require any memory allocation, merely initialization of their pre-allocated contents so it simplifies a little code.
That I found worrying. What is the added benefit for the loss of portability? You now reference 2 relatively system specific kind of data structure, where we used to reference only the xmlMutex one which was portable. And i don't see why, you need to do this for the goal stated before.
The problem is that we need to use pthread_t / pthread_create APIs for other parts of libvirt. libxml2 doesn't provide any portability layer for threads - only for mutexes. Any platform which has pthread_t, already has pthread_mutex_t, so although in theory the pthread_mutex_t is less portable than xmlMutex, in practice the overall portability is the same due to the constraint of needing pthread_t. 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 -=|
Daniel P. Berrange wrote:
The problem is that we need to use pthread_t / pthread_create APIs for other parts of libvirt. libxml2 doesn't provide any portability layer for threads - only for mutexes. Any platform which has pthread_t, already has pthread_mutex_t, so although in theory the pthread_mutex_t is less portable than xmlMutex, in practice the overall portability is the same due to the constraint of needing pthread_t.
Don't be forgetting about Windows. MinGW has a pthread API, and Red Hat also have one[1]. Whether they are the same or not I haven't found out yet. The one thing I noticed is you need to be careful about pthread_t : on Unix this is usually an int of some sort, but on Windows it's a structure. You might also want to check [2] for some gotchas, particularly around thread cancellation. Rich. [1] http://sources.redhat.com/pthreads-win32/ [2] http://sources.redhat.com/pthreads-win32/faq.html -- 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
On Mon, Jan 21, 2008 at 02:57:37PM +0000, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
The problem is that we need to use pthread_t / pthread_create APIs for other parts of libvirt. libxml2 doesn't provide any portability layer for threads - only for mutexes. Any platform which has pthread_t, already has pthread_mutex_t, so although in theory the pthread_mutex_t is less portable than xmlMutex, in practice the overall portability is the same due to the constraint of needing pthread_t.
Don't be forgetting about Windows. MinGW has a pthread API, and Red Hat also have one[1]. Whether they are the same or not I haven't found out yet.
Luckily the thread stuff should not be used in the remote driver - only the xen/qemu/storage drivers, so we ought not to have to worry about that (until we port the real drivers to windows :-)
The one thing I noticed is you need to be careful about pthread_t : on Unix this is usually an int of some sort, but on Windows it's a structure. You might also want to check [2] for some gotchas, particularly around thread cancellation.
Yeah, thread cancellation is a horibble thing that I intend to stay away from. 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 -=|
On Mon, Jan 21, 2008 at 02:48:58PM +0000, Daniel P. Berrange wrote:
That I found worrying. What is the added benefit for the loss of portability? You now reference 2 relatively system specific kind of data structure, where we used to reference only the xmlMutex one which was portable. And i don't see why, you need to do this for the goal stated before.
The problem is that we need to use pthread_t / pthread_create APIs for other parts of libvirt. libxml2 doesn't provide any portability layer for threads -
Oh there is a good reason for this :-)
only for mutexes. Any platform which has pthread_t, already has pthread_mutex_t, so although in theory the pthread_mutex_t is less portable than xmlMutex, in practice the overall portability is the same due to the constraint of needing pthread_t.
<grin/> ... I see, Okay +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones