lastError.code is used as flag in qemu monitor. Unfortunately due to temporary OOM conditions (very unlikely through as thread local error is allocated on first use) we can fail to set this flag in case of monitor eofs/errors. This can cause hangs. Let's make sure flag is always set in case eofs/errors. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_monitor.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7b64752..e3fedd3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -765,6 +765,11 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error while processing monitor IO")); virCopyLastError(&mon->lastError); + + /* set error code if due to OOM conditions we fail to set it before */ + if (mon->lastError.code == VIR_ERR_OK) + mon->lastError.code = VIR_ERR_INTERNAL_ERROR; + virResetLastError(); } -- 1.8.3.1

Block job abort operation can not handle properly qemu crashes when waiting for abort/pivot completion. Deadlock scenario is next: - qemuDomainBlockJobAbort waits for pivot/abort completion - qemu crashes, then qemuProcessBeginStopJob broadcasts for VM condition and then waits for job condition (taken by qemuDomainBlockJobAbort) - qemuDomainBlockJobAbort awakes but nothing really changed, VM is still active (vm->def->id != -1) so thread starts waiting for completion again. Now two threads are in deadlock. First let's add some condition besides domain active status so that waiting thread can check it when awakes. Second let's move signalling to the place where condition is set, namely monitor eof/error handlers. Having signalling in qemuProcessBeginStopJob is not useful. The patch copies monitor error to domain state because at time waiting thread awakes there can be no monitor and it is useful to report monitor error to user. The patch has a drawback that on destroying a domain when waiting for event from monitor we get not very convinient error message like 'EOF from monitor' from waiting API. On the other hand if qemu crashes this is more useful then 'domain is not running'. The first case will be addressed in another patch. The patch also fixes other places where we wait for event from qemu. Namely handling device removal and tray ejection. Other places which used virDomainObjWait (dump, migration, save) were good because of async jobs which allow concurrent destroy job. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 43 ------------------------------------------- src/conf/domain_conf.h | 3 --- src/libvirt_private.syms | 2 -- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 ++++- src/qemu/qemu_driver.c | 6 +++--- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration.c | 12 ++++++------ src/qemu/qemu_process.c | 27 ++++++++++++++++++++++----- 9 files changed, 82 insertions(+), 65 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aacd06a..63df651 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3248,49 +3248,6 @@ virDomainObjBroadcast(virDomainObjPtr vm) } -int -virDomainObjWait(virDomainObjPtr vm) -{ - if (virCondWait(&vm->cond, &vm->parent.lock) < 0) { - virReportSystemError(errno, "%s", - _("failed to wait for domain condition")); - return -1; - } - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("domain is not running")); - return -1; - } - - return 0; -} - - -/** - * Waits for domain condition to be triggered for a specific period of time. - * - * Returns: - * -1 in case of error - * 0 on success - * 1 on timeout - */ -int -virDomainObjWaitUntil(virDomainObjPtr vm, - unsigned long long whenms) -{ - if (virCondWaitUntil(&vm->cond, &vm->parent.lock, whenms) < 0) { - if (errno != ETIMEDOUT) { - virReportSystemError(errno, "%s", - _("failed to wait for domain condition")); - return -1; - } - return 1; - } - return 0; -} - - /* * Mark the current VM config as transient. Ensures transient hotplug * operations do not persist past shutdown. diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 68473c3..b08cae7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2744,9 +2744,6 @@ bool virDomainObjTaint(virDomainObjPtr obj, virDomainTaintFlags taint); void virDomainObjBroadcast(virDomainObjPtr vm); -int virDomainObjWait(virDomainObjPtr vm); -int virDomainObjWaitUntil(virDomainObjPtr vm, - unsigned long long whenms); void virDomainPanicDefFree(virDomainPanicDefPtr panic); void virDomainResourceDefFree(virDomainResourceDefPtr resource); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c4f70f1..30f8393 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -491,8 +491,6 @@ virDomainObjSetMetadata; virDomainObjSetState; virDomainObjTaint; virDomainObjUpdateModificationImpact; -virDomainObjWait; -virDomainObjWaitUntil; virDomainOSTypeFromString; virDomainOSTypeToString; virDomainParseMemory; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f03aa03..4b4052b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1930,6 +1930,7 @@ qemuDomainObjPrivateFree(void *data) qemuDomainObjFreeJob(priv); VIR_FREE(priv->lockState); VIR_FREE(priv->origname); + virResetError(&priv->monError); virChrdevFree(priv->devs); @@ -11960,3 +11961,47 @@ qemuProcessEventFree(struct qemuProcessEvent *event) } VIR_FREE(event); } + + +/** + * Waits for domain condition to be triggered for a specific period of time. + * if @until is 0 then waits indefinetely. + * + * Returns: + * -1 on error + * 0 on success + * 1 on timeout + */ +int +qemuDomainObjWait(virDomainObjPtr vm, unsigned long long until) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; + + if (until) + rc = virCondWaitUntil(&vm->cond, &vm->parent.lock, until); + else + rc = virCondWait(&vm->cond, &vm->parent.lock); + + if (rc < 0) { + if (until && errno == ETIMEDOUT) + return 1; + + virReportSystemError(errno, "%s", + _("failed to wait for domain condition")); + return -1; + } + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("domain is not running")); + return -1; + } + + if (priv->monError.code != VIR_ERR_OK) { + virSetError(&priv->monError); + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 21e12f6..9cbb5e4 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -258,7 +258,7 @@ struct _qemuDomainObjPrivate { qemuMonitorPtr mon; virDomainChrSourceDefPtr monConfig; bool monJSON; - bool monError; + virError monError; unsigned long long monStart; qemuAgentPtr agent; @@ -1003,4 +1003,7 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg); +int +qemuDomainObjWait(virDomainObjPtr vm, unsigned long long until); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d97d2f4..1cf33bc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2747,7 +2747,7 @@ qemuDomainGetControlInfo(virDomainPtr dom, memset(info, 0, sizeof(*info)); - if (priv->monError) { + if (priv->monError.code != VIR_ERR_OK) { info->state = VIR_DOMAIN_CONTROL_ERROR; info->details = VIR_DOMAIN_CONTROL_ERROR_REASON_MONITOR; } else if (priv->job.active) { @@ -3746,7 +3746,7 @@ qemuDumpWaitForCompletion(virDomainObjPtr vm) VIR_DEBUG("Waiting for dump completion"); while (!priv->job.dumpCompleted && !priv->job.abortJob) { - if (virDomainObjWait(vm) < 0) + if (qemuDomainObjWait(vm, 0) < 0) return -1; } @@ -16915,7 +16915,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk, NULL); while (diskPriv->blockjob) { - if (virDomainObjWait(vm) < 0) { + if (qemuDomainObjWait(vm, 0) < 0) { ret = -1; goto endjob; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f0d549d..ec89f71 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -206,7 +206,7 @@ qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver, return -1; while (disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) { - if ((rc = virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT)) < 0) + if ((rc = qemuDomainObjWait(vm, now + CHANGE_MEDIA_TIMEOUT)) < 0) return -1; if (rc > 0) { @@ -4675,7 +4675,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) until += qemuDomainRemoveDeviceWaitTime; while (priv->unplug.alias) { - if ((rc = virDomainObjWaitUntil(vm, until)) == 1) + if ((rc = qemuDomainObjWait(vm, until)) == 1) return 0; if (rc < 0) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e523155..5807ca6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -893,7 +893,7 @@ qemuMigrationSrcCancelDriveMirror(virQEMUDriverPtr driver, if (failed && !err) err = virSaveLastError(); - if (virDomainObjWait(vm) < 0) + if (qemuDomainObjWait(vm, 0) < 0) goto cleanup; } @@ -1032,7 +1032,7 @@ qemuMigrationSrcDriveMirror(virQEMUDriverPtr driver, goto cleanup; } - if (virDomainObjWait(vm) < 0) + if (qemuDomainObjWait(vm, 0) < 0) goto cleanup; } @@ -1396,7 +1396,7 @@ qemuMigrationSrcWaitForSpice(virDomainObjPtr vm) VIR_DEBUG("Waiting for SPICE to finish migration"); while (!priv->job.spiceMigrated && !priv->job.abortJob) { - if (virDomainObjWait(vm) < 0) + if (qemuDomainObjWait(vm, 0) < 0) return -1; } return 0; @@ -1675,7 +1675,7 @@ qemuMigrationSrcWaitForCompletion(virQEMUDriverPtr driver, return rv; if (events) { - if (virDomainObjWait(vm) < 0) { + if (qemuDomainObjWait(vm, 0) < 0) { jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED; return -2; } @@ -1728,7 +1728,7 @@ qemuMigrationDstWaitForCompletion(virQEMUDriverPtr driver, while ((rv = qemuMigrationAnyCompleted(driver, vm, asyncJob, NULL, flags)) != 1) { - if (rv < 0 || virDomainObjWait(vm) < 0) + if (rv < 0 || qemuDomainObjWait(vm, 0) < 0) return -1; } @@ -3982,7 +3982,7 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, if (priv->monJSON) { while (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { priv->signalStop = true; - rc = virDomainObjWait(vm); + rc = qemuDomainObjWait(vm, 0); priv->signalStop = false; if (rc < 0) goto error; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c0105c8..72e2977 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -267,6 +267,23 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) return 0; } +static void +qemuProcessNotifyMonitorError(virDomainObjPtr vm, + qemuMonitorPtr mon) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr err = qemuMonitorLastError(mon); + + virCopyError(err, &priv->monError); + + /* set error code if due to OOM conditions we fail to set it before */ + if (priv->monError.code == VIR_ERR_OK) + priv->monError.code = VIR_ERR_INTERNAL_ERROR; + + /* Wake up anything waiting for events from monitor */ + virDomainObjBroadcast(vm); + virFreeError(err); +} /* * This is a callback registered with a qemuMonitorPtr instance, @@ -285,6 +302,8 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon, virObjectLock(vm); + qemuProcessNotifyMonitorError(vm, mon); + VIR_DEBUG("Received EOF on %p '%s'", vm, vm->def->name); priv = vm->privateData; @@ -337,7 +356,8 @@ qemuProcessHandleMonitorError(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virObjectLock(vm); - ((qemuDomainObjPrivatePtr) vm->privateData)->monError = true; + qemuProcessNotifyMonitorError(vm, mon); + event = virDomainEventControlErrorNewFromObj(vm); qemuDomainEventQueue(driver, event); @@ -5725,7 +5745,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, goto cleanup; priv->monJSON = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON); - priv->monError = false; + virResetError(&priv->monError); priv->monStart = 0; priv->gotShutdown = false; @@ -6481,9 +6501,6 @@ qemuProcessBeginStopJob(virQEMUDriverPtr driver, if (qemuProcessKill(vm, killFlags) < 0) goto cleanup; - /* Wake up anything waiting on domain condition */ - virDomainObjBroadcast(vm); - if (qemuDomainObjBeginJob(driver, vm, job) < 0) goto cleanup; -- 1.8.3.1