[libvirt] [PATCH 0/5] Couple of 'virsh create' fixes
*** BLURB HERE *** Michal Privoznik (5): qemuDomainCreateXML: Don't remove persistent domains on error qemuDomainCreateXML: Make domain definition transient qemu: Move vm->persistent check into qemuDomainRemoveInactive lxcDomainCreateXMLWithFiles: Don't remove persistent domains on error lxcDomainCreateXMLWithFiles: Make domain definition transient src/lxc/lxc_driver.c | 7 +++++-- src/qemu/qemu_domain.c | 9 ++++++++- src/qemu/qemu_driver.c | 37 +++++++++++++++---------------------- src/qemu/qemu_migration.c | 14 +++++--------- src/qemu/qemu_process.c | 12 ++++-------- 5 files changed, 37 insertions(+), 42 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, 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/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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; } -- 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. Le sigh. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+) 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; -- 2.4.9
On 09/22/2015 09:28 AM, 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. Le sigh.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+)
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;
It looks like the libxl driver should be fixed similarly, right? Regards, Jim
On 23.09.2015 03:53, Jim Fehlig wrote:
On 09/22/2015 09:28 AM, 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. Le sigh.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+)
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;
It looks like the libxl driver should be fixed similarly, right?
Oh right. Let me respin with more drivers fixed. Michal
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
Similarly to one of previous commits, don't lose a persistent domain on some error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 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; } -- 2.4.9
Yet again, the same error that was in qemu driver is in lxc driver too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 1 + 1 file changed, 1 insertion(+) 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; -- 2.4.9
participants (2)
-
Jim Fehlig -
Michal Privoznik