Re: [libvirt] [PATCH 06/10] qemu: agent: drop agent error flag

On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote: > Let's take a closer look at qemuAgentIO. In the case of error > we stop listening to any events besides error and eof. > Then set last error so that all next loop invocations do very little: > > 1. if it is an error then just call error callback once more. > Current callback is noop for subsequent invocations. > > 2. if it is an eof then call eof callback, it will close > monitor eventually. > > So why waiting for eof? Let's just close monitor on error. > Now we can drop error flag and deal with NULL monitor > case only (qemuDomainAgentAvailable stays correct). > --- > src/qemu/qemu_domain.c | 8 -------- > src/qemu/qemu_domain.h | 1 - > src/qemu/qemu_driver.c | 1 - > src/qemu/qemu_process.c | 19 ++----------------- > 4 files changed, 2 insertions(+), 27 deletions(-) > While we're not "reading" anything - why continue to sent and wait for more stuff if before sending (e.g. qemuDomainAgentAvailable callers) we could detect that there was a failure and thus we shouldn't even attempt the send.

Wouldn't the EOF tell us that we're all done processing whatever was sent our way before we got the first error?

Not so sure about this one. I'd have to think more about things in light of what's being chased.

John > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index cd788c8..b6756b1 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -5055,14 +5055,6 @@ qemuDomainAgentAvailable(virDomainObjPtr vm, > } > return false; > } > - if (priv->agentError) { > - if (reportError) { > - virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", > - _("QEMU guest agent is not " > - "available due to an error")); > - } > - return false; > - } > > if (priv->agent) > return true; > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index 521531b..257bfcb 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -177,7 +177,6 @@ struct _qemuDomainObjPrivate { > unsigned long long monStart; > > qemuAgentPtr agent; > - bool agentError; > unsigned long long agentStart; > > bool gotShutdown; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index edff973..b6fb1df 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4455,7 +4455,6 @@ processSerialChangedEvent(virQEMUDriverPtr driver, > if (priv->agent) { > qemuAgentClose(priv->agent); > priv->agent = NULL; > - priv->agentError = false; > } > } > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 78d530f..3da1bd5 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -151,7 +151,6 @@ qemuProcessHandleAgentEOF(qemuAgentPtr agent, > > qemuAgentClose(agent); > priv->agent = NULL; > - priv->agentError = false; > > virObjectUnlock(vm); > return; > @@ -169,21 +168,10 @@ qemuProcessHandleAgentEOF(qemuAgentPtr agent, > * allowed > */ > static void > -qemuProcessHandleAgentError(qemuAgentPtr agent ATTRIBUTE_UNUSED, > +qemuProcessHandleAgentError(qemuAgentPtr agent, > virDomainObjPtr vm) > { > - qemuDomainObjPrivatePtr priv; > - > - VIR_DEBUG("Received error from agent on %p '%s'", vm, vm->def->name); > - > - virObjectLock(vm); > - > - priv = vm->privateData; > - > - if (priv->agent) > - priv->agentError = true; > - > - virObjectUnlock(vm); > + qemuProcessHandleAgentEOF(agent, vm); > } > > static void qemuProcessHandleAgentDestroy(qemuAgentPtr agent, > @@ -209,8 +197,6 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) > qemuAgentPtr agent = NULL; > virDomainChrDefPtr config = qemuFindAgentConfig(vm->def); > > - priv->agentError = false; > - > if (!config) > return 0; > > @@ -5915,7 +5901,6 @@ void qemuProcessStop(virQEMUDriverPtr driver, > if (priv->agent) { > qemuAgentClose(priv->agent); > priv->agent = NULL; > - priv->agentError = false; > } > > if (priv->mon) { >