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

[Daniel P. Berrange]

On Wed, Dec 03, 2014 at 04:28:46PM +0100, Martin Kletzander wrote:
> 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 :)

Yep, that would work - pass in a  'virDomainObjPtr *' instead i guess

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 :|