Re: [libvirt] [PATCH 01/10] qemu: agent: unset error flag in EOF handler

On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote: What error occurred that would be avoided by clearing the agentError flag unconditionally? Considering that agentError being set is primarily used by "new connection" request processing (e.g. callers to qemuDomainAgentAvailable) in order to ascertain whether they should actually attempt the connection or fail because of some condition that would eventually lead to failure. I would think EOF from qemu_monitor for the qemu_agent would be one of those conditions *before* the domain state is not RUNNING or the priv->agent has been fully removed. I think what I'm missing is what sequence of events led to this condition? How was it reproduced. So I understand "eventually" your goal is to remove agentError in patch 6, but you need to lead down the path to get there. So let's see if I can follow along... The issue you chased is related to domain shutdown processing "order". The purpose up to this point in time for 'agentError' is to ensure callers of qemuDomainAgentAvailable() will fail before trying to use the agent. The qemuDomainAgentAvailable checks domain active, agentError, and whether the priv->agent is available. So the contention is during shutdown the EOF event firing at about the same time as the processSerialChangedEvent has caused an issue, but it's not 100% clear just from the description the sequence of events and the issue. So let's consider why these things happen: 1. The 'eofNotify' comes from the qemuMonitorIO function - ostensibly if the guest is shutting down 2. The 'processSerialChangedEvent' seems to occur for multiple reasons - startup, shutdown, some sort of channel failure (unix socket or pty). If I'm reading things correctly, this path is only involved when VSERPORT is being used. So, is this a case of guest startup where the initial agent connect/startup occurs (VSERPORT) and a guest shutdown racing? Or is this purely a shutdown ordering thing? Because this is cleared, then qemuDomainAgentAvailable will fail because the agent is not present instead of failing with there was some error. That would seem to make the following clearing "safe" since we know we'll have a failure any way, but... How does this alone solve the problem you saw. AFAICT it just changes *where* the error occurs and the error message displayed. Conversely, if for some reason for either 1. qemuConnectAgent failed and is being "flagged" in 1a. processSerialChangedEvent (for initial CONNECTED failure when QEMU_CAPS_VSERPORT_CHANGE) 1b. qemuMigrationFinish (for migration target) 1c. qemuProcessReconnect (for libvirtd restart) 1d. qemuProcessLaunch (initial domain startup before QEMU_CAPS_VSERPORT_CHANGE) 1e. qemuProcessAttach (for qemu-attach to existing qemu process) or 2. qemuProcessHandleAgentError - some sort of error and we're stopping any further agent communication. then you're potentially clearing agentError and could conceivably cause "extra" debugging steps to be taken only to figure out the reason it was because of some error rather than the "next" connection failing and indicating the failure was because of some previous error (it's a startup race for some other thread desiring to use the agent). So given all that - I'm not 100% clear how this really fixes the issue you say. Although, I have to wonder if the bug you're chasing is perhaps that qemuConnectAgent doesn't clear agentError at the right time as patch 3 attempts to fix. After reading through patch 4 - I think Patch 4 and a slightly modified patch 3 will fix what you're seeing, but I'd only know for sure given more details about what was seen. John