Re: [libvirt] [PATCH 2/2] qemu: completely rework reference counting

On Wed, Dec 03, 2014 at 02:54:11PM +0100, Martin Kletzander wrote: > On Wed, Dec 03, 2014 at 10:48:49AM +0000, Daniel P. Berrange wrote: > >On Wed, Dec 03, 2014 at 06:49:04AM +0100, Martin Kletzander wrote: > > > >>@@ -109,7 +117,6 @@ To lock the virDomainObjPtr > >> To acquire the normal job condition > >> > >> qemuDomainObjBeginJob() > >>- - Increments ref count on virDomainObjPtr > >> - Waits until the job is compatible with current async job or no > >> async job is running > >> - Waits for job.cond condition 'job.active != 0' using virDomainObjPtr > >>@@ -122,14 +129,12 @@ To acquire the normal job condition > >> qemuDomainObjEndJob() > >> - Sets job.active to 0 > >> - Signals on job.cond condition > >>- - Decrements ref count on virDomainObjPtr > > > >I think this is really the key improvement here. Currently we have > >this error prone code where we have to check for NULLs after leaving > >the job > > > > if (qemuDomainObjEndJob() < 0) > > vm = NULL; > > > > if (vm) > > virObjectUnlock(vm) > > > >With the methods now fully owning a reference, the 'vm' object can > >never be disposed off by another thread. So it will let us make the > >code simpler and less error prone. > > > >>+void > >>+qemuDomObjEndAPI(virDomainObjPtr vm) > >>+{ > >>+ if (virObjectUnref(vm)) > >>+ virObjectUnlock(vm); > >>+} > > > >I've not thought about it too deeply, so could be missing something, > >but I wonder if it is not sufficient todo > > > > virObjectUnlock(vm); > > virObjectUnref(vm); > > > >As there is no requirement for the object to be locked now we're > >using atomic ints for reference counting. This would avoid the > >need go hide the unlock/unref behind this qemuDomObjEndAPI method. > >Just have this inline so it is obvious to the casual reader that > >we're unrefing + unlocking the object > > > > I had an idea of having some global PRE-call method that would be > called for every API function and it would do the dance that all APIs > do (checking flags, checking ACLs, getting all the objects, eventually > creating a job) and there would be one at the end, doing all the > reverse stuff. That's, of course, way more difficult or maybe > impossible or too complex (I haven't explored the idea any more), but > that's where the qemuDomObjEndAPI() came from. It also makes it easy > to add any code for each "API". But of course it can be changed with > unlock'n'unref as you said. And the reason for calling conditionally > only when unref'ing any other reference than the last one, was that it > is more usable with NULL pointers and also in APIs where it gets > removed from the list. The APIs where we remove the dom from the object list should no longer be a special case after this change, right ? The list owns a reference and the executing API call owns a reference, instead of borrowing the list's reference. So we can always assume that 'dom' is non-NULL for these APIs now, even after being removed from the list.

Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|