On Wed, Mar 12, 2025 at 17:17:44 +0000, Daniel P. Berrangé wrote:
> It may be desirable to treat transient VMs differently from persistent
> VMs. For example, while performing managed save on persistent VMs makes
> sense, the same not usually true of transient VMs, since by their
> nature they will have no config to restore from.
>
> This also lets us fix a long standing problem with incorrectly
> attempting to perform managed save on transient VMs.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> src/hypervisor/domain_driver.c | 62 +++++++++++++++++++++++++++++++---
> src/hypervisor/domain_driver.h | 18 ++++++++--
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_driver.c | 6 ++--
> 4 files changed, 77 insertions(+), 11 deletions(-)
>
> diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> index e625726c07..c510e1d2ae 100644
> --- a/src/hypervisor/domain_driver.c
> +++ b/src/hypervisor/domain_driver.c
> @@ -35,6 +35,13 @@
>
> VIR_LOG_INIT("hypervisor.domain_driver");
>
> +VIR_ENUM_IMPL(virDomainDriverAutoShutdownScope,
> + VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_LAST,
> + "none",
> + "persistent",
> + "transient",
> + "all");
> +
> char *
> virDomainDriverGenerateRootHash(const char *drivername,
> const char *root)
> @@ -721,9 +728,13 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> int numDomains = 0;
> size_t i;
> virDomainPtr *domains = NULL;
> + g_autofree bool *transient = NULL;
>
> - VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d poweroff=%d waitShutdownSecs=%u",
> - cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff,
> + VIR_DEBUG("Run autoshutdown uri=%s trySave=%s tryShutdown=%s poweroff=%s waitShutdownSecs=%u",
> + cfg->uri,
> + virDomainDriverAutoShutdownScopeTypeToString(cfg->trySave),
> + virDomainDriverAutoShutdownScopeTypeToString(cfg->tryShutdown),
> + virDomainDriverAutoShutdownScopeTypeToString(cfg->poweroff),
> cfg->waitShutdownSecs);
>
> /*
> @@ -740,6 +751,21 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> if (cfg->waitShutdownSecs <= 0)
> cfg->waitShutdownSecs = 30;
>
> + /* Short-circuit if all actions are disabled */
> + if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> + cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> + cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
> + return;
> +
> + if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> + cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT) {
> + /* virDomainManagedSave will return an error. We'll let
> + * the code run, since it'll just fall through to the next
> + * actions, but give a clear warning upfront */
> + VIR_WARN("Managed save not supported for transient VMs");
> + return;
This looks like better suited to the config parser. Reporting a
programming error as warning seems wrong.
This code should IMO at ignore invalid setting for managed-save config
rather than not do anything at all.
> + }
> +
> if (!(conn = virConnectOpen(cfg->uri)))
> goto cleanup;
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
