On Thu, Jul 03, 2025 at 02:50:33PM +0200, Peter Krempa via Devel wrote:
> From: Peter Krempa <pkre...@redhat.com>
>
> 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 <pkre...@redhat.com>
> ---
> 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 <phrd...@redhat.com>
signature.asc
Description: PGP signature
