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

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.