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

[Martin Kletzander]

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.

>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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141203/479667b6/attachment-0001.sig>