[libvirt] [PATCH 06/10] Make QEMU perform managed save of all VMs on stop of libvirtd

[Eric Blake]

> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> When the virStateStop() method is invoked, perform a managed
> save of all VMs currently running
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 69
>  ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)

How does this interact with the libvirt-guests init script?  I guess
that if that is installed, it gets to run first before the system
libvirtd is shutdown, so the qemu:///system won't see any running
guests, and this patch would just affect qemu:///session?

Is managed save always the right thing, or should we allow the user to
configure for a graceful shutdown and/or a migration to other host?  Do
we need timeouts, as managed save may take a while per guest?  Similar
to how /etc/sysconfig/libvirt-guests can request O_DIRECT to avoid
file system pollution, do we need a way for the user to configure the
use of that flag?  Is doing the same thing for all guests wise, or do
we need to allow for the possibility of a per-guest choice of what action
to take (maybe via a new <on_host_shutdown> element?) where we only fall
back to the configured default if the guest XML didn't request something?
Do we correctly and automatically restart all the guests that were saved
by this hook the next time libvirtd restarts?

> +    /* First we pause all VMs to make them stop dirtying
> +       pages, etc. We remember if any VMs were paused so
> +       we can restore that on resume. */
> +    for (i = 0 ; i < numDomains ; i++) {
> +        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;
> +            }
> +        }
> +        virDomainSuspend(domains[i]);

Should you be checking for errors here?

> +    }
> +
> +    ret = 0;
> +    /* Then we save the VMs to disk */
> +    for (i = 0 ; i < numDomains ; i++)
> +        if (virDomainManagedSave(domains[i], flags[i]) < 0)
> +            ret = -1;

What happens if a guest managed to exit itself after the initial
virConnectListAllDomains() and this point?  The virDomainManagedSave
will fail, but that is not an error since a stopped guest has
nothing to save after all.  Do you need to be checking specific
error codes here before treating this as a failure?

In general, I like the idea, but it introduces enough questions that
I'm not quite sure about ack'ing it yet.  I'll see if reviewing
the rest of the series impacts my decision.