On Thu, Jul 03, 2025 at 02:50:33PM +0200, Peter Krempa via Devel wrote: > From: Peter Krempa <[email protected]> > > Decide separately and record what shutdown modes are to be applied on > given VM object rather than spreading out the logic through the code. > > This centralization simplifies the conditions in the worker functions > and also: > - provides easy way to check if the auto-shutdown code will be acting > on domain object (will be used to fix attempt to auto-restore of > VMs which were not selected to be acted on > - will simplify further work where the desired shutdown action will be > picked per-VM > > This refactor also fixes a bug where if restoring of the state is > applied also on VMs that are not selected for action based on current > logic. > > Signed-off-by: Peter Krempa <[email protected]> > --- > src/hypervisor/domain_driver.c | 176 +++++++++++++++++++-------------- > 1 file changed, 100 insertions(+), 76 deletions(-) > > diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c > index d8ccee40d5..7f958c087f 100644 > --- a/src/hypervisor/domain_driver.c > +++ b/src/hypervisor/domain_driver.c > @@ -738,25 +738,32 @@ > virDomainDriverAutoShutdownActive(virDomainDriverAutoShutdownConfig *cfg) > } > > > +enum { > + VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_SAVE = 1 >> 1, > + VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_SHUTDOWN = 1 >> 2, > + VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_POWEROFF = 1 >> 3, > + VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_RESTORE = 1 >> 4,
s/>>/<</
> +} virDomainDriverAutoShutdownModeFlag;
[...]
> +static unsigned int
> +virDomainDriverAutoShutdownGetMode(virDomainPtr domain,
> + virDomainDriverAutoShutdownConfig *cfg)
> +{
> + unsigned int mode = 0;
> +
> + if (virDomainIsPersistent(domain) == 0) {
> + if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> + cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT)
> + mode |= VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_SAVE;
> +
> + if (cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> + cfg->tryShutdown ==
> VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT)
> + mode |= VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_SHUTDOWN;
> +
> + if (cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> + cfg->poweroff ==
> VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT)
> + mode |= VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_POWEROFF;
> +
> + /* Don't restore VMs which weren't selected for auto-shutdown */
> + if (mode != 0 && cfg->autoRestore)
> + mode |= VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_RESTORE;
> + } else {
> + if (cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> + cfg->tryShutdown ==
> VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT)
> + mode |= VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_SHUTDOWN;
> +
> + if (cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> + cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT)
> + mode |= VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_POWEROFF;
> +
> + if (cfg->autoRestore)
> + VIR_DEBUG("Cannot auto-restore transient VM '%s'",
> + virDomainGetName(domain));
Wrong indentation.
> + }
> +
> + return mode;
> +}
> +
>
> void
> virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> @@ -907,7 +941,7 @@
> virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> int numDomains = 0;
> size_t i;
> virDomainPtr *domains = NULL;
> - g_autofree bool *transient = NULL;
> + g_autofree unsigned int *modes = NULL;
>
> VIR_DEBUG("Run autoshutdown uri=%s trySave=%s tryShutdown=%s poweroff=%s
> waitShutdownSecs=%u saveBypassCache=%d autoRestore=%d",
> cfg->uri,
> @@ -948,58 +982,48 @@
> virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> return;
>
> if (!(conn = virConnectOpen(cfg->uri)))
> - goto cleanup;
> + return;
>
> if ((numDomains = virConnectListAllDomains(conn,
> &domains,
>
> VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0)
> - goto cleanup;
> + return;
>
> VIR_DEBUG("Auto shutdown with %d running domains", numDomains);
>
> - transient = g_new0(bool, numDomains);
> + modes = g_new0(unsigned int, numDomains);
> +
> for (i = 0; i < numDomains; i++) {
> - if (virDomainIsPersistent(domains[i]) == 0)
> - transient[i] = true;
> + modes[i] = virDomainDriverAutoShutdownGetMode(domains[i], cfg);
>
> - if (cfg->autoRestore) {
> - if (transient[i]) {
> - VIR_DEBUG("Cannot auto-restore transient VM %s",
> - virDomainGetName(domains[i]));
> - } else {
> - VIR_DEBUG("Mark %s for autostart on next boot",
> - virDomainGetName(domains[i]));
> - if (virDomainSetAutostartOnce(domains[i], 1) < 0) {
> - VIR_WARN("Unable to mark domain '%s' for auto restore:
> %s",
> - virDomainGetName(domains[i]),
> - virGetLastErrorMessage());
> - }
> + if (modes[i] == 0) {
> + /* VM wasn't selected for any of the shutdown modes. There's not
> + * much we can do about that as the host is powering off, logging
> + * at least lets admins know */
> + VIR_WARN("auto-shutdown: domain '%s' not successfully shut off
> by any action",
> + domains[i]->name);
> + }
> +
> + if (modes[i] & VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_MODE_RESTORE) {
> + VIR_DEBUG("Mark %s for autostart on next boot",
Suggestion: I know it's preexisting but would be nice to s/%s/'%s'/.
With the issues fixed:
Reviewed-by: Pavel Hrdina <[email protected]>
signature.asc
Description: PGP signature
