On 01/20/2014 06:09 PM, Peter Krempa wrote:
We shouldn't access the domain definition while we are in the monitor section as the domain is unlocked. Additionaly after we exit from the monitor we need to check if the VM is still alive. Not doing so resulted into crash if qemu exits while attempting to do a external VM snapshot. ---
Notes: Version 1 was reported to break locking of a VM but withoit enough data to reproduce/trace. Version 2: - changed some of the conditions so that proper cleanup paths are taken - fixed returned error value in case the job can't be entered
src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 12 deletions(-)
@@ -12605,8 +12606,25 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, + if (!actions) { + qemuDomainObjExitMonitor(driver, vm); + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Domain crashed while taking the snapshot")); + ret = -1; + } + } +
You use 'Domain' here...
@@ -12724,24 +12739,36 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, if (actions) { - if (ret == 0) - ret = qemuMonitorTransaction(priv->mon, actions); + if (ret == 0) { + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { + ret = qemuMonitorTransaction(priv->mon, actions); + qemuDomainObjExitMonitor(driver, vm); + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("domain crashed while taking the snapshot")); + ret = -1; + } + } else {
and 'domain' here. It would be better to be consistent, if only because translations are case-sensitive. I'd suggest going with lowercase, as it seems to be used in most of the snapshot errors. Jan