https://bugzilla.redhat.com/show_bug.cgi?id=1476866 For some reason, we completely ignore <on_reboot/> setting for domains. The implementation is simply not there. It never was. However, things are slightly more complicated. QEMU sends us two RESET events on domain reboot. Fortunately, the event contains this 'guest' field telling us who initiated the reboot. And since we don't want to destroy the domain if the reset is initiated by a user, we have to ignore those events. Whatever, just look at the code. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 8 +++++++- src/qemu/qemu_process.c | 34 ++++++++++++++++++++++++++++++---- 5 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4c9050aff..d865e67c7 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -233,6 +233,7 @@ struct _qemuDomainObjPrivate { bool agentError; bool gotShutdown; + bool gotReset; bool beingDestroyed; char *pidfile; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 19082d8bf..8f81a2b28 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1344,12 +1344,12 @@ qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest) int -qemuMonitorEmitReset(qemuMonitorPtr mon) +qemuMonitorEmitReset(qemuMonitorPtr mon, virTristateBool guest) { int ret = -1; VIR_DEBUG("mon=%p", mon); - QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm); + QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm, guest); return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 31f7e97ba..8c33f6783 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -134,6 +134,7 @@ typedef int (*qemuMonitorDomainShutdownCallback)(qemuMonitorPtr mon, void *opaque); typedef int (*qemuMonitorDomainResetCallback)(qemuMonitorPtr mon, virDomainObjPtr vm, + virTristateBool guest, void *opaque); typedef int (*qemuMonitorDomainPowerdownCallback)(qemuMonitorPtr mon, virDomainObjPtr vm, @@ -346,7 +347,7 @@ int qemuMonitorEmitEvent(qemuMonitorPtr mon, const char *event, long long seconds, unsigned int micros, const char *details); int qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest); -int qemuMonitorEmitReset(qemuMonitorPtr mon); +int qemuMonitorEmitReset(qemuMonitorPtr mon, virTristateBool guest); int qemuMonitorEmitPowerdown(qemuMonitorPtr mon); int qemuMonitorEmitStop(qemuMonitorPtr mon); int qemuMonitorEmitResume(qemuMonitorPtr mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index b8a68154a..8a1501ced 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -536,7 +536,13 @@ static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, virJSONValuePtr da static void qemuMonitorJSONHandleReset(qemuMonitorPtr mon, virJSONValuePtr data ATTRIBUTE_UNUSED) { - qemuMonitorEmitReset(mon); + bool guest = false; + virTristateBool guest_initiated = VIR_TRISTATE_BOOL_ABSENT; + + if (data && virJSONValueObjectGetBoolean(data, "guest", &guest) == 0) + guest_initiated = guest ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + + qemuMonitorEmitReset(mon, guest_initiated); } static void qemuMonitorJSONHandlePowerdown(qemuMonitorPtr mon, virJSONValuePtr data ATTRIBUTE_UNUSED) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0aecce3b1..889efc7f0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -478,27 +478,51 @@ qemuProcessFindVolumeQcowPassphrase(qemuMonitorPtr mon ATTRIBUTE_UNUSED, static int qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm, + virTristateBool guest_initiated, void *opaque) { virQEMUDriverPtr driver = opaque; - virObjectEventPtr event; + virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + bool callOnReboot = false; virObjectLock(vm); + priv = vm->privateData; + + /* This is a bit tricky. When a guest does 'reboot' we receive RESET event + * twice, both times it's guest initiated. However, if users call 'virsh + * reset' we still receive two events but the first one is guest_initiated + * = no, the second one is guest_initiated = yes. Therefore, to avoid + * executing onReboot action in the latter case we need this complicated + * construction. */

+ if (guest_initiated == VIR_TRISTATE_BOOL_NO) { + VIR_DEBUG("Ignoring not guest initiated RESET event from domain %s", + vm->def->name); + priv->gotReset = true; + } else if (priv->gotReset && guest_initiated == VIR_TRISTATE_BOOL_YES) { + VIR_DEBUG("Ignoring second RESET event from domain %s", + vm->def->name); + priv->gotReset = false; + } else { + callOnReboot = true;

+ } + event = virDomainEventRebootNewFromObj(vm); - priv = vm->privateData; if (priv->agent) qemuAgentNotifyEvent(priv->agent, QEMU_AGENT_EVENT_RESET); if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) VIR_WARN("Failed to save status on vm %s", vm->def->name); + if (callOnReboot && + guest_initiated == VIR_TRISTATE_BOOL_YES &&

+ vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY) + qemuProcessShutdownOrReboot(driver, vm); +

