Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
On 09/28/2018 11:36 AM, Wang Yechao wrote:
> After calling qemuAgentClose(), it is still possible for
> the QEMU Agent I/O event callback to get invoked. This
> will trigger an agent error because mon->fd has been set
> to -1 at this point. Then vm->privateData->agentError is
> always 'true' except that restart libvirtd or restart
> qemu-guest-agent process in guest.
>
> Silently ignore the case where mon->fd is -1, likewise for
> mon->watch being zero.
>
> Signed-off-by: Wang Yechao <wang.yechao255(a)zte.com.cn>
> ---
> v1 patch:
> https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
>
> Changes in v2:
> - do not set agentError, let agent state as disconnected instead of error.
> ---
> src/qemu/qemu_agent.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 97ad0e7..d842b0e 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon)
> VIR_EVENT_HANDLE_HANGUP |
> VIR_EVENT_HANDLE_ERROR;
>
> + if (!mon->watch)
> + return;
> +
> if (mon->lastError.code == VIR_ERR_OK) {
> events |= VIR_EVENT_HANDLE_READABLE;
>
> @@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque)
> VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd,
events);
> #endif
>
> + if (mon->fd == -1 || mon->watch == 0) {
> + virObjectUnlock(mon);
> + virObjectUnref(mon);
> + return;
> + }
> +
Is this safe to do? What if there's a thread waiting for a message from
the agent? Shouldn't @eof variable be set in this case so that
appropriate code is run?
It is safe to do. The waiting thread is waked up when qemuAgentClose
invoked, and it cannot get any message from the agent at this time.
There is no need to set the @eof variable, because the EOF callback's job
has been done when closing the agent.
> if (mon->fd != fd || mon->watch != watch) {
> if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> eof = true;
> @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon)
> virObjectLock(mon);
>
> if (mon->fd >= 0) {
> - if (mon->watch)
> + if (mon->watch) {
> virEventRemoveHandle(mon->watch);
> + mon->watch = 0;
> + }
> VIR_FORCE_CLOSE(mon->fd);
> }
>
>
Michal
---
Best wishes
Wang Yechao