[libvirt] [PATCH v2 0/4] Couple of 'virsh create' fixes
And this time 'virsh restore' too. The diff to v1 is that more drivers is fixed. Michal Privoznik (4): virDomainCreateXML: Don't remove persistent domains on error virDomainCreateXML: Make domain definition transient qemu: Move vm->persistent check into qemuDomainRemoveInactive virDomainRestore: Don't keep transient domains around src/bhyve/bhyve_driver.c | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 7 +++++-- src/openvz/openvz_driver.c | 1 + src/qemu/qemu_domain.c | 9 ++++++++- src/qemu/qemu_driver.c | 39 ++++++++++++++++----------------------- src/qemu/qemu_migration.c | 14 +++++--------- src/qemu/qemu_process.c | 12 ++++-------- src/test/test_driver.c | 15 +++++++++++++-- src/uml/uml_driver.c | 8 +++++--- src/vmware/vmware_driver.c | 7 +++++-- 11 files changed, 64 insertions(+), 50 deletions(-) -- 2.4.9
https://bugzilla.redhat.com/show_bug.cgi?id=871452 Okay, so we allow users to 'virsh create' an already existing domain, providing completely different XML than the one stored in Libvirt. Well, as long as name and UUID matches. However, in some drivers the code that handles errors unconditionally removes the domain that failed to start even though the domain might have been persistent. Fortunately, the domain is removed just from the internal list of domains and the config file is kept around. Steps to reproduce: 1) virsh dumpxml $dom > /tmp/dom.xml 2) change XML so that it is still parse-able but won't boot, e.g. change guest agent path to /foo/bar 3) virsh create /tmp/dom.xml 4) virsh dumpxml $dom 5) Observe "No such domain" error Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 6 ++++-- src/qemu/qemu_driver.c | 6 ++++-- src/test/test_driver.c | 7 ++++++- src/uml/uml_driver.c | 7 ++++--- src/vmware/vmware_driver.c | 6 ++++-- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a9f0005..e5e6c5a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1239,8 +1239,10 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, (flags & VIR_DOMAIN_START_AUTODESTROY), VIR_DOMAIN_RUNNING_BOOTED) < 0) { virDomainAuditStart(vm, "booted", false); - virDomainObjListRemove(driver->domains, vm); - vm = NULL; + if (!vm->persistent) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2387cf3..30d2d98 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1752,7 +1752,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, def = NULL; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { - qemuDomainRemoveInactive(driver, vm); + if (!vm->persistent) + qemuDomainRemoveInactive(driver, vm); goto cleanup; } @@ -1762,7 +1763,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); qemuDomainObjEndJob(driver, vm); - qemuDomainRemoveInactive(driver, vm); + if (!vm->persistent) + qemuDomainRemoveInactive(driver, vm); goto cleanup; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d11cda1..b40b079 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1621,8 +1621,13 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, goto cleanup; def = NULL; - if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) + if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) { + if (!dom->persistent) { + virDomainObjListRemove(privconn->domains, dom); + dom = NULL; + } goto cleanup; + } event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STARTED, diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 2b61f73..d4b03b3 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1623,9 +1623,10 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, if (umlStartVMDaemon(conn, driver, vm, (flags & VIR_DOMAIN_START_AUTODESTROY)) < 0) { virDomainAuditStart(vm, "booted", false); - virDomainObjListRemove(driver->domains, - vm); - vm = NULL; + if (!vm->persistent) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } goto cleanup; } virDomainAuditStart(vm, "booted", true); diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index e228aaa..152af39 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -716,8 +716,10 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, vmdef = NULL; if (vmwareStartVM(driver, vm) < 0) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; + if (!vm->persistent) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } goto cleanup; } -- 2.4.9
https://bugzilla.redhat.com/show_bug.cgi?id=871452 So, you want to create a domain from XML. The domain already exists in libvirt's database of domains. It's okay, because name and UUID matches. However, on domain startup, internal representation of the domain is overwritten with your XML even though we claim that the XML you've provided is a transient one. The bug is to be found across nearly all the drivers. Le sigh. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_driver.c | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vmware/vmware_driver.c | 1 + 8 files changed, 8 insertions(+) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 7f365b1..d44cf2c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -918,6 +918,7 @@ bhyveDomainCreateXML(virConnectPtr conn, if (!(vm = virDomainObjListAdd(privconn->domains, def, privconn->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; def = NULL; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5048957..fc6dcec 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -974,6 +974,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e5e6c5a..b408be0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1229,6 +1229,7 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index fc8db7e..d78e2f5 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1093,6 +1093,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, if (!(vm = virDomainObjListAdd(driver->domains, vmdef, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 30d2d98..2a4b026 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1745,6 +1745,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b40b079..01ab1e3 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1616,6 +1616,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, if (!(dom = virDomainObjListAdd(privconn->domains, def, privconn->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index d4b03b3..14598fc 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1615,6 +1615,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 152af39..a12b03a 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -704,6 +704,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, if (!(vm = virDomainObjListAdd(driver->domains, vmdef, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; -- 2.4.9
On Wed, Sep 23, 2015 at 03:00:35PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871452
So, you want to create a domain from XML. The domain already exists in libvirt's database of domains. It's okay, because name and UUID matches. However, on domain startup, internal representation of the domain is overwritten with your XML even though we claim that the XML you've provided is a transient one. The bug is to be found across nearly all the drivers. Le sigh.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_driver.c | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vmware/vmware_driver.c | 1 + 8 files changed, 8 insertions(+)
It's sad that VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE now exists just because of vmware, otherwise VIR_DOMAIN_OBJ_LIST_ADD_LIVE could do just that. Anyway, thanks for fixing it.
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 7f365b1..d44cf2c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -918,6 +918,7 @@ bhyveDomainCreateXML(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(privconn->domains, def, privconn->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; def = NULL; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5048957..fc6dcec 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -974,6 +974,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e5e6c5a..b408be0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1229,6 +1229,7 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index fc8db7e..d78e2f5 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1093,6 +1093,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, if (!(vm = virDomainObjListAdd(driver->domains, vmdef, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 30d2d98..2a4b026 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1745,6 +1745,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b40b079..01ab1e3 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1616,6 +1616,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, if (!(dom = virDomainObjListAdd(privconn->domains, def, privconn->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index d4b03b3..14598fc 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1615,6 +1615,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml,
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 152af39..a12b03a 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -704,6 +704,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, if (!(vm = virDomainObjListAdd(driver->domains, vmdef, driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; -- 2.4.9
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
So far we have the following pattern occurring over and over again: if (!vm->persistent) qemuDomainRemoveInactive(driver, vm); It's safe to put the check into the function and save some LoC. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 9 ++++++++- src/qemu/qemu_driver.c | 42 ++++++++++++++++-------------------------- src/qemu/qemu_migration.c | 14 +++++--------- src/qemu/qemu_process.c | 12 ++++-------- 4 files changed, 33 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7d92f3a..367d662 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2632,7 +2632,14 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, { bool haveJob = true; char *snapDir; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virQEMUDriverConfigPtr cfg; + + if (vm->persistent) { + /* Short-circuit, we don't want to remove a persistent domain */ + return; + } + + cfg = virQEMUDriverGetConfig(driver); if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) haveJob = false; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2a4b026..3532973 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1753,8 +1753,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, def = NULL; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); goto cleanup; } @@ -1764,8 +1763,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); qemuDomainObjEndJob(driver, vm); - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); goto cleanup; } @@ -2250,7 +2248,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: qemuDomainObjEndJob(driver, vm); - if (ret == 0 && !vm->persistent) + if (ret == 0) qemuDomainRemoveInactive(driver, vm); cleanup: @@ -3273,7 +3271,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, } } qemuDomainObjEndAsyncJob(driver, vm); - if (ret == 0 && !vm->persistent) + if (ret == 0) qemuDomainRemoveInactive(driver, vm); cleanup: @@ -3774,7 +3772,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, } qemuDomainObjEndAsyncJob(driver, vm); - if (ret == 0 && flags & VIR_DUMP_CRASH && !vm->persistent) + if (ret == 0 && flags & VIR_DUMP_CRASH) qemuDomainRemoveInactive(driver, vm); cleanup: @@ -4054,8 +4052,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver, virDomainAuditStop(vm, "destroyed"); - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); break; case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_RESTART: @@ -6831,7 +6828,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_WARN("Failed to close %s", path); qemuDomainObjEndJob(driver, vm); - if (ret < 0 && !vm->persistent) + if (ret < 0) qemuDomainRemoveInactive(driver, vm); cleanup: @@ -7526,6 +7523,7 @@ static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); + vm->persistent = 0; qemuDomainRemoveInactive(driver, vm); } goto cleanup; @@ -7651,11 +7649,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, * domainDestroy and domainShutdown will take care of removing the * domain obj from the hash table. */ - if (virDomainObjIsActive(vm)) { - vm->persistent = 0; - } else { + vm->persistent = 0; + if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm); - } ret = 0; @@ -15550,12 +15546,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) { - if (!vm->persistent) { - qemuDomainObjEndJob(driver, vm); - qemuDomainRemoveInactive(driver, vm); - goto cleanup; - } - goto endjob; + qemuDomainObjEndJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); + goto cleanup; } if (config) virDomainObjAssignDef(vm, config, false, NULL); @@ -15575,12 +15568,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { - if (!vm->persistent) { - qemuDomainObjEndJob(driver, vm); - qemuDomainRemoveInactive(driver, vm); - goto cleanup; - } - goto endjob; + qemuDomainObjEndJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); + goto cleanup; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3dd3718..7440108 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3483,8 +3483,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_FREE(priv->origname); virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); priv->nbdPort = 0; - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); } virDomainObjEndAPI(&vm); qemuDomainEventQueue(driver, event); @@ -3873,8 +3872,7 @@ qemuMigrationConfirm(virConnectPtr conn, flags, cancelled); qemuMigrationJobFinish(driver, vm); - if (!virDomainObjIsActive(vm) && - (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE))) { + if (!virDomainObjIsActive(vm)) { if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); qemuDomainRemoveInactive(driver, vm); @@ -5357,9 +5355,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, } qemuMigrationJobFinish(driver, vm); - if (!virDomainObjIsActive(vm) && - (!vm->persistent || - (ret == 0 && (flags & VIR_MIGRATE_UNDEFINE_SOURCE)))) { + if (!virDomainObjIsActive(vm) && ret == 0) { if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); qemuDomainRemoveInactive(driver, vm); @@ -5435,7 +5431,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, qemuMigrationJobFinish(driver, vm); else qemuMigrationJobContinue(vm); - if (!virDomainObjIsActive(vm) && !vm->persistent) + if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm); cleanup: @@ -5804,7 +5800,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, VIR_WARN("Unable to encode migration cookie"); qemuMigrationJobFinish(driver, vm); - if (!vm->persistent && !virDomainObjIsActive(vm)) + if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm); cleanup: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7187dc1..c9beadd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -323,8 +323,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuProcessStop(driver, vm, stopReason, stopFlags); virDomainAuditStop(vm, auditReason); - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); cleanup: virObjectUnlock(vm); @@ -3898,8 +3897,7 @@ qemuProcessReconnect(void *opaque) qemuProcessStop(driver, obj, state, stopFlags); } - if (!obj->persistent) - qemuDomainRemoveInactive(driver, obj); + qemuDomainRemoveInactive(driver, obj); cleanup: virDomainObjEndAPI(&obj); @@ -3943,8 +3941,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, "might be incomplete")); /* We can't spawn a thread and thus connect to monitor. Kill qemu. */ qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0); - if (!obj->persistent) - qemuDomainRemoveInactive(src->driver, obj); + qemuDomainRemoveInactive(src->driver, obj); virDomainObjEndAPI(&obj); virObjectUnref(data->conn); @@ -5749,8 +5746,7 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, qemuDomainObjEndJob(driver, dom); - if (!dom->persistent) - qemuDomainRemoveInactive(driver, dom); + qemuDomainRemoveInactive(driver, dom); qemuDomainEventQueue(driver, event); -- 2.4.9
On Wed, Sep 23, 2015 at 03:00:36PM +0200, Michal Privoznik wrote:
So far we have the following pattern occurring over and over again:
if (!vm->persistent) qemuDomainRemoveInactive(driver, vm);
You could've done that earlier so you would save some lines in the patches before. Anyway, nice cleanup;
It's safe to put the check into the function and save some LoC.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 9 ++++++++- src/qemu/qemu_driver.c | 42 ++++++++++++++++-------------------------- src/qemu/qemu_migration.c | 14 +++++--------- src/qemu/qemu_process.c | 12 ++++-------- 4 files changed, 33 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7d92f3a..367d662 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2632,7 +2632,14 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, { bool haveJob = true; char *snapDir; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virQEMUDriverConfigPtr cfg; + + if (vm->persistent) { + /* Short-circuit, we don't want to remove a persistent domain */ + return; + } + + cfg = virQEMUDriverGetConfig(driver);
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) haveJob = false; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2a4b026..3532973 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1753,8 +1753,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, def = NULL;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); goto cleanup; }
@@ -1764,8 +1763,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); qemuDomainObjEndJob(driver, vm); - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); goto cleanup; }
@@ -2250,7 +2248,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: qemuDomainObjEndJob(driver, vm); - if (ret == 0 && !vm->persistent) + if (ret == 0) qemuDomainRemoveInactive(driver, vm);
cleanup: @@ -3273,7 +3271,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, } } qemuDomainObjEndAsyncJob(driver, vm); - if (ret == 0 && !vm->persistent) + if (ret == 0) qemuDomainRemoveInactive(driver, vm);
cleanup: @@ -3774,7 +3772,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, }
qemuDomainObjEndAsyncJob(driver, vm); - if (ret == 0 && flags & VIR_DUMP_CRASH && !vm->persistent) + if (ret == 0 && flags & VIR_DUMP_CRASH) qemuDomainRemoveInactive(driver, vm);
cleanup: @@ -4054,8 +4052,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver,
virDomainAuditStop(vm, "destroyed");
- if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); break;
case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_RESTART: @@ -6831,7 +6828,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_WARN("Failed to close %s", path);
qemuDomainObjEndJob(driver, vm); - if (ret < 0 && !vm->persistent) + if (ret < 0) qemuDomainRemoveInactive(driver, vm);
cleanup: @@ -7526,6 +7523,7 @@ static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); + vm->persistent = 0; qemuDomainRemoveInactive(driver, vm); } goto cleanup; @@ -7651,11 +7649,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, * domainDestroy and domainShutdown will take care of removing the * domain obj from the hash table. */ - if (virDomainObjIsActive(vm)) { - vm->persistent = 0; - } else { + vm->persistent = 0; + if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm); - }
ret = 0;
@@ -15550,12 +15546,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, }
if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) { - if (!vm->persistent) { - qemuDomainObjEndJob(driver, vm); - qemuDomainRemoveInactive(driver, vm); - goto cleanup; - } - goto endjob; + qemuDomainObjEndJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); + goto cleanup; } if (config) virDomainObjAssignDef(vm, config, false, NULL); @@ -15575,12 +15568,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { - if (!vm->persistent) { - qemuDomainObjEndJob(driver, vm); - qemuDomainRemoveInactive(driver, vm); - goto cleanup; - } - goto endjob; + qemuDomainObjEndJob(driver, vm); + qemuDomainRemoveInactive(driver, vm); + goto cleanup; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3dd3718..7440108 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3483,8 +3483,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_FREE(priv->origname); virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); priv->nbdPort = 0; - if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm); } virDomainObjEndAPI(&vm); qemuDomainEventQueue(driver, event); @@ -3873,8 +3872,7 @@ qemuMigrationConfirm(virConnectPtr conn, flags, cancelled);
qemuMigrationJobFinish(driver, vm); - if (!virDomainObjIsActive(vm) && - (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE))) { + if (!virDomainObjIsActive(vm)) { if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); qemuDomainRemoveInactive(driver, vm); @@ -5357,9 +5355,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, }
qemuMigrationJobFinish(driver, vm); - if (!virDomainObjIsActive(vm) && - (!vm->persistent || - (ret == 0 && (flags & VIR_MIGRATE_UNDEFINE_SOURCE)))) { + if (!virDomainObjIsActive(vm) && ret == 0) { if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); qemuDomainRemoveInactive(driver, vm); @@ -5435,7 +5431,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, qemuMigrationJobFinish(driver, vm); else qemuMigrationJobContinue(vm); - if (!virDomainObjIsActive(vm) && !vm->persistent) + if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm);
cleanup: @@ -5804,7 +5800,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, VIR_WARN("Unable to encode migration cookie");
qemuMigrationJobFinish(driver, vm); - if (!vm->persistent && !virDomainObjIsActive(vm)) + if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm);
cleanup: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7187dc1..c9beadd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -323,8 +323,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuProcessStop(driver, vm, stopReason, stopFlags); virDomainAuditStop(vm, auditReason);
- if (!vm->persistent) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm);
cleanup: virObjectUnlock(vm); @@ -3898,8 +3897,7 @@ qemuProcessReconnect(void *opaque) qemuProcessStop(driver, obj, state, stopFlags); }
- if (!obj->persistent) - qemuDomainRemoveInactive(driver, obj); + qemuDomainRemoveInactive(driver, obj);
cleanup: virDomainObjEndAPI(&obj); @@ -3943,8 +3941,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, "might be incomplete")); /* We can't spawn a thread and thus connect to monitor. Kill qemu. */ qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0); - if (!obj->persistent) - qemuDomainRemoveInactive(src->driver, obj); + qemuDomainRemoveInactive(src->driver, obj);
virDomainObjEndAPI(&obj); virObjectUnref(data->conn); @@ -5749,8 +5746,7 @@ qemuProcessAutoDestroy(virDomainObjPtr dom,
qemuDomainObjEndJob(driver, dom);
- if (!dom->persistent) - qemuDomainRemoveInactive(driver, dom); + qemuDomainRemoveInactive(driver, dom);
qemuDomainEventQueue(driver, event);
-- 2.4.9
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
So while working on my previous patches, I've noticed that virDomainRestore implementation in qemu and test drivers has the same problem as I am fixing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3532973..562a0b6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6828,8 +6828,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_WARN("Failed to close %s", path); qemuDomainObjEndJob(driver, vm); - if (ret < 0) - qemuDomainRemoveInactive(driver, vm); cleanup: virDomainDefFree(def); @@ -6837,6 +6835,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_FREE(xml); VIR_FREE(xmlout); virFileWrapperFdFree(wrapperFd); + if (vm && ret < 0) + qemuDomainRemoveInactive(driver, vm); virDomainObjEndAPI(&vm); virNWFilterUnlockFilterUpdates(); return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 01ab1e3..9ccd567 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2132,8 +2132,13 @@ testDomainRestoreFlags(virConnectPtr conn, goto cleanup; def = NULL; - if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) + if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) { + if (!dom->persistent) { + virDomainObjListRemove(privconn->domains, dom); + dom = NULL; + } goto cleanup; + } event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STARTED, -- 2.4.9
On Wed, Sep 23, 2015 at 03:00:33PM +0200, Michal Privoznik wrote:
And this time 'virsh restore' too. The diff to v1 is that more drivers is fixed.
Michal Privoznik (4): virDomainCreateXML: Don't remove persistent domains on error virDomainCreateXML: Make domain definition transient qemu: Move vm->persistent check into qemuDomainRemoveInactive virDomainRestore: Don't keep transient domains around
ACK series
src/bhyve/bhyve_driver.c | 1 + src/libxl/libxl_driver.c | 1 + src/lxc/lxc_driver.c | 7 +++++-- src/openvz/openvz_driver.c | 1 + src/qemu/qemu_domain.c | 9 ++++++++- src/qemu/qemu_driver.c | 39 ++++++++++++++++----------------------- src/qemu/qemu_migration.c | 14 +++++--------- src/qemu/qemu_process.c | 12 ++++-------- src/test/test_driver.c | 15 +++++++++++++-- src/uml/uml_driver.c | 8 +++++--- src/vmware/vmware_driver.c | 7 +++++-- 11 files changed, 64 insertions(+), 50 deletions(-)
-- 2.4.9
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Michal Privoznik