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/