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

On Wed, Dec 03, 2014 at 03:02:52PM +0000, Daniel P. Berrange wrote: >On Wed, Dec 03, 2014 at 03:57:08PM +0100, Martin Kletzander wrote: >>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. > >I think there's a slight benefit to having the top level driver methods >do an explicit > > virObjectUnlock(vm); > virObjectUnref(vm); > >As opposed to > > qemuDomainEndAPI(vm); > >because, to me at least, the latter doesn't imply that the 'vm' object >is now potentially invalid, whereas seeing an ObjectUnref call makes >it obvious that 'vm' should no longer be used past that point. > What if qemuDomainEndAPI(vm) sets the vm to NULL to make sure it is not used? That would make it clear first time someone tries that :)