[Libvir] RFC: PATCH 1/5: cleanup connection ref counting

[Daniel P. Berrange]

On Mon, Jan 14, 2008 at 10:35:10AM +0000, Richard W.M. Jones wrote:
> 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.
> 
> The reference counting in libvirt is not just ugly when interfacing with 
> a language with real GC, but also broken at the moment.
> 
> The most serious example are the connect/domain/network handles included 
> in a virterror.  These are not reference counted so that the caller 
> doesn't have to free the virterror or these handles.  But on the other 
> hand it means that the handles have an indeterminate lifetime, so cannot 
> be used safely.

Ahhh you noticed that too then :-)  That's on my list of things to fix
somehow I've thought of a couple of options:

 - Increment ref count when assign a domain/network to a virError

Or

 - When ref count of domain/network drops to zero check for any virError 
   a NULL-ify the domain/network

Or

 - Or tell people not to use the domain/network objects in errors and
   don't set them at all

With the first option we might consider a special case in virConnectClose
to automatically decrement the ref count held by the error object, otherwie
we'd be in situation where a virConnectPtr would potentially never be
free'd unless user calls virErrorReset which they probably don't.

My preference is the last. IMHO its a flawed idea because its not extensible
to other objects we're adding. eg I've no way to add a Job or StoragePool
or StorageVol object to the virErrorPtr without breaking ABI each time.
IMHO we should just make use of 'str1' and 'str2' and 'int1' to store the
name, uuid and id of the associated objects, since we have all these  spare
generic fields never used ..

> >So I've moved the code for garbage collecting a virConnectPtr
> >object into a new virUnrefConnect() method which can be called
> >from virFreeConnect, virFreeDomain and virFreeNetwork.
> 
> This probably has negative implications in the language bindings.  At 
> this moment I don't care much because network objects are in practice 
> used only very rarely by real code.  This could change when we have 
> storage objects which, I guess, will be used frequently like domains.  A 
> bit too early in the morning for me to be thinking about GC and its 
> interaction with reference counting :-)

Actually it shouldn't hurt the language bindings. When I say I moved
the gargage collection code, this is merely a re-factoring of the 
internal cleanup code. So instead of having the same broken duplicted
in virFreeConnect, virFreeDomain and virFreeNetwork it is centralized
in virUnrefConnect. The public API is unchanged, just the impl.

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  -=|