On Thu, Jul 03, 2025 at 14:50:33 +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(-)
While actually testing this I've noticed that ...
> +static unsigned int
> +virDomainDriverAutoShutdownGetMode(virDomainPtr domain,
> + virDomainDriverAutoShutdownConfig *cfg)
> +{
> + unsigned int mode = 0;
> +
> + if (virDomainIsPersistent(domain) == 0) {
... this check is incorrect as it matches transient domains. Given
that ...
> + 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;
> - 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;
... it was written this way, to pick the 'transient' code path only when
"a successful false" was returned ...
> + modes[i] = virDomainDriverAutoShutdownGetMode(domains[i], cfg);
>
> - if (cfg->autoRestore) {
... I'll change it to
+ if (virDomainIsPersistent(domain) != 0) {
// code when persistent
...
before pushing and also I'll wear the brown paper bag of shame for this
patch.
