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

[Nikolay Shirokovskiy]

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) {
>>