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