Re: [PATCH 10/10] virDomainDriverAutoShutdown: Refactor selection logic for VMs
On Thu, Jul 03, 2025 at 14:50:33 +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa(a)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 <pkrempa(a)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.