[libvirt] [PATCH 4/5] qemu: implement virConnectListAllDomains() for qemu driver

[Daniel P. Berrange]

On Thu, May 24, 2012 at 03:22:28PM +0200, Michal Privoznik wrote:
> On 24.05.2012 14:14, Daniel P. Berrange wrote:
> > On Thu, May 24, 2012 at 05:46:51AM -0600, Eric Blake wrote:
> >> On 05/20/2012 09:56 AM, Peter Krempa wrote:
> >>> This patch adds a basic implementation of the listing code for
> >>> virConnectListAllDomains() to qemu driver. The listing code does
> >>> not support any filtering flags yet, but they may be easily added
> >>> later.
> >>> ---
> >>>  src/qemu/qemu_driver.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 files changed, 97 insertions(+), 0 deletions(-)
> >>
> >>> +static void
> >>> +qemuPopulateDomainList(void *payload,
> >>> +                       const void *name ATTRIBUTE_UNUSED,
> >>> +                       void *opaque)
> >>> +{
> >>> +    struct virDomainListData *data = opaque;
> >>> +    virDomainObjPtr vm = payload;
> >>> +
> >>> +    if (data->error ||
> >>> +        (data->limit >= 0 &&
> >>> +         data->ndomains >= data->limit))
> >>> +        return;
> >>> +
> >>> +    if (!data->populate) {
> >>> +        data->ndomains++;
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    virDomainObjLock(vm);
> >>
> >> I just realized: Since we are executing this under the driver lock, and
> >> no VM can change state until we let go of the driver lock, it is not
> >> necessary to lock vms in this loop.  That will help things go faster in
> >> computing the list.
> > 
> > Hmm, this feels slightly dangerous to me. Saying that is in effect saying
> > you can do reads on a virDomainObjPtr without locking. This would  be ok
> > if it were impossible for the fields we're reading to be changed without
> > driver lock held.
> > 
> >  1. Thread A lock(driver)
> >  2. Thread A lock(vm1)
> >  3. Thread A unlock(driver)
> >  4. Thread B lock(driver)
> >  5. Thread B ...starts getting the list of domains...
> > 
> > Now consider Thread A changes the 'id' feld in a virDomainPtr eg due
> > to the guest shutting down.
> > 
> > Won't thread B be doing unsafe reads of 'id' now ?
> 
> If a domain dies during enumeration the 'id' won't be valid at the end
> anyway.

There's a significant difference between an ID value not being valid
anymore, which will be caught if it is used (assuming the ID values
have not wrapped), vs an ID value that is totally scrambled due to
non-atomic integer read/writes which could result in giving the ID
value of a different guest.

> Moreover, long running APIs (like shutdown) set job and unlock
> VM. Besides, the real id change is done in qemuProcessStop() rather than
> qemuDomainShutdownFlags(). Since domain can die without any mgmt
> application interaction, I don't think any app should rely on them. So
> I'd say this doesn't really matter.

I think that's a separate question really. We should be threadsafe
in our usage regardless. The benefits of avoiding the locking here
are not so great as to make it worthwhile

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