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

[Martin Kletzander]

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

>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 :|
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/d982830b/attachment-0001.sig>