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.
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list