[libvirt] [PATCH 02/10] qemu: agent: dont set error after agent cleanup

[Nikolay Shirokovskiy]

On 26.10.2016 22:49, John Ferlan wrote:
> 
> 
> On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote:
>> If there is an error event after eof event then after agent
>> is cleaned up on eof error flag will be set back and remains
>> set after next domain start up making agent unavailable.
>> Thus let's check before set this flag on error event.
>> ---
>>  src/qemu/qemu_process.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index a581f96..3637f4b 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -180,7 +180,8 @@ qemuProcessHandleAgentError(qemuAgentPtr agent ATTRIBUTE_UNUSED,
>>  
>>      priv = vm->privateData;
>>  
>> -    priv->agentError = true;
>> +    if (priv->agent)
>> +        priv->agentError = true;
> 
> A NULL priv->agent happens for the following reasons:
> 
> 1. qemuDomainObjExitAgent - when the last reference is removed... I
> would hope we're not falling into this path for what you've seen...
> 
> 2. processSerialChangedEvent - after a qemuAgentClose for VSERPORT
> callback and agentError is set to false... (so that seems to be
> happening properly)
> 
> 3. qemuProcessHandleAgentEOF (discussed in patch 1) - where I don't
> believe you should be clearing agentError
> 
> 4. qemuConnectAgent has a failure after qemuAgentOpen creates the
> 'agent', but before sets priv->agent = agent.
> 
> 5. qemuProcessStop - stops a started agent *and* clears agentError flag
> (so that seems to be happening properly).
> 
> w/r/t #4... there are windows in qemuConnectAgent where some error after
> qemuAgentOpen *succeeds* could trigger this function to be called before
> priv->agent is filled in.

Can not agree.
Places you mentioned and qemuProcessHandleAgentError are serialized
thru domain lock. So if the agent start up is ok and error is 
in io loop we do not break anything - when qemuProcessHandleAgentError
get change to handle error priv->agent is not NULL. On the other
hand this patch cures the situation that similar to described in patch 1.

1. shutdown
2. we get VSERPORT
3. we get ERROR from agent and set error flag
4. again we have problems on next domain start and agent usage, just
as described in patch 1.

Actually this is more like a protection. As on step 2 we close agent
and eventually remove handle from io loop and this operation is
synchronous (no more events will be delivered).

> 
> Thus by only clearing it when priv->agent is set, we could miss some
> error that occurred.  While the "next" call could also fail and
> eventually set the error, I'm not convinced that only clearing when
> priv->agent is set is right.
> 
> Long way of saying NAK on this one.
> 
> John
>>  
>>      virObjectUnlock(vm);
>>  }
>>