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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list