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