[libvirt] [PATCH 0/3] Do not check for domain liveness in virDomainObjSetDefTransient
virDomainObjSetDefTransient has an unintuitive attribute 'live': when set to 'true', it ignores the virDomainObjIsActive test. The most common usage is on domain startup, where most of the callers want to create a transient definition even though the domain is not yet active. The only place where live=false is still required is virDomainObjGetPersistentDef (and its usage could possibly be cleaned up too, since it's only called in the drivers that already do SetDefTransient on domain startup). Split out the virDomainObjIsActive check into virDomainObjGetPersistentDef and remove it for the rest of the callers along with the 'live' attribute. Ján Tomko (3): Clean up redundant usage of virDomainObjSetDefTransient Check if the domain is active in virDomainObjGetPersistentDef Do not check for domain liveness in virDomainObjSetDefTransient src/conf/domain_conf.c | 15 ++++----------- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_domain.c | 3 +-- src/lxc/lxc_process.c | 6 +----- src/qemu/qemu_process.c | 4 ++-- src/test/test_driver.c | 2 +- src/uml/uml_driver.c | 5 ++--- 7 files changed, 12 insertions(+), 26 deletions(-) -- 2.7.3
Commit 45ec297d from November 2010: Make state driver device hotplug/update actually transient added virDomainObjSetDefTransient calls to the domain startup function in several drivers. In November 2011, commit 8866eed: Set aliases for LXC/UML console devices added a call earlier in the startup function, without removing the existing ones. Also, in the UML driver it seems the function never did anything useful - vm->def->id is set asynchronnously in umlNotifyEvent. At the time of calling virDomainObjSetDefTransient with live=false, vm->def->id was likely still -1, making the call a no-op. --- src/lxc/lxc_process.c | 4 ---- src/uml/uml_driver.c | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 17fbc5f..058c3e1 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1542,10 +1542,6 @@ int virLXCProcessStart(virConnectPtr conn, conn, lxcProcessAutoDestroy) < 0) goto cleanup; - if (virDomainObjSetDefTransient(caps, driver->xmlopt, - vm, false) < 0) - goto cleanup; - /* We don't need the temporary NIC names anymore, clear them */ virLXCProcessCleanInterfaces(vm->def); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 923c3f6..d68054e 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1130,7 +1130,7 @@ static int umlStartVMDaemon(virConnectPtr conn, umlProcessAutoDestroyAdd(driver, vm, conn) < 0) goto cleanup; - ret = virDomainObjSetDefTransient(driver->caps, driver->xmlopt, vm, false); + ret = 0; cleanup: VIR_FORCE_CLOSE(logfd); virCommandFree(cmd); -- 2.7.3
On Fri, May 27, 2016 at 17:45:13 +0200, Ján Tomko wrote:
Commit 45ec297d from November 2010: Make state driver device hotplug/update actually transient added virDomainObjSetDefTransient calls to the domain startup function in several drivers.
In November 2011, commit 8866eed: Set aliases for LXC/UML console devices added a call earlier in the startup function, without removing the existing ones.
Also, in the UML driver it seems the function never did anything useful - vm->def->id is set asynchronnously in umlNotifyEvent. At the time of calling virDomainObjSetDefTransient with live=false, vm->def->id was likely still -1, making the call a no-op.
Well, since it was called earlier in the same function it was a no-op regardless of the id :)
--- src/lxc/lxc_process.c | 4 ---- src/uml/uml_driver.c | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-)
ACK after the release.
Calling virDomainObjSetDefTransient with live=false is a no-op on an inactive domain. Only call it on an active domain, since this is the only place using the live bool. --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 568c699..baa51fd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2936,7 +2936,8 @@ virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { - if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) + if (virDomainObjIsActive(domain) && + virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL; if (domain->newDef) -- 2.7.3
On Fri, May 27, 2016 at 17:45:14 +0200, Ján Tomko wrote:
Calling virDomainObjSetDefTransient with live=false is a no-op on an inactive domain.
Only call it on an active domain, since this is the only place using the live bool. --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK
Remove the live attribute and mark the definition as transient whether the domain is runing or not. There were only two callers left calling with live=false: * testDomainStartState, where the domain already is active because we assigned vm->def->id just a few lines above the call * virDomainObjGetPersistentDef, which now only calls virDomainObjSetDefTransient for an active domain --- src/conf/domain_conf.c | 14 +++----------- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_domain.c | 3 +-- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 4 ++-- src/test/test_driver.c | 2 +- src/uml/uml_driver.c | 3 +-- 7 files changed, 10 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index baa51fd..48cfb7b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2885,29 +2885,21 @@ virDomainObjWaitUntil(virDomainObjPtr vm, /* - * Mark the running VM config as transient. Ensures transient hotplug + * Mark the current VM config as transient. Ensures transient hotplug * operations do not persist past shutdown. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @param live if true, run this operation even for an inactive domain. - * this allows freely updated domain->def with runtime defaults before - * starting the VM, which will be discarded on VM shutdown. Any cleanup - * paths need to be sure to handle newDef if the domain is never started. * @return 0 on success, -1 on failure */ int virDomainObjSetDefTransient(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - virDomainObjPtr domain, - bool live) + virDomainObjPtr domain) { int ret = -1; - if (!virDomainObjIsActive(domain) && !live) - return 0; - if (!domain->persistent) return 0; @@ -2937,7 +2929,7 @@ virDomainObjGetPersistentDef(virCapsPtr caps, virDomainObjPtr domain) { if (virDomainObjIsActive(domain) && - virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) + virDomainObjSetDefTransient(caps, xmlopt, domain) < 0) return NULL; if (domain->newDef) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b1953b3..c182747 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2525,8 +2525,7 @@ void virDomainObjAssignDef(virDomainObjPtr domain, virDomainDefPtr *oldDef); int virDomainObjSetDefTransient(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - virDomainObjPtr domain, - bool live); + virDomainObjPtr domain); virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 8a3866f..9fe8be0 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1096,8 +1096,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, VIR_FREE(managed_save_path); } - if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, - vm, true) < 0) + if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm) < 0) goto cleanup; if (virDomainLockProcessStart(driver->lockManager, diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 058c3e1..07eb22a 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1286,7 +1286,7 @@ int virLXCProcessStart(virConnectPtr conn, * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, true) < 0) + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) goto cleanup; /* Run an early hook to set-up missing devices */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e847cd1..7a4a2ca 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4447,7 +4447,7 @@ qemuProcessInit(virQEMUDriverPtr driver, * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, true) < 0) + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) goto stop; if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { @@ -5944,7 +5944,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, true) < 0) + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) goto error; vm->def->id = qemuDriverAllocateID(driver); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a51eb09..fb40e87 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -601,7 +601,7 @@ testDomainStartState(testDriverPtr privconn, if (virDomainObjSetDefTransient(privconn->caps, privconn->xmlopt, - dom, false) < 0) { + dom) < 0) { goto cleanup; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index d68054e..a674c12 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1099,8 +1099,7 @@ static int umlStartVMDaemon(virConnectPtr conn, * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(driver->caps, driver->xmlopt, - vm, true) < 0) { + if (virDomainObjSetDefTransient(driver->caps, driver->xmlopt, vm) < 0) { VIR_FORCE_CLOSE(logfd); return -1; } -- 2.7.3
On Fri, May 27, 2016 at 17:45:15 +0200, Ján Tomko wrote:
Remove the live attribute and mark the definition as transient whether the domain is runing or not.
There were only two callers left calling with live=false: * testDomainStartState, where the domain already is active because we assigned vm->def->id just a few lines above the call * virDomainObjGetPersistentDef, which now only calls virDomainObjSetDefTransient for an active domain --- src/conf/domain_conf.c | 14 +++----------- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_domain.c | 3 +-- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 4 ++-- src/test/test_driver.c | 2 +- src/uml/uml_driver.c | 3 +-- 7 files changed, 10 insertions(+), 21 deletions(-)
ACK
participants (2)
-
Ján Tomko -
Peter Krempa