With current flow in qemudDomainDefine we might loose data when updating an existing domain. We parse given XML and overwrite the configuration. Then we try to save the new config. However, this step may fail and we don't perform any roll back. In fact, we remove the domain from the list of domains held up by qemu driver. This is okay as long as the domain was brand new one. --- The easiest steps to reproduce: https://bugzilla.redhat.com/show_bug.cgi?id=851963 src/qemu/qemu_driver.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3fcca0e..e760ed9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5538,6 +5538,7 @@ out: static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { struct qemud_driver *driver = conn->privateData; virDomainDefPtr def; + virDomainDefPtr def_backup = NULL; virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; virDomainEventPtr event = NULL; @@ -5561,20 +5562,48 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { if (qemuDomainAssignAddresses(def, NULL, NULL) < 0) goto cleanup; - if (!(vm = virDomainAssignDef(driver->caps, - &driver->domains, - def, false))) { - goto cleanup; + /* We need to differentiate two cases: + * a) updating an existing domain - must preserve previous definition + * so we can roll back if something fails + * b) defining a brand new domain - virDomainAssignDef is just sufficient + */ + if ((vm = virDomainFindByUUID(&driver->domains, def->uuid))) { + if (virDomainObjIsActive(vm)) { + def_backup = vm->newDef; + vm->newDef = def; + } else { + def_backup = vm->def; + vm->def = def; + } + } else { + if (!(vm = virDomainAssignDef(driver->caps, + &driver->domains, + def, false))) { + goto cleanup; + } } def = NULL; vm->persistent = 1; if (virDomainSaveConfig(driver->configDir, vm->newDef ? vm->newDef : vm->def) < 0) { - VIR_INFO("Defining domain '%s'", vm->def->name); - qemuDomainRemoveInactive(driver, vm); - vm = NULL; + if (def_backup) { + /* There is backup so this VM was defined before. + * Just restore the backup. */ + VIR_INFO("Restoring domain '%s' definition", vm->def->name); + if (virDomainObjIsActive(vm)) + vm->newDef = def_backup; + else + vm->def = def_backup; + } else { + /* Brand new domain. Remove it */ + VIR_INFO("Deleting domain '%s'", vm->def->name); + qemuDomainRemoveInactive(driver, vm); + vm = NULL; + } goto cleanup; + } else { + virDomainDefFree(def_backup); } event = virDomainEventNewFromObj(vm, -- 1.7.8.6