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 -=| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list