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

2017-09-26 Thread 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));
>  }
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-09-08 Thread Nikolay Shirokovskiy 
Ouch, I don't escape test patch [1] in message body so
is is applied together with the main patch. Be careful.

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));
>  }
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-09-08 Thread Nikolay Shirokovskiy 
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));
 }
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list