There's no commit message...
You're altering qemuConnectAgent to return -1 on the only error and
perform the VIR_WARN plus agentError = true on other "soft" failures.
Not exactly nothing going on!
On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote:
> ---
> src/qemu/qemu_driver.c | 8 +------
> src/qemu/qemu_migration.c | 13 ++---------
> src/qemu/qemu_process.c | 58 ++++++++++++-----------------------------------
> 3 files changed, 17 insertions(+), 62 deletions(-)
>
This idea seems to have merit too - something that I've thought now that
I've read the first 3 patches...
I think you should have started with this patch, it probably would have
made the others easier to think about. In fact, I'm curious if with just
this change and the suggestion in patch 3 for clearing agentError
whether you can reproduced the bug you're trying to fix/resolve without
any of the other patches.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 12ddbc0..edff973 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4397,7 +4397,6 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
> virObjectEventPtr event = NULL;
> virDomainDeviceDef dev;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - int rc;
>
> if (connected)
> newstate = VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED;
> @@ -4450,13 +4449,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)
> + if (!priv->agent && qemuConnectAgent(driver, vm) < 0)
FWIW:
qemuConnectAgent returns 0 "if (priv->agent)", so there's one less check
here too. Could just be "if (qemuConnectAgent(driver, vm) < 0)"
> goto endjob;
The indention for this is off (remove leading 4 spaces)
> -
> - if (rc < 0)
> - priv->agentError = true;
> - }
> } else {
> if (priv->agent) {
> qemuAgentClose(priv->agent);
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index e2ca330..0a02236 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -6171,7 +6171,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;
> @@ -6244,16 +6243,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 42f7f84..d7c9ce3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -206,7 +206,6 @@ int
> qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - int ret = -1;
> qemuAgentPtr agent = NULL;
> virDomainChrDefPtr config = qemuFindAgentConfig(vm->def);
>
> @@ -252,8 +251,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,
> @@ -264,18 +262,16 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
> goto cleanup;
> }
>
> -
> priv->agent = agent;
>
> - if (priv->agent == NULL) {
> - VIR_INFO("Failed to connect agent for %s", vm->def->name);
Interesting a "marker" of sorts to know the virAgentOpen "failed" is
that we'd get an Informational "Failed to connect..." followed shortly
thereafter by a Warning "Cannot connect..." (depending of course on
one's message display severity level).
I think if you "restore" this without the goto cleanup (below) and of
course the removal of the {}, then at least message parity is
achieved... I suppose it could move up into the "if (agent == NULL)"
too, but then it could be given even though an Error is reported.
It's not that important, but it does leave some breadcrumbs.
Again I'd like to see the breadcrumbs with just this patch applied to
reproduced what you're chasing in patches 1-4...
John
> - 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;
> }
>
>
> @@ -3249,7 +3245,6 @@ qemuProcessReconnect(void *opaque)
> int reason;
> virQEMUDriverConfigPtr cfg;
> size_t i;
> - int ret;
> unsigned int stopFlags = 0;
> bool jobStarted = false;
> virCapsPtr caps = NULL;
> @@ -3393,16 +3388,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)
> @@ -5488,16 +5475,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))
> @@ -6176,7 +6155,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");
>
> @@ -6303,16 +6281,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)
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list