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

Martin Kletzander mkletzan at redhat.com
Wed Dec 3 13:54:11 UTC 2014


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.

Martin

>
>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/636215a8/attachment-0001.sig>


More information about the libvir-list mailing list