[PATCH 00/22] qemu: Fix mess with lifecycle actions and unbreak transient disks
While investigating how to fix issue with transient disks breaking when -no-shutdown is not used I've ended up figuring out that lifecycle action handling in the qemu driver is very broken. Unbreak the handling by rejecting some actions which were never implemented, add support for 'set-action' qmp command and use it to update the 'reset' action in qemu and always use '-no-shutdown'. Peter Krempa (22): qemuMonitorJSONSetWatchdogAction: Use automatic memory clearing qemuDomainSetLifecycleAction: Add a note about argument range-check qemu: driver: Use 'qemuDomainSaveStatus' for saving status XML qemu: validate: Reformat header and purge unused includes qemu: Reject 'rename-restart' action for 'on_reboot'/'on_poweroff'/'on_crash' qemu: driver: Validate lifecycle actions in 'qemuDomainSetLifecycleAction' qemu: Reject 'preserve' action for 'on_reboot'/'on_poweroff'/'on_crash' qemu: Honor 'restart' action for 'on_poweroff' qemu: capablities: Detect presence of 'set-action' as QEMU_CAPS_SET_ACTION qemu: monitor: Implement monitor code for 'set-action' command qemuDomainAttachWatchdog: Use 'set-action' instead of 'watchdog-set-action' if supported qemuxml2argvtest: Add 'LATEST' version of 'misc-no-reboot' test case qemuDomainObjPrivate: Annotate 'allowReboot' field qemu: migration: Don't transfer 'allowReboot' flag qemu: domain: Remove qemuDomainIsUsingNoShutdown qemuDomainSetLifecycleAction: Forbid live update of 'on_reboot' qemuProcessHandleReset: Don't emulate lifecycle actions for RESET event qemuProcessLaunch: Setup handling of 'on_reboot' via QMP when starting the process qemu: command: Always use '-no-shutdown' qemu: process: Don't set 'allowReboot' when qemu supports 'set-action' qemuDomainSetLifecycleAction: Properly update 'onReboot' action in qemu qemu: process: Ignore 'RESET' event during startup docs/formatdomain.rst | 8 +- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 14 +- src/qemu/qemu_domain.c | 16 -- src/qemu/qemu_domain.h | 11 +- src/qemu/qemu_driver.c | 201 +++++++++++------- src/qemu/qemu_hotplug.c | 54 ++++- src/qemu/qemu_migration.c | 5 - src/qemu/qemu_migration_cookie.c | 22 -- src/qemu/qemu_migration_cookie.h | 4 - src/qemu/qemu_monitor.c | 16 ++ src/qemu/qemu_monitor.h | 49 +++++ src/qemu/qemu_monitor_json.c | 95 ++++++++- src/qemu/qemu_monitor_json.h | 7 + src/qemu/qemu_process.c | 85 +++++--- src/qemu/qemu_validate.c | 51 +++++ src/qemu/qemu_validate.h | 31 +-- .../caps_6.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../basic-xml2xml-out.xml | 1 - .../full-xml2xml-out.xml | 1 - .../modern-dom-out-dest.xml | 1 - .../modern-dom-out-source.xml | 1 - .../nbd-bitmaps-xml2xml-out.xml | 1 - tests/qemumonitorjsontest.c | 6 + .../misc-no-reboot.x86_64-latest.args | 35 +++ tests/qemuxml2argvtest.c | 1 + 30 files changed, 529 insertions(+), 194 deletions(-) create mode 100644 tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8e5af9f79a..8fb2bf4dc3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8560,9 +8560,8 @@ int qemuMonitorJSONSetWatchdogAction(qemuMonitor *mon, const char *action) { - virJSONValue *cmd; - virJSONValue *reply = NULL; - int ret = -1; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("watchdog-set-action", "s:action", action, @@ -8570,17 +8569,12 @@ qemuMonitorJSONSetWatchdogAction(qemuMonitor *mon, return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.31.1
The public API wrapper range-checks the arguments. Save the next reader the hassle of looking it up. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 444e9e5cbc..2de17aaa4e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19654,6 +19654,8 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, virDomainDef *persistentDef = NULL; int ret = -1; + /* note that 'action' and 'type' are range-checked in the public API wrapper */ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); -- 2.31.1
We've got multiple random open-coded versions. Switch to the helper function which doesn't report errors as they'd be mostly wrong as the operation was indeed successful. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 96 ++++++++++++------------------------------ 1 file changed, 26 insertions(+), 70 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2de17aaa4e..39347f84c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1869,8 +1869,7 @@ static int qemuDomainSuspend(virDomainPtr dom) if (qemuProcessStopCPUs(driver, vm, reason, QEMU_ASYNC_JOB_NONE) < 0) goto endjob; } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); ret = 0; endjob: @@ -1927,8 +1926,7 @@ static int qemuDomainResume(virDomainPtr dom) goto endjob; } } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); ret = 0; endjob: @@ -2522,8 +2520,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, } def->memballoon->period = period; - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (persistentDef) { @@ -3623,7 +3620,6 @@ processGuestPanicEvent(virQEMUDriver *driver, { qemuDomainObjPrivate *priv = vm->privateData; virObjectEvent *event = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); bool removeInactive = false; unsigned long flags = VIR_DUMP_MEMORY_ONLY; @@ -3648,10 +3644,7 @@ processGuestPanicEvent(virQEMUDriver *driver, virObjectEventStateQueue(driver->domainEventState, event); - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) { - VIR_WARN("Unable to save status on vm %s after state change", - vm->def->name); - } + qemuDomainSaveStatus(vm); if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) VIR_WARN("Unable to release lease on %s", vm->def->name); @@ -3704,7 +3697,6 @@ processDeviceDeletedEvent(virQEMUDriver *driver, virDomainObj *vm, const char *devAlias) { - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virDomainDeviceDef dev; VIR_DEBUG("Removing device %s from domain %p %s", @@ -3728,9 +3720,7 @@ processDeviceDeletedEvent(virQEMUDriver *driver, goto endjob; } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - VIR_WARN("unable to save domain status after removing device %s", - devAlias); + qemuDomainSaveStatus(vm); endjob: qemuDomainObjEndJob(driver, vm); @@ -4045,7 +4035,6 @@ processSerialChangedEvent(virQEMUDriver *driver, const char *devAlias, bool connected) { - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virDomainChrDeviceState newstate; virObjectEvent *event = NULL; virDomainDeviceDef dev; @@ -4096,9 +4085,7 @@ processSerialChangedEvent(virQEMUDriver *driver, dev.data.chr->state = newstate; - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - VIR_WARN("unable to save status of domain %s after updating state of " - "channel %s", vm->def->name, devAlias); + qemuDomainSaveStatus(vm); if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0")) { if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) { @@ -4520,7 +4507,6 @@ qemuDomainPinVcpuLive(virDomainObj *vm, virDomainDef *def, int vcpu, virQEMUDriver *driver, - virQEMUDriverConfig *cfg, virBitmap *cpumap) { virBitmap *tmpmap = NULL; @@ -4572,8 +4558,7 @@ qemuDomainPinVcpuLive(virDomainObj *vm, vcpuinfo->cpumask = tmpmap; tmpmap = NULL; - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto cleanup; + qemuDomainSaveStatus(vm); if (g_snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) { @@ -4646,7 +4631,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, } if (def && - qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap) < 0) + qemuDomainPinVcpuLive(vm, def, vcpu, driver, pcpumap) < 0) goto endjob; if (persistentDef) { @@ -4787,8 +4772,7 @@ qemuDomainPinEmulator(virDomainPtr dom, virBitmapFree(def->cputune.emulatorpin); def->cputune.emulatorpin = virBitmapNewCopy(pcpumap); - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); str = virBitmapFormat(pcpumap); if (virTypedParamsAddString(&eventParams, &eventNparams, @@ -5247,8 +5231,7 @@ qemuDomainPinIOThread(virDomainPtr dom, if (virProcessSetAffinity(iothrid->thread_id, pcpumap, false) < 0) goto endjob; - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); if (g_snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN, iothread_id) < 0) { @@ -5694,8 +5677,7 @@ qemuDomainChgIOThread(virQEMUDriver *driver, } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (persistentDef) { @@ -7825,8 +7807,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObj *vm, * changed even if we failed to attach the device. For example, * a new controller may be created. */ - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - return -1; + qemuDomainSaveStatus(vm); } /* Finally, if no error until here, we can save config. */ @@ -7964,15 +7945,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, * device we're going to update. */ if ((ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force)) < 0) goto endjob; - /* - * update domain status forcibly because the domain status may be - * changed even if we failed to attach the device. For example, - * a new controller may be created. - */ - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) { - ret = -1; - goto endjob; - } + + qemuDomainSaveStatus(vm); } /* Finally, if no error until here, we can save config. */ @@ -8057,13 +8031,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver, if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; - /* - * update domain status forcibly because the domain status may be - * changed even if we failed to attach the device. For example, - * a new controller may be created. - */ - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto cleanup; + qemuDomainSaveStatus(vm); } /* Finally, if no error until here, we can save config. */ @@ -8436,9 +8404,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, if (def) { ret = virDomainCgroupSetupDomainBlkioParameters(priv->cgroup, def, params, nparams); - - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (ret < 0) goto endjob; @@ -8613,9 +8579,8 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, params, nparams) < 0) goto endjob; - if (def && - virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + if (def) + qemuDomainSaveStatus(vm); if (persistentDef && virDomainDefSave(persistentDef, driver->xmlopt, cfg->configDir) < 0) @@ -8876,8 +8841,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, -1, mode, nodeset) < 0) goto endjob; - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (persistentDef) { @@ -9068,8 +9032,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (persistentDef) { @@ -9520,8 +9483,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, } } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); if (eventNparams) { event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); @@ -10418,8 +10380,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, goto endjob; } - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (persistentNet) { @@ -14444,7 +14405,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, { virQEMUDriver *driver = dom->conn->privateData; virDomainDiskDef *disk = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); g_autoptr(qemuBlockJobData) job = NULL; @@ -14508,7 +14468,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, job->state = QEMU_BLOCKJOB_STATE_ABORTING; } - ignore_value(virDomainObjSave(vm, driver->xmlopt, cfg->stateDir)); + qemuDomainSaveStatus(vm); if (!async) { qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); @@ -16191,8 +16151,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainSetGroupBlockIoTune(def, &info); - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); if (eventNparams) { event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); @@ -19686,8 +19645,7 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, qemuDomainModifyLifecycleAction(def, type, action); - if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto endjob; + qemuDomainSaveStatus(vm); } if (persistentDef) { @@ -20225,9 +20183,7 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom, QEMU_DOMAIN_PRIVATE(vm)->agentTimeout = timeout; - if (virDomainObjIsActive(vm) && - virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) - goto cleanup; + qemuDomainSaveStatus(vm); ret = 0; -- 2.31.1
Use the new style header formatting. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_validate.h | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_validate.h b/src/qemu/qemu_validate.h index 271460d019..19629ac3af 100644 --- a/src/qemu/qemu_validate.h +++ b/src/qemu/qemu_validate.h @@ -20,19 +20,21 @@ #pragma once -#include <glib-object.h> - #include "domain_conf.h" #include "qemu_capabilities.h" -#include "qemu_conf.h" -int qemuValidateDomainDef(const virDomainDef *def, - void *opaque, - void *parseOpaque); -int qemuValidateDomainDeviceDefDisk(const virDomainDiskDef *disk, - const virDomainDef *def, - virQEMUCaps *qemuCaps); -int qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev, +int +qemuValidateDomainDef(const virDomainDef *def, + void *opaque, + void *parseOpaque); + +int +qemuValidateDomainDeviceDefDisk(const virDomainDiskDef *disk, const virDomainDef *def, - void *opaque, - void *parseOpaque); + virQEMUCaps *qemuCaps); + +int +qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev, + const virDomainDef *def, + void *opaque, + void *parseOpaque); -- 2.31.1
The qemu driver didn't ever implement any meaningful handling for the 'rename-restart' action. At this point the following handling would take place: 'on_reboot' set to 'rename-restart' is ignored on guest-initiated reboots, the guest simply reboots. For on_poweroff set to 'rename-restart' the following happens: guest initiated shutdown -> 'destroy' libvirt initiated shutdown -> 'reboot' In addition when 'on_reboot' is 'destroy' in addition to 'on_poweroff' being 'rename-restart' the guest is able to execute instructions after issuing a reset before libvirt terminates it. This will be addressed separately later. Forbid the flag in the qemu def validator and update the documentation to be factual. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 6 +++--- src/qemu/qemu_validate.c | 32 ++++++++++++++++++++++++++++++++ src/qemu/qemu_validate.h | 5 +++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 512e6abd27..fba4765e35 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1743,12 +1743,12 @@ Each of these states allow for the same four possible actions. ``preserve`` The domain will be terminated and its resource preserved to allow analysis. ``rename-restart`` - The domain will be terminated and then restarted with a new name. + The domain will be terminated and then restarted with a new name. (Only + supported by the libxl hypervisor driver.) QEMU/KVM supports the ``on_poweroff`` and ``on_reboot`` events handling the ``destroy`` and ``restart`` actions. The ``preserve`` action for an -``on_reboot`` event is treated as a ``destroy`` and the ``rename-restart`` -action for an ``on_poweroff`` event is treated as a ``restart`` event. +``on_reboot`` event is treated as a ``destroy``. The ``on_crash`` event supports these additional actions :since:`since 0.8.4` . diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index e5c4e3af26..7bd8d8d9e7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1063,6 +1063,35 @@ qemuValidateDomainDeviceInfo(virDomainDef *def G_GNUC_UNUSED, return 0; } + +int +qemuValidateLifecycleAction(virDomainLifecycleAction onPoweroff, + virDomainLifecycleAction onReboot, + virDomainLifecycleAction onCrash) +{ + /* The qemu driver doesn't yet implement any meaningful handling for + * VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME */ + if (onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME || + onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME || + onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("qemu driver doesn't support the 'rename-restart' action for 'on_reboot'/'on_poweroff'/'on_crash'")); + return -1; + } + + return 0; +} + + +static int +qemuValidateDomainLifecycleAction(const virDomainDef *def) +{ + return qemuValidateLifecycleAction(def->onPoweroff, + def->onReboot, + def->onCrash); +} + + int qemuValidateDomainDef(const virDomainDef *def, void *opaque, @@ -1157,6 +1186,9 @@ qemuValidateDomainDef(const virDomainDef *def, } } + if (qemuValidateDomainLifecycleAction(def) < 0) + return -1; + if (qemuValidateDomainDefCpu(driver, def, qemuCaps) < 0) return -1; diff --git a/src/qemu/qemu_validate.h b/src/qemu/qemu_validate.h index 19629ac3af..1e0546633c 100644 --- a/src/qemu/qemu_validate.h +++ b/src/qemu/qemu_validate.h @@ -38,3 +38,8 @@ qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev, const virDomainDef *def, void *opaque, void *parseOpaque); + +int +qemuValidateLifecycleAction(virDomainLifecycleAction onPoweroff, + virDomainLifecycleAction onReboot, + virDomainLifecycleAction onCrash); -- 2.31.1
Some actions are not supported by qemu. Use the recently added 'qemuValidateLifecycleAction' helper to ensure that the API does the same validation as we do on startup in the validation callbacks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39347f84c7..0472f54a37 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -53,6 +53,7 @@ #include "qemu_namespace.h" #include "qemu_saveimage.h" #include "qemu_snapshot.h" +#include "qemu_validate.h" #include "virerror.h" #include "virlog.h" @@ -19577,6 +19578,36 @@ qemuDomainSetBlockThreshold(virDomainPtr dom, } +static int +qemuDomainSetLifecycleActionValidate(virDomainDef *def, + virDomainLifecycle type, + virDomainLifecycleAction action) +{ + virDomainLifecycleAction onPoweroff = def->onPoweroff; + virDomainLifecycleAction onReboot = def->onReboot; + virDomainLifecycleAction onCrash = def->onCrash; + + switch (type) { + case VIR_DOMAIN_LIFECYCLE_POWEROFF: + onPoweroff = action; + break; + case VIR_DOMAIN_LIFECYCLE_REBOOT: + onReboot = action; + break; + case VIR_DOMAIN_LIFECYCLE_CRASH: + onCrash = action; + break; + case VIR_DOMAIN_LIFECYCLE_LAST: + break; + } + + if (qemuValidateLifecycleAction(onPoweroff, onReboot, onCrash) < 0) + return -1; + + return 0; +} + + static void qemuDomainModifyLifecycleAction(virDomainDef *def, virDomainLifecycle type, @@ -19635,6 +19666,10 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; + if ((def && qemuDomainSetLifecycleActionValidate(def, type, action) < 0) || + (persistentDef && qemuDomainSetLifecycleActionValidate(persistentDef, type, action) < 0)) + goto endjob; + if (def) { if (priv->allowReboot == VIR_TRISTATE_BOOL_NO) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", -- 2.31.1
The qemu driver didn't ever implement any meaningful handling for the 'preserve' action. Forbid the flag in the qemu def validator and update the documentation to be factual. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 3 +-- src/qemu/qemu_validate.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index fba4765e35..a894a51c00 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1747,8 +1747,7 @@ Each of these states allow for the same four possible actions. supported by the libxl hypervisor driver.) QEMU/KVM supports the ``on_poweroff`` and ``on_reboot`` events handling the -``destroy`` and ``restart`` actions. The ``preserve`` action for an -``on_reboot`` event is treated as a ``destroy``. +``destroy`` and ``restart`` actions. The ``on_crash`` event supports these additional actions :since:`since 0.8.4` . diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 7bd8d8d9e7..d27c3b6c6c 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1079,6 +1079,16 @@ qemuValidateLifecycleAction(virDomainLifecycleAction onPoweroff, return -1; } + /* The qemu driver doesn't yet implement any meaningful handling for + * VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE */ + if (onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE || + onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE || + onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("qemu driver doesn't support the 'preserve' action for 'on_reboot'/'on_poweroff'/'on_crash'")); + return -1; + } + return 0; } -- 2.31.1
On 24.08.21 16:44, Peter Krempa wrote:
The qemu driver didn't ever implement any meaningful handling for the 'preserve' action.
Forbid the flag in the qemu def validator and update the documentation to be factual.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
NACK. It is a perfectly sane usecase to have "preserve" on a crash. The qemu will simply sit in that state and the user can then do virsh dump or similar.
--- docs/formatdomain.rst | 3 +-- src/qemu/qemu_validate.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index fba4765e35..a894a51c00 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1747,8 +1747,7 @@ Each of these states allow for the same four possible actions. supported by the libxl hypervisor driver.)
QEMU/KVM supports the ``on_poweroff`` and ``on_reboot`` events handling the -``destroy`` and ``restart`` actions. The ``preserve`` action for an -``on_reboot`` event is treated as a ``destroy``. +``destroy`` and ``restart`` actions.
The ``on_crash`` event supports these additional actions :since:`since 0.8.4` .
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 7bd8d8d9e7..d27c3b6c6c 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1079,6 +1079,16 @@ qemuValidateLifecycleAction(virDomainLifecycleAction onPoweroff, return -1; }
+ /* The qemu driver doesn't yet implement any meaningful handling for + * VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE */ + if (onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE || + onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE || + onCrash == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("qemu driver doesn't support the 'preserve' action for 'on_reboot'/'on_poweroff'/'on_crash'")); + return -1; + } + return 0; }
On 15.09.21 11:45, Christian Borntraeger wrote:
On 24.08.21 16:44, Peter Krempa wrote:
The qemu driver didn't ever implement any meaningful handling for the 'preserve' action.
Forbid the flag in the qemu def validator and update the documentation to be factual.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
NACK. It is a perfectly sane usecase to have "preserve" on a crash. The qemu will simply sit in that state and the user can then do virsh dump or similar.
To make this statement stronger. I use this A LOT in real life to force libvirt to leave the QEMU in that state to get dump and debug info.
On Wed, Sep 15, 2021 at 11:47:04AM +0200, Christian Borntraeger wrote:
On 15.09.21 11:45, Christian Borntraeger wrote:
On 24.08.21 16:44, Peter Krempa wrote:
The qemu driver didn't ever implement any meaningful handling for the 'preserve' action.
Forbid the flag in the qemu def validator and update the documentation to be factual.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
NACK. It is a perfectly sane usecase to have "preserve" on a crash. The qemu will simply sit in that state and the user can then do virsh dump or similar.
To make this statement stronger. I use this A LOT in real life to force libvirt to leave the QEMU in that state to get dump and debug info.
We probably just need to add a commentin libvirt source somewhere relevant to the effect that "preserve" is the default behaviour from QEMU so libvirt doesn't need todo anything extra. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Sep 15, 2021 at 10:57:23 +0100, Daniel P. Berrangé wrote:
On Wed, Sep 15, 2021 at 11:47:04AM +0200, Christian Borntraeger wrote:
On 15.09.21 11:45, Christian Borntraeger wrote:
On 24.08.21 16:44, Peter Krempa wrote:
The qemu driver didn't ever implement any meaningful handling for the 'preserve' action.
Forbid the flag in the qemu def validator and update the documentation to be factual.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
NACK. It is a perfectly sane usecase to have "preserve" on a crash. The qemu will simply sit in that state and the user can then do virsh dump or similar.
To make this statement stronger. I use this A LOT in real life to force libvirt to leave the QEMU in that state to get dump and debug info.
We probably just need to add a commentin libvirt source somewhere relevant to the effect that "preserve" is the default behaviour from QEMU so libvirt doesn't need todo anything extra.
Hmm, yeah for 'onCrash' it actually might make sense. Unfortunately it was totally broken for onReboot and onShutdown. I'll remove the check for onCrash, but for the other two I'll leave it in as it didn't work at all and I don't really care enough to implement it.
We simply terminate qemu instead of issuing a reset as the semantics of the setting dictate. Fix it by handling it identically to 'fake reboot'. We need to forbid the combination of 'onReboot' -> 'destroy' and 'onPoweroff' -> reboot though as the handling would be hairy and it honetly makes no sense. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 3 ++- src/qemu/qemu_process.c | 6 ++++-- src/qemu/qemu_validate.c | 9 +++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index a894a51c00..1f3136b3f2 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1747,7 +1747,8 @@ Each of these states allow for the same four possible actions. supported by the libxl hypervisor driver.) QEMU/KVM supports the ``on_poweroff`` and ``on_reboot`` events handling the -``destroy`` and ``restart`` actions. +``destroy`` and ``restart`` actions, but the combiatnion of ``on_poweroff`` set +to ``restart`` and ``on_reboot`` set to ``destroy`` is forbidden. The ``on_crash`` event supports these additional actions :since:`since 0.8.4` . diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 77da9992f4..5958a48182 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -543,7 +543,8 @@ qemuProcessShutdownOrReboot(virQEMUDriver *driver, { qemuDomainObjPrivate *priv = vm->privateData; - if (priv->fakeReboot) { + if (priv->fakeReboot || + vm->def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_RESTART) { g_autofree char *name = g_strdup_printf("reboot-%s", vm->def->name); virThread th; @@ -619,7 +620,8 @@ qemuProcessHandleShutdown(qemuMonitor *mon G_GNUC_UNUSED, /* In case of fake reboot qemu shutdown state is transient so don't * change domain state nor send events. */ - if (!priv->fakeReboot) { + if (!priv->fakeReboot || + vm->def->onPoweroff != VIR_DOMAIN_LIFECYCLE_ACTION_RESTART) { VIR_DEBUG("Transitioned guest %s to shutdown state", vm->def->name); virDomainObjSetState(vm, diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index d27c3b6c6c..8906aa52d9 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1089,6 +1089,15 @@ qemuValidateLifecycleAction(virDomainLifecycleAction onPoweroff, return -1; } + /* the qemu driver can't meaningfully handle + * onPoweroff -> reboot + onReboot -> destroy */ + if (onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_RESTART && + onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("qemu driver doesn't support 'onReboot' set to 'destroy and 'onPoweroff' set to 'reboot'")); + return -1; + } + return 0; } -- 2.31.1
The 'set-action' QMP command allows modifying the behaviour when the guest resets. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 + 6 files changed, 7 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 286d34ae54..70c3ec2f0c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -637,6 +637,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "confidential-guest-support", /* QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT */ "query-display-options", /* QEMU_CAPS_QUERY_DISPLAY_OPTIONS */ "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */ + "set-action", /* QEMU_CAPS_SET_ACTION */ ); @@ -1180,6 +1181,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-display-options", QEMU_CAPS_QUERY_DISPLAY_OPTIONS }, { "blockdev-reopen", QEMU_CAPS_BLOCKDEV_REOPEN }, { "set-numa-node", QEMU_CAPS_NUMA }, + { "set-action", QEMU_CAPS_SET_ACTION }, }; struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 17fe91dae9..bc762d1916 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -617,6 +617,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT, /* -machine confidential-guest-support */ QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp command present */ QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */ + QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml index c65884deca..dcc41ed067 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml @@ -210,6 +210,7 @@ <flag name='input-linux'/> <flag name='confidential-guest-support'/> <flag name='query-display-options'/> + <flag name='set-action'/> <version>6000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml index 7124e5194a..ebcca6e114 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml @@ -164,6 +164,7 @@ <flag name='confidential-guest-support'/> <flag name='query-display-options'/> <flag name='s390-pv-guest'/> + <flag name='set-action'/> <version>6000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml index 7e06d93c0f..4951644354 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -254,6 +254,7 @@ <flag name='input-linux'/> <flag name='confidential-guest-support'/> <flag name='query-display-options'/> + <flag name='set-action'/> <version>6000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml index 1fad52ba90..1ba344ea1c 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -256,6 +256,7 @@ <flag name='virtio-vga-gl'/> <flag name='confidential-guest-support'/> <flag name='query-display-options'/> + <flag name='set-action'/> <version>6000090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 16 ++++++++ src/qemu/qemu_monitor.h | 49 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.c | 79 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 7 ++++ tests/qemumonitorjsontest.c | 6 +++ 5 files changed, 157 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 14fb605e92..42846349c4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4609,3 +4609,19 @@ qemuMonitorQueryDirtyRate(qemuMonitor *mon, return qemuMonitorJSONQueryDirtyRate(mon, info); } + + +int +qemuMonitorSetAction(qemuMonitor *mon, + qemuMonitorActionShutdown shutdown, + qemuMonitorActionReboot reboot, + qemuMonitorActionWatchdog watchdog, + qemuMonitorActionPanic panic) +{ + VIR_DEBUG("shutdown=%u, reboot=%u, watchdog=%u panic=%u", + shutdown, reboot, watchdog, panic); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetAction(mon, shutdown, reboot, watchdog, panic); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8af271dc96..2f08357c0c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -101,6 +101,48 @@ struct _qemuMonitorRdmaGidStatus { }; +typedef enum { + QEMU_MONITOR_ACTION_SHUTDOWN_KEEP, /* do not change the current setting */ + QEMU_MONITOR_ACTION_SHUTDOWN_POWEROFF, + QEMU_MONITOR_ACTION_SHUTDOWN_PAUSE, + + QEMU_MONITOR_ACTION_SHUTDOWN_LAST +} qemuMonitorActionShutdown; + + +typedef enum { + QEMU_MONITOR_ACTION_REBOOT_KEEP, /* do not change the current setting */ + QEMU_MONITOR_ACTION_REBOOT_RESET, + QEMU_MONITOR_ACTION_REBOOT_SHUTDOWN, + + QEMU_MONITOR_ACTION_REBOOT_LAST +} qemuMonitorActionReboot; + + +typedef enum { + QEMU_MONITOR_ACTION_WATCHDOG_KEEP, /* do not change the current setting */ + QEMU_MONITOR_ACTION_WATCHDOG_RESET, + QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN, + QEMU_MONITOR_ACTION_WATCHDOG_POWEROFF, + QEMU_MONITOR_ACTION_WATCHDOG_PAUSE, + QEMU_MONITOR_ACTION_WATCHDOG_DEBUG, + QEMU_MONITOR_ACTION_WATCHDOG_NONE, + QEMU_MONITOR_ACTION_WATCHDOG_INJECT_NMI, + + QEMU_MONITOR_ACTION_WATCHDOG_LAST +} qemuMonitorActionWatchdog; + + +typedef enum { + QEMU_MONITOR_ACTION_PANIC_KEEP, /* do not change the current setting */ + QEMU_MONITOR_ACTION_PANIC_PAUSE, + QEMU_MONITOR_ACTION_PANIC_SHUTDOWN, + QEMU_MONITOR_ACTION_PANIC_NONE, + + QEMU_MONITOR_ACTION_PANIC_LAST +} qemuMonitorActionPanic; + + typedef enum { QEMU_MONITOR_JOB_TYPE_UNKNOWN, /* internal value, not exposed by qemu */ QEMU_MONITOR_JOB_TYPE_COMMIT, @@ -1488,3 +1530,10 @@ struct _qemuMonitorDirtyRateInfo { int qemuMonitorQueryDirtyRate(qemuMonitor *mon, qemuMonitorDirtyRateInfo *info); + +int +qemuMonitorSetAction(qemuMonitor *mon, + qemuMonitorActionShutdown shutdown, + qemuMonitorActionReboot reboot, + qemuMonitorActionWatchdog watchdog, + qemuMonitorActionPanic panic); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8fb2bf4dc3..8d3c4031a6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9325,3 +9325,82 @@ qemuMonitorJSONQueryDirtyRate(qemuMonitor *mon, return qemuMonitorJSONExtractDirtyRateInfo(data, info); } + + +VIR_ENUM_DECL(qemuMonitorActionShutdown); +VIR_ENUM_IMPL(qemuMonitorActionShutdown, + QEMU_MONITOR_ACTION_SHUTDOWN_LAST, + "", + "poweroff", + "pause"); + +VIR_ENUM_DECL(qemuMonitorActionReboot); +VIR_ENUM_IMPL(qemuMonitorActionReboot, + QEMU_MONITOR_ACTION_REBOOT_LAST, + "", + "reset", + "shutdown"); + +VIR_ENUM_DECL(qemuMonitorActionWatchdog); +VIR_ENUM_IMPL(qemuMonitorActionWatchdog, + QEMU_MONITOR_ACTION_WATCHDOG_LAST, + "", + "reset", + "shutdown", + "poweroff", + "pause", + "debug", + "none", + "inject-nmi"); + +VIR_ENUM_DECL(qemuMonitorActionPanic); +VIR_ENUM_IMPL(qemuMonitorActionPanic, + QEMU_MONITOR_ACTION_PANIC_LAST, + "", + "pause", + "shutdown", + "none"); + + +int +qemuMonitorJSONSetAction(qemuMonitor *mon, + qemuMonitorActionShutdown shutdown, + qemuMonitorActionReboot reboot, + qemuMonitorActionWatchdog watchdog, + qemuMonitorActionPanic panic) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + const char *actionShutdown = NULL; + const char *actionReboot = NULL; + const char *actionWatchdog = NULL; + const char *actionPanic = NULL; + + if (shutdown != QEMU_MONITOR_ACTION_SHUTDOWN_KEEP) + actionShutdown = qemuMonitorActionShutdownTypeToString(shutdown); + + if (reboot != QEMU_MONITOR_ACTION_REBOOT_KEEP) + actionReboot = qemuMonitorActionRebootTypeToString(reboot); + + if (watchdog != QEMU_MONITOR_ACTION_WATCHDOG_KEEP) + actionWatchdog = qemuMonitorActionWatchdogTypeToString(watchdog); + + if (panic != QEMU_MONITOR_ACTION_PANIC_KEEP) + actionPanic = qemuMonitorActionPanicTypeToString(panic); + + if (!(cmd = qemuMonitorJSONMakeCommand("set-action", + "S:shutdown", actionShutdown, + "S:reboot", actionReboot, + "S:watchdog", actionWatchdog, + "S:panic", actionPanic, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index fbeab2bf6d..c8cf734a1c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -703,3 +703,10 @@ qemuMonitorJSONStartDirtyRateCalc(qemuMonitor *mon, int qemuMonitorJSONQueryDirtyRate(qemuMonitor *mon, qemuMonitorDirtyRateInfo *info); + +int +qemuMonitorJSONSetAction(qemuMonitor *mon, + qemuMonitorActionShutdown shutdown, + qemuMonitorActionReboot reboot, + qemuMonitorActionWatchdog watchdog, + qemuMonitorActionPanic panic); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 6d7ecb0ab1..9ec5f06981 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1204,6 +1204,11 @@ GEN_TEST_FUNC(qemuMonitorJSONBitmapRemove, "foodev", "newnode") GEN_TEST_FUNC(qemuMonitorJSONJobDismiss, "jobname") GEN_TEST_FUNC(qemuMonitorJSONJobComplete, "jobname") GEN_TEST_FUNC(qemuMonitorJSONBlockJobCancel, "jobname", true) +GEN_TEST_FUNC(qemuMonitorJSONSetAction, + QEMU_MONITOR_ACTION_SHUTDOWN_PAUSE, + QEMU_MONITOR_ACTION_REBOOT_RESET, + QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN, + QEMU_MONITOR_ACTION_PANIC_SHUTDOWN) static int testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque) @@ -3067,6 +3072,7 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONJobDismiss); DO_TEST_GEN(qemuMonitorJSONJobComplete); DO_TEST_GEN(qemuMonitorJSONBlockJobCancel); + DO_TEST_GEN(qemuMonitorJSONSetAction); DO_TEST(qemuMonitorJSONGetBalloonInfo); DO_TEST(qemuMonitorJSONGetBlockInfo); DO_TEST(qemuMonitorJSONGetAllBlockStatsInfo); -- 2.31.1
If current qemu supports 'set-action' use it instead of the single-use command. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 54 +++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c3c49fe080..64580b573b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3117,8 +3117,6 @@ qemuDomainAttachWatchdog(virQEMUDriver *driver, int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_WATCHDOG, { .watchdog = watchdog } }; - virDomainWatchdogAction actualAction = watchdog->action; - const char *actionStr = NULL; g_autofree char *watchdogstr = NULL; bool releaseAddress = false; int rv; @@ -3146,17 +3144,59 @@ qemuDomainAttachWatchdog(virQEMUDriver *driver, if (!(watchdogstr = qemuBuildWatchdogDevStr(vm->def, watchdog, priv->qemuCaps))) goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); + /* QEMU doesn't have a 'dump' action; we tell qemu to 'pause', then libvirt listens for the watchdog event, and we perform the dump ourselves. so convert 'dump' to 'pause' for the qemu cli */ - if (actualAction == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) - actualAction = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { + qemuMonitorActionWatchdog watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_KEEP; - actionStr = virDomainWatchdogActionTypeToString(actualAction); + switch (watchdog->action) { + case VIR_DOMAIN_WATCHDOG_ACTION_RESET: + watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_RESET; + break; - qemuDomainObjEnterMonitor(driver, vm); + case VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN: + watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN; + break; + + case VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF: + watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_POWEROFF; + break; + + case VIR_DOMAIN_WATCHDOG_ACTION_PAUSE: + case VIR_DOMAIN_WATCHDOG_ACTION_DUMP: + watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_PAUSE; + break; + + case VIR_DOMAIN_WATCHDOG_ACTION_NONE: + watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_NONE; + break; + + case VIR_DOMAIN_WATCHDOG_ACTION_INJECTNMI: + watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_INJECT_NMI; + break; + + case VIR_DOMAIN_WATCHDOG_ACTION_LAST: + default: + break; + }; + + rv = qemuMonitorSetAction(priv->mon, + QEMU_MONITOR_ACTION_SHUTDOWN_KEEP, + QEMU_MONITOR_ACTION_REBOOT_KEEP, + watchdogaction, + QEMU_MONITOR_ACTION_PANIC_KEEP); + } else { + virDomainWatchdogAction actualAction = watchdog->action; - rv = qemuMonitorSetWatchdogAction(priv->mon, actionStr); + if (actualAction == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) + actualAction = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE; + + rv = qemuMonitorSetWatchdogAction(priv->mon, + virDomainWatchdogActionTypeToString(actualAction)); + } if (rv >= 0) rv = qemuMonitorAddDevice(priv->mon, watchdogstr); -- 2.31.1
Upcoming patches will modify how '-no-reboot' is handled when qemu supports the 'set-action' QMP command. Add a test for it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../misc-no-reboot.x86_64-latest.args | 35 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 2 files changed, 36 insertions(+) create mode 100644 tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args diff --git a/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args b/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args new file mode 100644 index 0000000000..197b6375a7 --- /dev/null +++ b/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args @@ -0,0 +1,35 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +/usr/bin/qemu-system-i386 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-cpu qemu64 \ +-m 214 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-reboot \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ +-audiodev id=audio1,driver=none \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 00a85988d6..95abc9d83b 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1610,6 +1610,7 @@ mymain(void) DO_TEST("misc-enable-s4", QEMU_CAPS_PIIX_DISABLE_S4); DO_TEST_PARSE_ERROR_NOCAPS("misc-enable-s4"); DO_TEST_NOCAPS("misc-no-reboot"); + DO_TEST_CAPS_LATEST("misc-no-reboot"); DO_TEST_NOCAPS("misc-uuid"); DO_TEST_PARSE_ERROR_NOCAPS("vhost_queues-invalid"); DO_TEST("net-vhostuser", QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE); -- 2.31.1
Save further readers the headache of determining what it actually does and note that it's not used with qemu version supporting the 'set-action' command. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index abc3c7158d..f3c6bb3390 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -154,6 +154,14 @@ struct _qemuDomainObjPrivate { bool fakeReboot; bool pausedShutdown; + /* allowReboot: + * + * Unused with new QEMU versions which have QEMU_CAPS_SET_ACTION. + * + * Otherwise if it's set to VIR_TRISTATE_BOOL_YES, QEMU was started with + * -no-shutdown, and if set to VIR_TRISTATE_BOOL_NO qemu was started with + * -no-reboot instead. + */ virTristateBool allowReboot; int jobs_queued; -- 2.31.1
The original idea was to ensure that the destination has the same original state of the '-no-reboot' flag to ensure identical behaviour of the 'vidDomainModifyLifecycleAction' API. With newer qemu's we'll be able to modify the behaviour using the monitor so old daemons won't be able to keep up anyways. Remove this feature as it's not very useful and will be replaced by a proper solution. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 5 ----- src/qemu/qemu_migration_cookie.c | 22 ------------------- src/qemu/qemu_migration_cookie.h | 4 ---- .../basic-xml2xml-out.xml | 1 - .../full-xml2xml-out.xml | 1 - .../modern-dom-out-dest.xml | 1 - .../modern-dom-out-source.xml | 1 - .../nbd-bitmaps-xml2xml-out.xml | 1 - 8 files changed, 36 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b441d0226c..dd226ea4bc 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2415,8 +2415,6 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, if (priv->origCPU) cookieFlags |= QEMU_MIGRATION_COOKIE_CPU; - cookieFlags |= QEMU_MIGRATION_COOKIE_ALLOW_REBOOT; - if (!(flags & VIR_MIGRATE_OFFLINE)) cookieFlags |= QEMU_MIGRATION_COOKIE_CAPS; @@ -2876,7 +2874,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG | QEMU_MIGRATION_COOKIE_CPU_HOTPLUG | QEMU_MIGRATION_COOKIE_CPU | - QEMU_MIGRATION_COOKIE_ALLOW_REBOOT | QEMU_MIGRATION_COOKIE_CAPS | QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS))) goto cleanup; @@ -2933,8 +2930,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, goto stopjob; stopProcess = true; - priv->allowReboot = mig->allowReboot; - if (!(incoming = qemuMigrationDstPrepare(vm, tunnel, protocol, listenAddress, port, dataFD[0]))) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index b67728f9c0..c7b010f0a0 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -558,18 +558,6 @@ qemuMigrationCookieAddCPU(qemuMigrationCookie *mig, } -static void -qemuMigrationCookieAddAllowReboot(qemuMigrationCookie *mig, - virDomainObj *vm) -{ - qemuDomainObjPrivate *priv = vm->privateData; - - mig->allowReboot = priv->allowReboot; - - mig->flags |= QEMU_MIGRATION_COOKIE_ALLOW_REBOOT; -} - - static int qemuMigrationCookieAddCaps(qemuMigrationCookie *mig, virDomainObj *vm, @@ -901,9 +889,6 @@ qemuMigrationCookieXMLFormat(virQEMUDriver *driver, if (mig->flags & QEMU_MIGRATION_COOKIE_CPU && mig->cpu) virCPUDefFormatBufFull(buf, mig->cpu, NULL); - if (mig->flags & QEMU_MIGRATION_COOKIE_ALLOW_REBOOT) - qemuDomainObjPrivateXMLFormatAllowReboot(buf, mig->allowReboot); - if (mig->flags & QEMU_MIGRATION_COOKIE_CAPS) qemuMigrationCookieCapsXMLFormat(buf, mig->caps); @@ -1407,10 +1392,6 @@ qemuMigrationCookieXMLParse(qemuMigrationCookie *mig, false) < 0) return -1; - if (flags & QEMU_MIGRATION_COOKIE_ALLOW_REBOOT && - qemuDomainObjPrivateXMLParseAllowReboot(ctxt, &mig->allowReboot) < 0) - return -1; - if (flags & QEMU_MIGRATION_COOKIE_CAPS && !(mig->caps = qemuMigrationCookieCapsXMLParse(ctxt))) return -1; @@ -1491,9 +1472,6 @@ qemuMigrationCookieFormat(qemuMigrationCookie *mig, qemuMigrationCookieAddCPU(mig, dom) < 0) return -1; - if (flags & QEMU_MIGRATION_COOKIE_ALLOW_REBOOT) - qemuMigrationCookieAddAllowReboot(mig, dom); - if (flags & QEMU_MIGRATION_COOKIE_CAPS && qemuMigrationCookieAddCaps(mig, dom, party) < 0) return -1; diff --git a/src/qemu/qemu_migration_cookie.h b/src/qemu/qemu_migration_cookie.h index 6ac7b2084c..1726e5f2da 100644 --- a/src/qemu/qemu_migration_cookie.h +++ b/src/qemu/qemu_migration_cookie.h @@ -52,7 +52,6 @@ typedef enum { QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG = (1 << QEMU_MIGRATION_COOKIE_FLAG_MEMORY_HOTPLUG), QEMU_MIGRATION_COOKIE_CPU_HOTPLUG = (1 << QEMU_MIGRATION_COOKIE_FLAG_CPU_HOTPLUG), QEMU_MIGRATION_COOKIE_CPU = (1 << QEMU_MIGRATION_COOKIE_FLAG_CPU), - QEMU_MIGRATION_COOKIE_ALLOW_REBOOT = (1 << QEMU_MIGRATION_COOKIE_FLAG_ALLOW_REBOOT), QEMU_MIGRATION_COOKIE_CAPS = (1 << QEMU_MIGRATION_COOKIE_FLAG_CAPS), QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS = (1 << QEMU_MIGRATION_COOKIE_FLAG_BLOCK_DIRTY_BITMAPS), } qemuMigrationCookieFeatures; @@ -168,9 +167,6 @@ struct _qemuMigrationCookie { /* If flags & QEMU_MIGRATION_COOKIE_CPU */ virCPUDef *cpu; - /* If flags & QEMU_MIGRATION_COOKIE_ALLOW_REBOOT */ - virTristateBool allowReboot; - /* If flags & QEMU_MIGRATION_COOKIE_CAPS */ qemuMigrationCookieCaps *caps; diff --git a/tests/qemumigrationcookiexmldata/basic-xml2xml-out.xml b/tests/qemumigrationcookiexmldata/basic-xml2xml-out.xml index 42b351047e..e05f3c3a83 100644 --- a/tests/qemumigrationcookiexmldata/basic-xml2xml-out.xml +++ b/tests/qemumigrationcookiexmldata/basic-xml2xml-out.xml @@ -3,7 +3,6 @@ <uuid>dcf47dbd-46d1-4d5b-b442-262a806a333a</uuid> <hostname>hostname</hostname> <hostuuid>4a802f00-4cba-5df6-9679-a08c4c5b577f</hostuuid> - <allowReboot value='default'/> <capabilities> </capabilities> </qemu-migration> diff --git a/tests/qemumigrationcookiexmldata/full-xml2xml-out.xml b/tests/qemumigrationcookiexmldata/full-xml2xml-out.xml index a07dd1f147..61fcaff13f 100644 --- a/tests/qemumigrationcookiexmldata/full-xml2xml-out.xml +++ b/tests/qemumigrationcookiexmldata/full-xml2xml-out.xml @@ -211,7 +211,6 @@ <auto_converge_throttle>14</auto_converge_throttle> </statistics> <cpu mode='host-passthrough' check='partial' migratable='on'/> - <allowReboot value='yes'/> <capabilities> <cap name='xbzrle' auto='yes'/> <cap name='postcopy-ram' auto='no'/> diff --git a/tests/qemumigrationcookiexmldata/modern-dom-out-dest.xml b/tests/qemumigrationcookiexmldata/modern-dom-out-dest.xml index ba84c65a3d..246efc06aa 100644 --- a/tests/qemumigrationcookiexmldata/modern-dom-out-dest.xml +++ b/tests/qemumigrationcookiexmldata/modern-dom-out-dest.xml @@ -6,7 +6,6 @@ <feature name='memory-hotplug'/> <feature name='cpu-hotplug'/> <graphics type='spice' port='5900' listen='127.0.0.1' tlsPort='-1'/> - <allowReboot value='yes'/> <capabilities> </capabilities> </qemu-migration> diff --git a/tests/qemumigrationcookiexmldata/modern-dom-out-source.xml b/tests/qemumigrationcookiexmldata/modern-dom-out-source.xml index ba84c65a3d..246efc06aa 100644 --- a/tests/qemumigrationcookiexmldata/modern-dom-out-source.xml +++ b/tests/qemumigrationcookiexmldata/modern-dom-out-source.xml @@ -6,7 +6,6 @@ <feature name='memory-hotplug'/> <feature name='cpu-hotplug'/> <graphics type='spice' port='5900' listen='127.0.0.1' tlsPort='-1'/> - <allowReboot value='yes'/> <capabilities> </capabilities> </qemu-migration> diff --git a/tests/qemumigrationcookiexmldata/nbd-bitmaps-xml2xml-out.xml b/tests/qemumigrationcookiexmldata/nbd-bitmaps-xml2xml-out.xml index 09b6fa291c..2bd6de42be 100644 --- a/tests/qemumigrationcookiexmldata/nbd-bitmaps-xml2xml-out.xml +++ b/tests/qemumigrationcookiexmldata/nbd-bitmaps-xml2xml-out.xml @@ -29,7 +29,6 @@ <feature policy='disable' name='npt'/> <feature policy='disable' name='nrip-save'/> </cpu> - <allowReboot value='yes'/> <capabilities> <cap name='xbzrle' auto='no'/> <cap name='auto-converge' auto='no'/> -- 2.31.1
Directly use 'priv->allowReboot' as we now document what the behaiour is to avoid another lookup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 4 +--- src/qemu/qemu_domain.c | 16 ---------------- src/qemu/qemu_domain.h | 3 --- src/qemu/qemu_process.c | 3 +-- 4 files changed, 2 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2756f46b51..5b743dd1ad 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6165,11 +6165,9 @@ qemuBuildPMCommandLine(virCommand *cmd, { virQEMUCaps *qemuCaps = priv->qemuCaps; - /* Only add -no-reboot option if each event destroys domain */ if (priv->allowReboot == VIR_TRISTATE_BOOL_NO) virCommandAddArg(cmd, "-no-reboot"); - - if (qemuDomainIsUsingNoShutdown(priv)) + else virCommandAddArg(cmd, "-no-shutdown"); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9baa4b5d90..ce355d37a0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11149,22 +11149,6 @@ qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason) } -/* qemuDomainIsUsingNoShutdown: - * @priv: Domain private data - * - * We can receive an event when QEMU stops. If we use no-shutdown, then - * we can watch for this event and do a soft/warm reboot. - * - * Returns: @true when -no-shutdown either should be or was added to the - * command line. - */ -bool -qemuDomainIsUsingNoShutdown(qemuDomainObjPrivate *priv) -{ - return priv->allowReboot == VIR_TRISTATE_BOOL_YES; -} - - bool qemuDomainDiskIsMissingLocalOptional(virDomainDiskDef *disk) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f3c6bb3390..cb1cd968d5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1001,9 +1001,6 @@ void qemuDomainStorageIdReset(qemuDomainObjPrivate *priv); virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason); -bool -qemuDomainIsUsingNoShutdown(qemuDomainObjPrivate *priv); - bool qemuDomainDiskIsMissingLocalOptional(virDomainDiskDef *disk); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5958a48182..c9cdff4e82 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8751,8 +8751,7 @@ qemuProcessReconnect(void *opaque) * domain crashed; otherwise, if the monitor was started, * then we can blame ourselves, else we failed before the * monitor started so we don't really know. */ - if (!priv->mon && tryMonReconn && - qemuDomainIsUsingNoShutdown(priv)) + if (!priv->mon && tryMonReconn && priv->allowReboot == VIR_TRISTATE_BOOL_YES) state = VIR_DOMAIN_SHUTOFF_CRASHED; else if (priv->mon) state = VIR_DOMAIN_SHUTOFF_DAEMON; -- 2.31.1
Without the ability to tell qemu to change the behaviour on reboot of the guest it's fundamentally unsafe to change the action as the guest would be able to execute instructions after the reboot before libvirt terminates it due to the async nature of QMP events. Stricten the code for now until we implement support for 'set-action' QMP command. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0472f54a37..3c57d67d1c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19671,10 +19671,11 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, goto endjob; if (def) { - if (priv->allowReboot == VIR_TRISTATE_BOOL_NO) { + if (priv->allowReboot == VIR_TRISTATE_BOOL_NO || + (type == VIR_DOMAIN_LIFECYCLE_REBOOT && + def->onReboot != action)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot update lifecycle action because QEMU " - "was started with -no-reboot option")); + _("cannot update lifecycle action because QEMU was started with incompatible -no-reboot setting")); goto endjob; } -- 2.31.1
The RESET event is delivered by qemu only when the guest OS is actually allowed to reboot ('-no-reboot' or equivalent is not used) and due to the nature of async handling of the events VM is actually already executing guest code after the reboot, until our code gets to killing it. In general it should have been impossible to reach a state where the reboot action is 'destroy' but we didn't use '-no-reboot' but due to various bugs it was. Due to the fact that this was not a desired operation and additionally guest code already is executing I think the best option is not to kill the VM any more (possible data loss?) and rely for the proper fix where we use the new 'set-action' QMP command to enable an equivalent behaviour to '-no-reboot' during runtime. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c9cdff4e82..a839d587c2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -443,27 +443,6 @@ qemuProcessHandleReset(qemuMonitor *mon G_GNUC_UNUSED, if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) VIR_WARN("Failed to save status on vm %s", vm->def->name); - if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY || - vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE) { - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - - if (!virDomainObjIsActive(vm)) { - VIR_DEBUG("Ignoring RESET event from inactive domain %s", - vm->def->name); - goto endjob; - } - - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, - QEMU_ASYNC_JOB_NONE, 0); - virDomainAuditStop(vm, "destroyed"); - qemuDomainRemoveInactive(driver, vm); - endjob: - qemuDomainObjEndJob(driver, vm); - } - - cleanup: virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); } -- 2.31.1
Rather than using '-no-reboot' use the QMP command to update the lifecycle action of 'on_reboot'. This will be identical to how we set the behaviour during lifetime and also avoids problems with use of the 'system-reset' QMP command during bringup of the VM (used to update the firmware table of disks when disks were hotplugged as part of startup). Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a839d587c2..c41c6ab793 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7024,6 +7024,37 @@ qemuProcessSetupDisksTransient(virDomainObj *vm, } +static int +qemuProcessSetupLifecycleActions(virDomainObj *vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int rc; + + if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION))) + return 0; + + /* for now we handle only onReboot->destroy here as an alternative to + * '-no-reboot' on the commandline */ + if (vm->def->onReboot != VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY) + return 0; + + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return -1; + + rc = qemuMonitorSetAction(priv->mon, + QEMU_MONITOR_ACTION_SHUTDOWN_KEEP, + QEMU_MONITOR_ACTION_REBOOT_SHUTDOWN, + QEMU_MONITOR_ACTION_WATCHDOG_KEEP, + QEMU_MONITOR_ACTION_PANIC_KEEP); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || rc < 0) + return -1; + + return 0; +} + + /** * qemuProcessLaunch: * @@ -7382,6 +7413,10 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; } + VIR_DEBUG("Setting handling of lifecycle actions"); + if (qemuProcessSetupLifecycleActions(vm, asyncJob) < 0) + goto cleanup; + ret = 0; cleanup: -- 2.31.1
The '-no-shutdown' flag prevents qemu from terminating if a shutdown was requested. Libvirt will handle the termination of the qemu process anyways and using this consistently will allow greater flexibility for the virDomainSetLifecycleAction API as well as will allow using the 'system-reset' QMP command during startup to reinitiate devices exported to the firmware. This efectively partially reverts 0e034efaf9b963760516a65413fd9771034357aa Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 12 +++++++++--- src/qemu/qemu_process.c | 4 +++- .../misc-no-reboot.x86_64-latest.args | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5b743dd1ad..b230314f7f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6165,10 +6165,16 @@ qemuBuildPMCommandLine(virCommand *cmd, { virQEMUCaps *qemuCaps = priv->qemuCaps; - if (priv->allowReboot == VIR_TRISTATE_BOOL_NO) - virCommandAddArg(cmd, "-no-reboot"); - else + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { + /* with new qemu we always want '-no-shutdown' on startup and we set + * all the other behaviour later during startup */ virCommandAddArg(cmd, "-no-shutdown"); + } else { + if (priv->allowReboot == VIR_TRISTATE_BOOL_NO) + virCommandAddArg(cmd, "-no-reboot"); + else + virCommandAddArg(cmd, "-no-shutdown"); + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) { if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c41c6ab793..bbcef47885 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8765,7 +8765,9 @@ qemuProcessReconnect(void *opaque) * domain crashed; otherwise, if the monitor was started, * then we can blame ourselves, else we failed before the * monitor started so we don't really know. */ - if (!priv->mon && tryMonReconn && priv->allowReboot == VIR_TRISTATE_BOOL_YES) + if (!priv->mon && tryMonReconn && + (priv->allowReboot == VIR_TRISTATE_BOOL_YES || + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION))) state = VIR_DOMAIN_SHUTOFF_CRASHED; else if (priv->mon) state = VIR_DOMAIN_SHUTOFF_DAEMON; diff --git a/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args b/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args index 197b6375a7..f34e9c8708 100644 --- a/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args +++ b/tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args @@ -23,7 +23,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ --no-reboot \ +-no-shutdown \ -no-acpi \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -- 2.31.1
We don't use the value of the flag when the new handling is in place so we don't have to initialize it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bbcef47885..e2bcd23954 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6354,6 +6354,11 @@ qemuProcessPrepareAllowReboot(virDomainObj *vm) virDomainDef *def = vm->def; qemuDomainObjPrivate *priv = vm->privateData; + /* with 'set-action' QMP command we don't need to keep this around as + * we always update qemu with the proper state */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) + return; + if (priv->allowReboot != VIR_TRISTATE_BOOL_ABSENT) return; -- 2.31.1
When qemu supports 'set-action' command we can update what happens on reboot. Additionally we can fully relax the checks as we now properly update the lifecycle actions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 69 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3c57d67d1c..961f521f4b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19629,6 +19629,58 @@ qemuDomainModifyLifecycleAction(virDomainDef *def, } +static int +qemuDomainModifyLifecycleActionLive(virDomainObj *vm, + virDomainLifecycle type, + virDomainLifecycleAction action) +{ + qemuMonitorActionReboot monReboot = QEMU_MONITOR_ACTION_REBOOT_KEEP; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + int rc; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) + return 0; + + /* For now we only update 'reboot' action here as we want to keep the + * shutdown action as is (we're emulating the outcome anyways)) */ + if (type != VIR_DOMAIN_LIFECYCLE_REBOOT || + vm->def->onReboot == action) + return 0; + + + switch (action) { + case VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY: + monReboot = QEMU_MONITOR_ACTION_REBOOT_SHUTDOWN; + break; + + case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART: + monReboot = QEMU_MONITOR_ACTION_REBOOT_RESET; + break; + + case VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE: + case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME: + case VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_DESTROY: + case VIR_DOMAIN_LIFECYCLE_ACTION_COREDUMP_RESTART: + case VIR_DOMAIN_LIFECYCLE_ACTION_LAST: + return 0; + } + + + qemuDomainObjEnterMonitor(driver, vm); + + rc = qemuMonitorSetAction(priv->mon, + QEMU_MONITOR_ACTION_SHUTDOWN_KEEP, + monReboot, + QEMU_MONITOR_ACTION_WATCHDOG_KEEP, + QEMU_MONITOR_ACTION_PANIC_KEEP); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + return -1; + + return 0; +} + static int qemuDomainSetLifecycleAction(virDomainPtr dom, @@ -19671,14 +19723,19 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, goto endjob; if (def) { - if (priv->allowReboot == VIR_TRISTATE_BOOL_NO || - (type == VIR_DOMAIN_LIFECYCLE_REBOOT && - def->onReboot != action)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot update lifecycle action because QEMU was started with incompatible -no-reboot setting")); - goto endjob; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { + if (priv->allowReboot == VIR_TRISTATE_BOOL_NO || + (type == VIR_DOMAIN_LIFECYCLE_REBOOT && + def->onReboot != action)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot update lifecycle action because QEMU was started with incompatible -no-reboot setting")); + goto endjob; + } } + if (qemuDomainModifyLifecycleActionLive(vm, type, action) < 0) + goto endjob; + qemuDomainModifyLifecycleAction(def, type, action); qemuDomainSaveStatus(vm); -- 2.31.1
In cases when we are adding a <transient/> disk with sharing backend (and thus hotplugging it) we need to re-initialize ACPI tables so that the VM boots from the correct device. This has a side-effect of emitting the RESET event and forwarding it to the clients which is not correct. Fix this by ignoring RESET events during startup of the VM. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e2bcd23954..fc273b6365 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -429,12 +429,24 @@ qemuProcessHandleReset(qemuMonitor *mon G_GNUC_UNUSED, void *opaque) { virQEMUDriver *driver = opaque; - virObjectEvent *event; + virObjectEvent *event = NULL; qemuDomainObjPrivate *priv; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainState state; + int reason; virObjectLock(vm); + state = virDomainObjGetState(vm, &reason); + + /* ignore reset events on VM startup. Libvirt in certain instances does a + * reset during startup so that the ACPI tables are re-generated */ + if (state == VIR_DOMAIN_PAUSED && + reason == VIR_DOMAIN_PAUSED_STARTING_UP) { + VIR_DEBUG("ignoring reset event during startup"); + goto unlock; + } + event = virDomainEventRebootNewFromObj(vm); priv = vm->privateData; if (priv->agent) @@ -443,6 +455,7 @@ qemuProcessHandleReset(qemuMonitor *mon G_GNUC_UNUSED, if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) VIR_WARN("Failed to save status on vm %s", vm->def->name); + unlock: virObjectUnlock(vm); virObjectEventStateQueue(driver->domainEventState, event); } -- 2.31.1
On 8/24/21 4:44 PM, Peter Krempa wrote:
While investigating how to fix issue with transient disks breaking when -no-shutdown is not used I've ended up figuring out that lifecycle action handling in the qemu driver is very broken.
Unbreak the handling by rejecting some actions which were never implemented, add support for 'set-action' qmp command and use it to update the 'reset' action in qemu and always use '-no-shutdown'.
Peter Krempa (22): qemuMonitorJSONSetWatchdogAction: Use automatic memory clearing qemuDomainSetLifecycleAction: Add a note about argument range-check qemu: driver: Use 'qemuDomainSaveStatus' for saving status XML qemu: validate: Reformat header and purge unused includes qemu: Reject 'rename-restart' action for 'on_reboot'/'on_poweroff'/'on_crash' qemu: driver: Validate lifecycle actions in 'qemuDomainSetLifecycleAction' qemu: Reject 'preserve' action for 'on_reboot'/'on_poweroff'/'on_crash' qemu: Honor 'restart' action for 'on_poweroff' qemu: capablities: Detect presence of 'set-action' as QEMU_CAPS_SET_ACTION qemu: monitor: Implement monitor code for 'set-action' command qemuDomainAttachWatchdog: Use 'set-action' instead of 'watchdog-set-action' if supported qemuxml2argvtest: Add 'LATEST' version of 'misc-no-reboot' test case qemuDomainObjPrivate: Annotate 'allowReboot' field qemu: migration: Don't transfer 'allowReboot' flag qemu: domain: Remove qemuDomainIsUsingNoShutdown qemuDomainSetLifecycleAction: Forbid live update of 'on_reboot' qemuProcessHandleReset: Don't emulate lifecycle actions for RESET event qemuProcessLaunch: Setup handling of 'on_reboot' via QMP when starting the process qemu: command: Always use '-no-shutdown' qemu: process: Don't set 'allowReboot' when qemu supports 'set-action' qemuDomainSetLifecycleAction: Properly update 'onReboot' action in qemu qemu: process: Ignore 'RESET' event during startup
docs/formatdomain.rst | 8 +- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 14 +- src/qemu/qemu_domain.c | 16 -- src/qemu/qemu_domain.h | 11 +- src/qemu/qemu_driver.c | 201 +++++++++++------- src/qemu/qemu_hotplug.c | 54 ++++- src/qemu/qemu_migration.c | 5 - src/qemu/qemu_migration_cookie.c | 22 -- src/qemu/qemu_migration_cookie.h | 4 - src/qemu/qemu_monitor.c | 16 ++ src/qemu/qemu_monitor.h | 49 +++++ src/qemu/qemu_monitor_json.c | 95 ++++++++- src/qemu/qemu_monitor_json.h | 7 + src/qemu/qemu_process.c | 85 +++++--- src/qemu/qemu_validate.c | 51 +++++ src/qemu/qemu_validate.h | 31 +-- .../caps_6.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../basic-xml2xml-out.xml | 1 - .../full-xml2xml-out.xml | 1 - .../modern-dom-out-dest.xml | 1 - .../modern-dom-out-source.xml | 1 - .../nbd-bitmaps-xml2xml-out.xml | 1 - tests/qemumonitorjsontest.c | 6 + .../misc-no-reboot.x86_64-latest.args | 35 +++ tests/qemuxml2argvtest.c | 1 + 30 files changed, 529 insertions(+), 194 deletions(-) create mode 100644 tests/qemuxml2argvdata/misc-no-reboot.x86_64-latest.args
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Christian Borntraeger -
Daniel P. Berrangé -
Michal Prívozník -
Peter Krempa