Re: [PATCH v2 04/22] hypervisor: custom shutdown actions for transient vs persistent VMs

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt;