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

[Daniel P. Berrange]

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.

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