Re: [libvirt] [PATCH 04/10] qemu: agent: handle agent connection errors in one place

On 10/28/2016 03:37 AM, Nikolay Shirokovskiy wrote: Commit messages don't have to be a long story, but they shouldn't be empty. If you're relying on purely the 60 character commit message to convey what's been done, then the changes would have to be similarly short. Part of your refactor altered logic and that's something that needs to be mentioned. EOF and Error are different. That's why there's two different callbacks. The agent implementation has 'chosen' to just make Error essentially fatal for all future attempts while the guest is running. I seem to recall recent discussion regarding why there's a separate Error handler and the thought that some sort of error recover could be done, but I cannot find it in the last 3 months of archives on a simple title search. It could have been a conversation at KVM Forum too. Could someone come along and alter the current reality - perhaps. Your "choice" is remove a bunch of code that someone would then have to add back later. I'm erring on the side of caution - having separate Error and EOF does work - what doesn't work right is the 'agentError' logic. But we can fix that. As for difficult to follow - that's perhaps my other hot button issue - it's not self documenting and that's frustrating. Some will tout usage of git log history to understand how something's supposed to work. I land more on the side of - if you learn something about code you're changing, then leave entrails in the code to hopefully help the next person reading the code and not the git log's. It's a delicate balance for my penchant to be too verbose vs. providing sufficient information. I'll sometimes leave a comment somewhere that means nothing to someone else, but causes me to remember why I did something (like the pensieve in the Harry Potter movies ;-)). Check the callers of qemuDomainAgentAvailable and imagine a day where we can take an error and do something smarter... If there's multiple "things" an Agent can do and we disable them all just because of an error that's kind of frustrating. So maybe someone comes along and classifies the calls into groups (state, vcpu, FS, time, interface, security). Then let's say we have some sort of error from a vcpu call - does that necessarily mean we should disable getting/setting time? Do we know enough about the error. The vmagent is not something I focus on, so I don't have the answer to whether we could make something recoverable. I also know there's a Linux agent and Windows agent - so In any case, I don't think we should disable error processing just because it's difficult to follow... As an aside - another "interesting" part of the VSERPORT processing... qemuAgentConnect will avoid running opening the channel if VSERPORT capability is set *and* config->state != CONNECTED. However, the VSERPORT processing code in processSerialChangedEvent will only work if the target.name is "org.qemu.guest_agent.0"... Sure that's our default, but nonetheless interesting about two different checks. It's semantics... the error someone will get until VSERPORT runs is agent unavailable due to error"... Once the serial port magic happens, it could then be running, error, or not connected. But this would only be for the case where guest running, vmagent error, guest shutdown, and guest start (until serial port is connect). FWIW: One way around that is in the modern (processSerialChangedEvent) code rather than clearing agentError only if priv->agent was set, you could also unconditionally set it after/before. IOW: DISCONNECTED was received. A possible downside to that option is it's not architecturally clear to me what conditions other than grep-ing thru the JSON return data in qemuMonitorJSONExtractChardevInfo what could cause DISCONNECTED. Does it only occur at shutdown? Or could it occur when say some network/socket error causes a disconnect. In which case, a running guest/agent could have an error, get a disconnect, but clear agentError 'unexpectedly'... Does the agent logic at some point in time send a second CONNECTED if for some reason a network is restored. Again, I'm not that familiar with the inner workings there, so I err on the side of caution and not unconditionally clear. John