[libvirt] [PATCH] qemu: monitor: fix graceful shutdown corner cases

[Nikolay Shirokovskiy]

Please delay review, I plan to provide better patch.

On 08.09.2017 10:16, Nikolay Shirokovskiy wrote:
> Patch aeda1b8c needs some enhancement.
> 
> 1. Shutdown event is delivired on reboot too and we don't want
> to set willhangup flag is this case.
> 
> 2. There is a next race condition.
> 
>  - EOF is delivered in event loop thread
>  - qemuMonitorSend is called on client behalf and waits for notification
>    on message response or monitor close
>  - EOF handler tries to accquire job condition and fail on timeout as
>    it is grabbed by the request above
> 
>   Now qemuMonitorSend hangs.
> 
> Previously if qemuMonitorSend is called after EOF then it failed
> immediately due to lastError is set. Now we need to check willhangup too.
> 
> ---
> 
> Race is easy to trigger with patch [1]. One need to query domain
> frequently enough in a loop and do a shutdown.
> 
> [1] Patch to trigger race condition.
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b782451..4445f88 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4742,6 +4742,9 @@ static void qemuProcessEventHandler(void *data, void *opaque)
> 
>      VIR_DEBUG("vm=%p, event=%d", vm, processEvent->eventType);
> 
> +    if (processEvent->eventType == QEMU_PROCESS_EVENT_MONITOR_EOF)
> +        sleep(3);
> +
>      virObjectLock(vm);
> 
>      switch (processEvent->eventType) {
> 
> 
> 
> 
>  src/qemu/qemu_monitor.c | 16 +++++++++++++++-
>  src/qemu/qemu_monitor.h |  2 ++
>  src/qemu/qemu_process.c |  1 +
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 19082d8..6270191 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -315,6 +315,15 @@ qemuMonitorEscapeNonPrintable(const char *text)
>  #endif
>  
>  
> +void
> +qemuMonitorShutdown(qemuMonitorPtr mon)
> +{
> +    virObjectLock(mon);
> +    mon->willhangup = 1;
> +    virObjectUnlock(mon);
> +}
> +
> +
>  static void
>  qemuMonitorDispose(void *obj)
>  {
> @@ -1055,6 +1064,12 @@ qemuMonitorSend(qemuMonitorPtr mon,
>          return -1;
>      }
>  
> +    if (mon->willhangup) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("Domain is shutting down."));
> +        return -1;
> +    }
> +
>      mon->msg = msg;
>      qemuMonitorUpdateWatch(mon);
>  
> @@ -1336,7 +1351,6 @@ qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest)
>  {
>      int ret = -1;
>      VIR_DEBUG("mon=%p guest=%u", mon, guest);
> -    mon->willhangup = 1;
>  
>      QEMU_MONITOR_CALLBACK(mon, ret, domainShutdown, mon->vm, guest);
>      return ret;
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 9805a33..30533ef 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -303,6 +303,8 @@ void qemuMonitorUnregister(qemuMonitorPtr mon)
>      ATTRIBUTE_NONNULL(1);
>  void qemuMonitorClose(qemuMonitorPtr mon);
>  
> +void qemuMonitorShutdown(qemuMonitorPtr mon);
> +
>  virErrorPtr qemuMonitorLastError(qemuMonitorPtr mon);
>  
>  int qemuMonitorSetCapabilities(qemuMonitorPtr mon);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ab81d65..824be86 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -618,6 +618,7 @@ qemuProcessShutdownOrReboot(virQEMUDriverPtr driver,
>              virObjectUnref(vm);
>          }
>      } else {
> +        qemuMonitorShutdown(priv->mon);
>          ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT));
>      }
>  }
>