[libvirt] [PATCH] snapshot: allow reuse of existing files in disk snapshot

[Peter Krempa]

On 01/09/2012 08:03 PM, Eric Blake wrote:
> When disk snapshots were first implemented, libvirt blindly refused
> to allow an external snapshot destination that already exists, since
> qemu will blindly overwrite the contents of that file during the
> snapshot_blkdev monitor command, and we don't like a default of
> data loss by default.  But VDSM has a scenario where NFS permissions
> are intentionally set so that the destination file can only be
> created by the management machine, and not the machine where the
> guest is running, so that libvirt will necessarily see the destination
> file already existing; adding a flag will allow VDSM to force the file
> reuse without libvirt complaining of possible data loss.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=767104
>
> * include/libvirt/libvirt.h.in (virDomainSnapshotCreateFlags): Add
> VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT.
> * src/libvirt.c (virDomainSnapshotCreateXML): Document it.  Add
> note about partial failure.
> * tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Add new
> flag.
> * tools/virsh.pod (snapshot-create, snapshot-create-as): Document
> it.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskPrepare)
> (qemuDomainSnapshotCreateXML): Implement the new flag.
> ---
>
> I'm not sure I have the best name for the proposed flag; alternative
> suggestions are welcome.

I couldn't come up with anything better.

>
>   include/libvirt/libvirt.h.in |    2 ++
>   src/libvirt.c                |   13 +++++++++++++
>   src/qemu/qemu_driver.c       |   10 +++++++---
>   tools/virsh.c                |    8 +++++++-
>   tools/virsh.pod              |   16 +++++++++++++---
>   5 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index ad6fcce..0b564cf 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -2990,6 +2990,8 @@ typedef enum {
>                                                             after snapshot */
>       VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY   = (1<<  4), /* disk snapshot, not
>                                                             system checkpoint */
> +    VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1<<  5), /* reuse any existing
> +                                                          external files */
>   } virDomainSnapshotCreateFlags;

You added a new flag here

>
>   /* Take a snapshot of the current VM state */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1a7e816..9765a69 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10162,11 +10163,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,

--- SNIP from top of this func. ------
     virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
                   VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT |
                   VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA |
                   VIR_DOMAIN_SNAPSHOT_CREATE_HALT |
                   VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, NULL);
----- </snip> -----------

But you don't check for the new flag here.

Running virsh with the new flag:

virsh # snapshot-create 1 ../test.xml --reuse-external
error: invalid argument: qemuDomainSnapshotCreateXML: unsupported flags 
(0x20)

>               goto cleanup;
>
>           if (flags&  VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
> +            bool allow_reuse;
> +
> +            allow_reuse = (flags&  VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
>               if (virDomainSnapshotAlignDisks(def,
>                                               VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL,
>                                               false)<  0)

Apart from that I think the code looks sane. ACK with that flag check fixed.

Peter