On Wed, Oct 27, 2010 at 02:28:51PM -0600, Eric Blake wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=638285 - when migrating > a guest, it was very easy to provoke a race where an application > could query block information on a VM that had just been migrated > away. Any time qemu code obtains a job lock, it must also check > that the VM was not taken down in the time where it was waiting > for the lock. > > * src/qemu/qemu_driver.c (qemudDomainSetMemory) > (qemudDomainGetInfo, qemuDomainGetBlockInfo): Check that vm still > exists after obtaining job lock, before starting monitor action. > --- > > v2: incorporate danpb's suggestions, to minimize virDomainObjIsActive > calls and avoid use of goto for mid-function labels. > > src/qemu/qemu_driver.c | 60 ++++++++++++++++++++++++----------------------- > 1 files changed, 31 insertions(+), 29 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index b119ca1..f1f8cdf 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -466,7 +466,8 @@ static int ATTRIBUTE_RETURN_CHECK qemuDomainObjEndJob(virDomainObjPtr obj) > * obj must be locked before calling, qemud_driver must be unlocked > * > * To be called immediately before any QEMU monitor API call > - * Must have alrady called qemuDomainObjBeginJob(). > + * Must have already called qemuDomainObjBeginJob(), and checked > + * that the VM is still active. > * > * To be followed with qemuDomainObjExitMonitor() once complete > */ > @@ -507,7 +508,7 @@ static void qemuDomainObjExitMonitor(virDomainObjPtr obj) > * obj must be locked before calling, qemud_driver must be locked > * > * To be called immediately before any QEMU monitor API call > - * Must have alrady called qemuDomainObjBeginJob(). > + * Must have already called qemuDomainObjBeginJob(). > * > * To be followed with qemuDomainObjExitMonitorWithDriver() once complete > */ > @@ -525,7 +526,7 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir > /* obj must NOT be locked before calling, qemud_driver must be unlocked, > * and will be locked after returning > * > - * Should be paired with an earlier qemuDomainObjEnterMonitor() call > + * Should be paired with an earlier qemuDomainObjEnterMonitorWithDriver() call > */ > static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) > { > @@ -5077,12 +5078,6 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) { > goto cleanup; > } > > - if (!virDomainObjIsActive(vm)) { > - qemuReportError(VIR_ERR_OPERATION_INVALID, > - "%s", _("domain is not running")); > - goto cleanup; > - } > - > if (newmem > vm->def->mem.max_balloon) { > qemuReportError(VIR_ERR_INVALID_ARG, > "%s", _("cannot set memory higher than max memory")); > @@ -5092,6 +5087,12 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) { > if (qemuDomainObjBeginJob(vm) < 0) > goto cleanup; > > + if (!virDomainObjIsActive(vm)) { > + qemuReportError(VIR_ERR_OPERATION_INVALID, > + "%s", _("domain is not running")); > + goto endjob; > + } > + > priv = vm->privateData; > qemuDomainObjEnterMonitor(vm); > r = qemuMonitorSetBalloon(priv->mon, newmem); > @@ -5158,26 +5159,25 @@ static int qemudDomainGetInfo(virDomainPtr dom, > } else if (!priv->jobActive) { > if (qemuDomainObjBeginJob(vm) < 0) > goto cleanup; > - > - qemuDomainObjEnterMonitor(vm); > - err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); > - qemuDomainObjExitMonitor(vm); > - if (err < 0) { > - if (qemuDomainObjEndJob(vm) == 0) > - vm = NULL; > + if (!virDomainObjIsActive(vm)) > + err = 0; > + else { > + qemuDomainObjEnterMonitor(vm); > + err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); > + qemuDomainObjExitMonitor(vm); > + } > + if (qemuDomainObjEndJob(vm) == 0) { > + vm = NULL; > goto cleanup; > } > > + if (err < 0) > + goto cleanup; > if (err == 0) > /* Balloon not supported, so maxmem is always the allocation */ > info->memory = vm->def->mem.max_balloon; > else > info->memory = balloon; > - > - if (qemuDomainObjEndJob(vm) == 0) { > - vm = NULL; > - goto cleanup; > - } > } else { > info->memory = vm->def->mem.cur_balloon; > } > @@ -10510,19 +10510,21 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, > /* ..but if guest is running & not using raw > disk format and on a block device, then query > highest allocated extent from QEMU */ > - if (virDomainObjIsActive(vm) && > - disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && > + if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && > format != VIR_STORAGE_FILE_RAW && > S_ISBLK(sb.st_mode)) { > qemuDomainObjPrivatePtr priv = vm->privateData; > if (qemuDomainObjBeginJob(vm) < 0) > goto cleanup; > - > - qemuDomainObjEnterMonitor(vm); > - ret = qemuMonitorGetBlockExtent(priv->mon, > - disk->info.alias, > - &info->allocation); > - qemuDomainObjExitMonitor(vm); > + if (!virDomainObjIsActive(vm)) > + ret = 0; > + else { > + qemuDomainObjEnterMonitor(vm); > + ret = qemuMonitorGetBlockExtent(priv->mon, > + disk->info.alias, > + &info->allocation); > + qemuDomainObjExitMonitor(vm); > + } > > if (qemuDomainObjEndJob(vm) == 0) > vm = NULL; ACK, it's true it makes for a nicer patch than v1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list