On Thu, Mar 13, 2025 at 01:31:41PM +0100, Pavel Hrdina wrote:
> On Wed, Mar 12, 2025 at 05:17:45PM +0000, Daniel P. Berrangé wrote:
> > Currently automatic VM managed save is only performed in session
> > daemons, on desktop session close, or host OS shutdown request.
> >
> > With this change it is possible to control shutdown behaviour for
> > all daemons. A recommended setup might be:
> >
> > auto_shutdown_try_save = "persistent"
> > auto_shutdown_try_shutdown = "all"
> > auto_shutdown_poweroff = "all"
> >
> > Each setting accepts 'none', 'persistent', 'transient', and 'all'
> > to control what types of guest it applies to.
> >
> > For historical compatibility, for the system daemon, the settings
> > currently default to:
> >
> > auto_shutdown_try_save = "none"
> > auto_shutdown_try_shutdown = "none"
> > auto_shutdown_poweroff = "none"
> >
> > while for the session daemon they currently default to
> >
> > auto_shutdown_try_save = "persistent"
> > auto_shutdown_try_shutdown = "none"
> > auto_shutdown_poweroff = "none"
> >
> > The system daemon settings should NOT be enabled if the traditional
> > libvirt-guests.service is already enabled.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/qemu/libvirtd_qemu.aug | 3 ++
> > src/qemu/qemu.conf.in | 42 +++++++++++++++++++
> > src/qemu/qemu_conf.c | 65 ++++++++++++++++++++++++++++++
> > src/qemu/qemu_conf.h | 4 ++
> > src/qemu/qemu_driver.c | 9 ++---
> > src/qemu/test_libvirtd_qemu.aug.in | 3 ++
> > 6 files changed, 121 insertions(+), 5 deletions(-)
>
> [...]
>
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index b376841388..8554558201 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
>
> [...]
>
> > @@ -627,6 +642,8 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
> > g_autofree char *savestr = NULL;
> > g_autofree char *dumpstr = NULL;
> > g_autofree char *snapstr = NULL;
> > + g_autofree char *autoShutdownScope = NULL;
> > + int autoShutdownScopeVal;
> >
> > if (virConfGetValueString(conf, "save_image_format", &savestr) < 0)
> > return -1;
> > @@ -663,6 +680,54 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
> > return -1;
> > if (virConfGetValueUInt(conf, "auto_start_delay", &cfg->autoStartDelayMS) < 0)
> > return -1;
> > + if (virConfGetValueString(conf, "auto_shutdown_try_save", &autoShutdownScope) < 0)
> > + return -1;
> > +
> > + if (autoShutdownScope != NULL) {
> > + if ((autoShutdownScopeVal =
> > + virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> > + virReportError(VIR_ERR_INVALID_ARG,
> > + _("unknown auto_shutdown_try_save '%1$s'"),
> > + autoShutdownScope);
> > + return -1;
> > + }
> > + cfg->autoShutdownTrySave = autoShutdownScopeVal;
> > + }
> > +
> > + if (cfg->autoShutdownTrySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> > + cfg->autoShutdownTrySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("managed save cannot be requested for transient domains"));
> > + return -1;
> > + }
> > +
> > + if (virConfGetValueString(conf, "auto_shutdown_try_shutdown", &autoShutdownScope) < 0)
>
> I wanted to note the we would leak memory here when reusing the same
> string but virConfGetValueString frees the value before assigning new
> data to it. But it doesn't touch the variable if the config option is
> missing so the next check for NULL can be invalid if
> auto_shutdown_try_save is set but auto_shutdown_try_shutdown is not set.
Doh, yes, we need g_clear_pointer(&autoShutdownScope, g_free) before
re-fetching the next value, or perhaps better just stop overloading
one variable for fetching multiple config parameter
>
> > + return -1;
> > +
> > + if (autoShutdownScope != NULL) {
> > + if ((autoShutdownScopeVal =
> > + virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> > + virReportError(VIR_ERR_INVALID_ARG,
> > + _("unknown auto_shutdown_try_shutdown '%1$s'"),
> > + autoShutdownScope);
> > + return -1;
> > + }
> > + cfg->autoShutdownTryShutdown = autoShutdownScopeVal;
> > + }
> > +
> > + if (virConfGetValueString(conf, "auto_shutdown_poweroff", &autoShutdownScope) < 0)
> > + return -1;
>
> Same here.
>
> > + if (autoShutdownScope != NULL) {
> > + if ((autoShutdownScopeVal =
> > + virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> > + virReportError(VIR_ERR_INVALID_ARG,
> > + _("unknown auto_shutdown_poweroff '%1$s'"),
> > + autoShutdownScope);
> > + return -1;
> > + }
> > + cfg->autoShutdownPoweroff = autoShutdownScopeVal;
> > + }
> >
> > return 0;
> > }
>
> Pavel
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 :|
