On Thu, Mar 13, 2025 at 09:00:58AM +0100, Peter Krempa wrote:
> On Wed, Mar 12, 2025 at 17:17:43 +0000, Daniel P. Berrangé wrote:
> > The auto shutdown code can currently only perform managed save,
> > which may fail in some cases, for example when PCI devices are
> > assigned. On failure, shutdown inhibitors remain in place which
> > may be undesirable.
> >
> > This expands the logic to try a sequence of operations
> >
> > * Managed save
> > * Graceful shutdown
> > * Forced poweroff
> >
> > Each of these operations can be enabled or disabled, but they
> > are always applied in this order.
> >
> > With the shutdown option, a configurable time is allowed for
> > shutdown to complete, defaulting to 30 seconds, before moving
> > onto the forced poweroff phase.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/hypervisor/domain_driver.c | 130 ++++++++++++++++++++++++++++-----
> > src/hypervisor/domain_driver.h | 7 ++
> > src/qemu/qemu_driver.c | 3 +
> > 3 files changed, 121 insertions(+), 19 deletions(-)
>
> [...]
>
> >
> > @@ -732,31 +748,107 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> > VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0)
> > goto cleanup;
> >
> > - flags = g_new0(unsigned int, numDomains);
> > + VIR_DEBUG("Auto shutdown with %d running domains", numDomains);
> > + if (cfg->trySave) {
> > + g_autofree unsigned int *flags = g_new0(unsigned int, numDomains);
> > + for (i = 0; i < numDomains; i++) {
> > + int state;
> > + /*
> > + * Pause all VMs to make them stop dirtying pages,
> > + * so save is quicker. We remember if any VMs were
> > + * paused so we can restore that on resume.
> > + */
> > + flags[i] = VIR_DOMAIN_SAVE_RUNNING;
> > + if (virDomainGetState(domains[i], &state, NULL, 0) == 0) {
> > + if (state == VIR_DOMAIN_PAUSED)
> > + flags[i] = VIR_DOMAIN_SAVE_PAUSED;
> > + }
> > + if (flags[i] & VIR_DOMAIN_SAVE_RUNNING)
> > + virDomainSuspend(domains[i]);
> > + }
> > +
> > + for (i = 0; i < numDomains; i++) {
> > + if (virDomainManagedSave(domains[i], flags[i]) < 0) {
> > + VIR_WARN("Unable to perform managed save of '%s': %s",
> > + virDomainGetName(domains[i]),
>
> virDomainGetName() calls virResetLastError()
>
> > + virGetLastErrorMessage());
>
> IIRC argument evaluation order is not defined this might have random
> results.
In fact it is worse than that. If virGetLastErrorMessage runs
second (order as written) it will get no error message. If it
runs first (reverse as written) it will get the error message
which virDomainGetName will cause to be free()d, so it will
be passed freed memory to VIR_WARN.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
