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

[Daniel P. Berrange]

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 ?

The only way I can see that this is safe, is if we are sure that
every change of the 'name', 'uuid' and 'id' fields is protected
by the driver lock.


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