[Libvir] PATCH: Fix and cleanup ref counting/ domain/network object release

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/