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

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

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