[libvirt] [PATCHv2 3/3] snapshot: rudimentary qemu support for atomic disk snapshot

[Peter Krempa]

On 03/17/2012 05:33 PM, Eric Blake wrote:
> Taking an external snapshot of just one disk is atomic, without having
> to pause and resume the VM.  This also paves the way for later patches
> to interact with the new qemu 'transaction' monitor command.
>
> The various scenarios when requesting atomic are:
> online, 1 disk, old qemu - safe, allowed by this patch
> online, more than 1 disk, old qemu - failure, this patch
> offline snapshot - safe, once a future patch implements offline disk snapshot
> online, 1 or more disks, new qemu - safe, once future patch uses transaction
>
> Taking an online system checkpoint snapshot is atomic, since it is
> done via a single 'savevm' monitor command.
>
> Taking an offline system checkpoint snapshot currently uses multiple
> qemu-img invocations with no provision for cleanup of partial failure,
> so for now we mark it unsupported.
>
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support
> new flag for single-disk setups.
> (qemuDomainSnapshotDiskPrepare): Check for atomic here.
> (qemuDomainSnapshotCreateDiskActive): Skip pausing the VM when
> atomic supported.
> (qemuDomainSnapshotIsAllowed): Use bool instead of int.
> ---
>
> Patches 1/3 and 2/3 unchanged.
> v2: consider interactions of atomic with non-disk-snapshots
>
>   src/qemu/qemu_driver.c |   56 +++++++++++++++++++++++++++++++++--------------
>   1 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 85f8cf7..9e62738 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10279,6 +10295,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                                                  &vm, snap, flags)<  0)
>               goto cleanup;
>       } else if (!virDomainObjIsActive(vm)) {
> +        if (flags&  VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) {
> +            qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                            _("atomic snapshots of inactive domains not "
> +                              "implemented yet"));
> +            goto cleanup;

I wonder if we shouldn't add a error code for unimplemented 
functionality to differentiate it from unsupported configurations. 
(Well, but using a configuration that uses unimplemented functionality 
makes it an unsupported configuration basically, so I don't really mind)


> +        }
>           if (qemuDomainSnapshotCreateInactive(driver, vm, snap)<  0)
>               goto cleanup;
>       } else {

ACK,

Peter.