[libvirt] [PATCH v2 0/3] qemu: agent: cleanup agent error flag correctly
Patches 1 and 2 are refactorings to help review patch 3. diff from v1: ============= 1. error flag cleanup logic clarified 2. patches that touch io loop are dropped Nikolay Shirokovskiy (3): qemu: agent: handle agent connection errors in one place qemu: agent: remove redundant check qemu: agent: clenup agent error flag correctly src/qemu/qemu_driver.c | 12 +++------- src/qemu/qemu_migration.c | 13 ++-------- src/qemu/qemu_process.c | 61 +++++++++++++---------------------------------- 3 files changed, 22 insertions(+), 64 deletions(-) -- 1.8.3.1
qemuConnectAgent return -1 or -2 in case of different errors. A. -1 is a case of unsuccessuful connection to guest agent. B. -2 is a case of destoyed domain during connection attempt. All qemuConnectAgent callers handle the first error the same way so let's move this logic into qemuConnectAgent itself. Patched function returns 0 in case A and -1 in case B. --- src/qemu/qemu_driver.c | 10 ++------ src/qemu/qemu_migration.c | 13 ++--------- src/qemu/qemu_process.c | 58 ++++++++++++----------------------------------- 3 files changed, 19 insertions(+), 62 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c8414..9ebe66b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4409,7 +4409,6 @@ processSerialChangedEvent(virQEMUDriverPtr driver, virObjectEventPtr event = NULL; virDomainDeviceDef dev; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if (connected) newstate = VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED; @@ -4462,13 +4461,8 @@ processSerialChangedEvent(virQEMUDriverPtr driver, if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0")) { if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) { - if (!priv->agent) { - if ((rc = qemuConnectAgent(driver, vm)) == -2) - goto endjob; - - if (rc < 0) - priv->agentError = true; - } + if (!priv->agent && qemuConnectAgent(driver, vm) < 0) + goto endjob; } else { if (priv->agent) { qemuAgentClose(priv->agent); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d4a55d8..b029319 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6183,7 +6183,6 @@ qemuMigrationFinish(virQEMUDriverPtr driver, unsigned short port; unsigned long long timeReceived = 0; virObjectEventPtr event; - int rc; qemuDomainJobInfoPtr jobInfo = NULL; bool inPostCopy = false; bool doKill = true; @@ -6256,16 +6255,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) goto endjob; - if ((rc = qemuConnectAgent(driver, vm)) < 0) { - if (rc == -2) - goto endjob; - - VIR_WARN("Cannot connect to QEMU guest agent for %s", - vm->def->name); - virResetLastError(); - priv->agentError = true; - } - + if (qemuConnectAgent(driver, vm) < 0) + goto endjob; if (flags & VIR_MIGRATE_PERSIST_DEST) { if (qemuMigrationPersist(driver, vm, mig, !v3proto) < 0) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..34f8b04 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -204,7 +204,6 @@ int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - int ret = -1; qemuAgentPtr agent = NULL; virDomainChrDefPtr config = qemuFindAgentConfig(vm->def); @@ -248,8 +247,7 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) qemuAgentClose(agent); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("guest crashed while connecting to the guest agent")); - ret = -2; - goto cleanup; + return -1; } if (virSecurityManagerClearSocketLabel(driver->securityManager, @@ -260,18 +258,18 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) goto cleanup; } - priv->agent = agent; - - if (priv->agent == NULL) { + if (!priv->agent) VIR_INFO("Failed to connect agent for %s", vm->def->name); - goto cleanup; - } - - ret = 0; cleanup: - return ret; + if (!priv->agent) { + VIR_WARN("Cannot connect to QEMU guest agent for %s", vm->def->name); + priv->agentError = true; + virResetLastError(); + } + + return 0; } @@ -3245,7 +3243,6 @@ qemuProcessReconnect(void *opaque) int reason; virQEMUDriverConfigPtr cfg; size_t i; - int ret; unsigned int stopFlags = 0; bool jobStarted = false; virCapsPtr caps = NULL; @@ -3389,16 +3386,8 @@ qemuProcessReconnect(void *opaque) if (qemuProcessUpdateDevices(driver, obj) < 0) goto error; - /* Failure to connect to agent shouldn't be fatal */ - if ((ret = qemuConnectAgent(driver, obj)) < 0) { - if (ret == -2) - goto error; - - VIR_WARN("Cannot connect to QEMU guest agent for %s", - obj->def->name); - virResetLastError(); - priv->agentError = true; - } + if (qemuConnectAgent(driver, obj) < 0) + goto error; /* update domain state XML with possibly updated state in virDomainObj */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj, driver->caps) < 0) @@ -5533,16 +5522,8 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup; - /* Failure to connect to agent shouldn't be fatal */ - if ((rv = qemuConnectAgent(driver, vm)) < 0) { - if (rv == -2) - goto cleanup; - - VIR_WARN("Cannot connect to QEMU guest agent for %s", - vm->def->name); - virResetLastError(); - priv->agentError = true; - } + if (qemuConnectAgent(driver, vm) < 0) + goto cleanup; VIR_DEBUG("Detecting if required emulator features are present"); if (!qemuProcessVerifyGuestCPU(driver, vm, asyncJob)) @@ -6232,7 +6213,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virCapsPtr caps = NULL; bool active = false; - int ret; VIR_DEBUG("Beginning VM attach process"); @@ -6359,16 +6339,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (qemuProcessWaitForMonitor(driver, vm, QEMU_ASYNC_JOB_NONE, priv->qemuCaps, NULL) < 0) goto error; - /* Failure to connect to agent shouldn't be fatal */ - if ((ret = qemuConnectAgent(driver, vm)) < 0) { - if (ret == -2) - goto error; - - VIR_WARN("Cannot connect to QEMU guest agent for %s", - vm->def->name); - virResetLastError(); - priv->agentError = true; - } + if (qemuConnectAgent(driver, vm) < 0) + goto error; VIR_DEBUG("Detecting VCPU PIDs"); if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE, false) < 0) -- 1.8.3.1
--- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9ebe66b..ae1b9fe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4461,7 +4461,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver, if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0")) { if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) { - if (!priv->agent && qemuConnectAgent(driver, vm) < 0) + if (qemuConnectAgent(driver, vm) < 0) goto endjob; } else { if (priv->agent) { -- 1.8.3.1
Sometimes after domain restart agent is unavailabe even if it is up and running in guest. Diagnostic message is "QEMU guest agent is not available due to an error" that is 'priv->agentError' is set. Investiagion shows that 'priv->agent' is not NULL, so error flag is set probably during domain shutdown process and not cleaned up eventually. The patch is quite simple - just clean up error flag unconditionally upon domain stop. Other hunks address other cases when error flag is not cleaned up. 1. processSerialChangedEvent. We need to clean error flag unconditionally here too. For example if upon first 'connected' event we fail to connect and set error flag and then connect on second 'connected' event then error flag will remain set erroneously and make agent unavailable. 2. qemuProcessHandleAgentEOF. If error flag is set and we get EOF we need to change state (and diagnostic) from 'error' to 'not connected'. --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ae1b9fe..c2a6336 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4467,8 +4467,8 @@ processSerialChangedEvent(virQEMUDriverPtr driver, if (priv->agent) { qemuAgentClose(priv->agent); priv->agent = NULL; - priv->agentError = false; } + priv->agentError = false; } event = virDomainEventAgentLifecycleNewFromObj(vm, newstate, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 34f8b04..ffc8126 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -151,6 +151,7 @@ qemuProcessHandleAgentEOF(qemuAgentPtr agent, qemuAgentClose(agent); priv->agent = NULL; + priv->agentError = false; virObjectUnlock(vm); return; @@ -5963,8 +5964,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, if (priv->agent) { qemuAgentClose(priv->agent); priv->agent = NULL; - priv->agentError = false; } + priv->agentError = false; if (priv->mon) { qemuMonitorClose(priv->mon); -- 1.8.3.1
On 16.11.2016 14:43, Nikolay Shirokovskiy wrote:
Patches 1 and 2 are refactorings to help review patch 3.
diff from v1: ============= 1. error flag cleanup logic clarified 2. patches that touch io loop are dropped
Nikolay Shirokovskiy (3): qemu: agent: handle agent connection errors in one place qemu: agent: remove redundant check qemu: agent: clenup agent error flag correctly
src/qemu/qemu_driver.c | 12 +++------- src/qemu/qemu_migration.c | 13 ++-------- src/qemu/qemu_process.c | 61 +++++++++++++---------------------------------- 3 files changed, 22 insertions(+), 64 deletions(-)
ACK series. Michal
22-Nov-16 18:17, Michal Privoznik пишет:
On 16.11.2016 14:43, Nikolay Shirokovskiy wrote:
Patches 1 and 2 are refactorings to help review patch 3.
diff from v1: ============= 1. error flag cleanup logic clarified 2. patches that touch io loop are dropped
Nikolay Shirokovskiy (3): qemu: agent: handle agent connection errors in one place qemu: agent: remove redundant check qemu: agent: clenup agent error flag correctly
src/qemu/qemu_driver.c | 12 +++------- src/qemu/qemu_migration.c | 13 ++-------- src/qemu/qemu_process.c | 61 +++++++++++++---------------------------------- 3 files changed, 22 insertions(+), 64 deletions(-)
ACK series.
Michal
Pushed the series. Maxim
participants (3)
-
Maxim Nestratov -
Michal Privoznik -
Nikolay Shirokovskiy