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

On Wed, Dec 03, 2014 at 02:01:10PM +0000, Daniel P. Berrange wrote: >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. > Yes, you're right, I explained myself improperly, sorry. I saw the functions, thought they are in a different order and didn't think about what really was the reason behind that. Yes, unlock && unref makes sense as well, but we'd lose the function common to all those calls.