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

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