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 :|
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list