On 26.10.2016 23:18, John Ferlan wrote:
>
>
> 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.
I did not suggested that. I don't want to drop error condition I just
want to drop the error flag to check for this condition.
I mean if we close monitor on error just as on eof then the priv->agent
pointer check will be enough. The commit message explains that there
is not sense keeping monitor after error anyway.
>
> Wouldn't the EOF tell us that we're all done processing whatever was
> sent our way before we got the first error?
We just don't use EOF this way. Some kind of half close from the agent.
>
> Not so sure about this one. I'd have to think more about things in light
> of what's being chased.
I want to drop the error flag after we discovered the kind of problems that it can cause)))
>
> 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) {
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list