virObjectUnlock(vm); - qemuDomainEventQueue(driver, event); -

virObjectUnref(cfg); return 0; } @@ -555,6 +579,7 @@ qemuProcessFakeReboot(void *opaque) goto endjob; } priv->gotShutdown = false; + priv->gotReset = false; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); @@ -5320,6 +5345,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, priv->monError = false; priv->monStart = 0; priv->gotShutdown = false; + priv->gotReset = false; VIR_DEBUG("Updating guest CPU definition"); if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0) -- 2.13.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 08/10/2017 04:02 PM, Martin Kletzander wrote: > On Thu, Aug 03, 2017 at 09:36:27AM +0200, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1476866 >> >> For some reason, we completely ignore <on_reboot/> setting for >> domains. The implementation is simply not there. It never was. >> However, things are slightly more complicated. QEMU sends us two >> RESET events on domain reboot. Fortunately, the event contains >> this 'guest' field telling us who initiated the reboot. And since >> we don't want to destroy the domain if the reset is initiated by >> a user, we have to ignore those events. Whatever, just look at >> the code. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_domain.h | 1 + >> src/qemu/qemu_monitor.c | 4 ++-- >> src/qemu/qemu_monitor.h | 3 ++- >> src/qemu/qemu_monitor_json.c | 8 +++++++- >> src/qemu/qemu_process.c | 34 ++++++++++++++++++++++++++++++---- >> 5 files changed, 42 insertions(+), 8 deletions(-) >> >> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h >> index 4c9050aff..d865e67c7 100644 >> --- a/src/qemu/qemu_domain.h >> +++ b/src/qemu/qemu_domain.h >> @@ -233,6 +233,7 @@ struct _qemuDomainObjPrivate { >> bool agentError; >> >> bool gotShutdown; >> + bool gotReset; >> bool beingDestroyed; >> char *pidfile; >> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index 19082d8bf..8f81a2b28 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -1344,12 +1344,12 @@ qemuMonitorEmitShutdown(qemuMonitorPtr mon, >> virTristateBool guest) >> >> >> int >> -qemuMonitorEmitReset(qemuMonitorPtr mon) >> +qemuMonitorEmitReset(qemuMonitorPtr mon, virTristateBool guest) >> { >> int ret = -1; >> VIR_DEBUG("mon=%p", mon); >> >> - QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm); >> + QEMU_MONITOR_CALLBACK(mon, ret, domainReset, mon->vm, guest); >> return ret; >> } >> >> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >> index 31f7e97ba..8c33f6783 100644 >> --- a/src/qemu/qemu_monitor.h >> +++ b/src/qemu/qemu_monitor.h >> @@ -134,6 +134,7 @@ typedef int >> (*qemuMonitorDomainShutdownCallback)(qemuMonitorPtr mon, >> void *opaque); >> typedef int (*qemuMonitorDomainResetCallback)(qemuMonitorPtr mon, >> virDomainObjPtr vm, >> + virTristateBool guest, >> void *opaque); >> typedef int (*qemuMonitorDomainPowerdownCallback)(qemuMonitorPtr mon, >> virDomainObjPtr vm, >> @@ -346,7 +347,7 @@ int qemuMonitorEmitEvent(qemuMonitorPtr mon, const >> char *event, >> long long seconds, unsigned int micros, >> const char *details); >> int qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest); >> -int qemuMonitorEmitReset(qemuMonitorPtr mon); >> +int qemuMonitorEmitReset(qemuMonitorPtr mon, virTristateBool guest); >> int qemuMonitorEmitPowerdown(qemuMonitorPtr mon); >> int qemuMonitorEmitStop(qemuMonitorPtr mon); >> int qemuMonitorEmitResume(qemuMonitorPtr mon); >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c >> index b8a68154a..8a1501ced 100644 >> --- a/src/qemu/qemu_monitor_json.c >> +++ b/src/qemu/qemu_monitor_json.c >> @@ -536,7 +536,13 @@ static void >> qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, virJSONValuePtr da >> >> static void qemuMonitorJSONHandleReset(qemuMonitorPtr mon, >> virJSONValuePtr data ATTRIBUTE_UNUSED) >> { >> - qemuMonitorEmitReset(mon); >> + bool guest = false; >> + virTristateBool guest_initiated = VIR_TRISTATE_BOOL_ABSENT; >> + >> + if (data && virJSONValueObjectGetBoolean(data, "guest", &guest) >> == 0) >> + guest_initiated = guest ? VIR_TRISTATE_BOOL_YES : >> VIR_TRISTATE_BOOL_NO; >> + >> + qemuMonitorEmitReset(mon, guest_initiated); >> } >> >> static void qemuMonitorJSONHandlePowerdown(qemuMonitorPtr mon, >> virJSONValuePtr data ATTRIBUTE_UNUSED) >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index 0aecce3b1..889efc7f0 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -478,27 +478,51 @@ >> qemuProcessFindVolumeQcowPassphrase(qemuMonitorPtr mon ATTRIBUTE_UNUSED, >> static int >> qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED, >> virDomainObjPtr vm, >> + virTristateBool guest_initiated, >> void *opaque) >> { >> virQEMUDriverPtr driver = opaque; >> - virObjectEventPtr event; >> + virObjectEventPtr event = NULL; >> qemuDomainObjPrivatePtr priv; >> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); >> + bool callOnReboot = false; >> >> virObjectLock(vm); >> >> + priv = vm->privateData; >> + >> + /* This is a bit tricky. When a guest does 'reboot' we receive >> RESET event >> + * twice, both times it's guest initiated. However, if users call >> 'virsh >> + * reset' we still receive two events but the first one is >> guest_initiated >> + * = no, the second one is guest_initiated = yes. Therefore, to >> avoid >> + * executing onReboot action in the latter case we need this >> complicated >> + * construction. */ > > I think there is a bug in qemu if calling reset gets us one > guest-initiated reset. You are not guaranteed to get two events anyway, > I believe. I think it's Seabios that issues the second reset. Therefore I don't think it's a bug. But the truth is I completely forgot about UEFI guests. > > Anyway, let's say you're right (for now), I think the following logic is > flawed a bit. > >> + if (guest_initiated == VIR_TRISTATE_BOOL_NO) { >> + VIR_DEBUG("Ignoring not guest initiated RESET event from >> domain %s", >> + vm->def->name); >> + priv->gotReset = true; >> + } else if (priv->gotReset && guest_initiated == >> VIR_TRISTATE_BOOL_YES) { >> + VIR_DEBUG("Ignoring second RESET event from domain %s", >> + vm->def->name); >> + priv->gotReset = false; >> + } else { >> + callOnReboot = true; > > This will be set either if guest_initiated == VIR_TRISTATE_BOOL_ABSENT > (keep in mind this will always be the case with older QEMUs) or > priv->gotReset == false && guest_initiated == VIR_TRISTATE_BOOL_YES. > > If we walk through your examples (reboot => guest_initiated = [yes, > yes], reset => guest_initiated = [no, yes]), then: > > reboot: > - first event (guest_initiated = yes) => callOnReboot = true; > - second event (guest_initiated = yes) => callOnReboot = true; > ( because priv->gotReset is still false ) > > reset: > - first event (guest_initiated = no) => priv->gotReset = true; > - second event (guest_initiated = yes) => priv->gotReset = false; > > So basically in the first scenario you only set the bool to true and in > the second one nothing is set ... True, 'reboot' ran from within the guest sets callOnReboot; 'virsh reset' does not. > >> + } >> + >> event = virDomainEventRebootNewFromObj(vm); >> - priv = vm->privateData; >> if (priv->agent) >> qemuAgentNotifyEvent(priv->agent, QEMU_AGENT_EVENT_RESET); >> >> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, >> driver->caps) < 0) >> VIR_WARN("Failed to save status on vm %s", vm->def->name); >> >> + if (callOnReboot && >> + guest_initiated == VIR_TRISTATE_BOOL_YES && > > ... so either this will be never executed or I missed something. Therefore, just 'reboot' has an option to fire the action. But since I completely forgot about UEFI, maybe we don't want this logic after all. I mean, how can we safely assume that whatever BIOS guest uses is going to issue the reset event? We can not! So this logic *is* flawed after all but for a different reason. So I guess the only thing we can do here is: a) throw this logic away, b) call whatever action configured

> >> + vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY) >> + qemuProcessShutdownOrReboot(driver, vm); >> + > > You should also check VIR_DOMAIN_LIFECYCLE_PRESERVE according to the > documentation: Shoot! You're right. > > ... The preserve action for an on_reboot event is treated as a destroy ... > >> virObjectUnlock(vm); >> - >> qemuDomainEventQueue(driver, event); >> - > > Spurious whitespace changes, feel free to keep them if was intended. Yeah, I'd like to keep them in. It's unnecessary to have them in a separate patch since they are trivial and I'm touching the area anyway.

Michal