Each <domainsnapshot> can now contain an optional <memory> element that describes how the VM state was handled, similar to disk snapshots. The new element will always appear in output; for back-compat, an input that lacks the element will assume 'no' or 'internal' according to the domain state. Along with this change, it is now possible to pass <disks> in the XML for an offline snapshot; this also needs to be wired up in a future patch, to make it possible to choose internal vs. external on a per-disk basis for each disk in an offline domain. At that point, using the --disk-only flag for an offline domain will be able to work. For some examples below, remember that qemu supports the following snapshot actions: qemu-img: offline external and internal disk savevm: online internal VM and disk migrate: online external VM transaction: online external disk ===== <domainsnapshot> <memory snapshot='no'/> ... </domainsnapshot> implies that there is no VM state saved (mandatory for offline and disk-only snapshots, not possible otherwise); using qemu-img for offline domains and transaction for online. ===== <domainsnapshot> <memory snapshot='internal'/> ... </domainsnapshot> state is saved inside one of the disks (as in qemu's 'savevm' system checkpoint implementation). If needed in the future, we can also add an attribute pointing out _which_ disk saved the internal state; maybe disk='vda'. ===== <domainsnapshot> <memory snapshot='external' file='/path/to/state'/> ... </domainsnapshot> This is not wired up yet, but future patches will allow this to control a combination of 'virsh save /path/to/state' plus disk snapshots from the same point in time. ===== So for 0.10.2, I plan to implement this table of combinations, with '*' designating new code and '+' designating existing code reached through new combinations of xml and/or the existing DISK_ONLY flag: domain memory disk disk-only | result ----------------------------------------- offline omit omit any | memory=no disk=int, via qemu-img offline no omit any |+memory=no disk=int, via qemu-img offline omit/no no any | invalid combination (nothing to snapshot) offline omit/no int any |+memory=no disk=int, via qemu-img offline omit/no ext any |*memory=no disk=ext, via qemu-img offline int/ext any any | invalid combination (no memory to save) online omit omit off | memory=int disk=int, via savevm online omit omit on | memory=no disk=default, via transaction online omit no/ext off | unsupported for now online omit no on | invalid combination (nothing to snapshot) online omit ext on | memory=no disk=ext, via transaction online omit int off |+memory=int disk=int, via savevm online omit int on | unsupported for now online no omit any |+memory=no disk=default, via transaction online no no any | invalid combination (nothing to snapshot) online no int any | unsupported for now online no ext any |+memory=no disk=ext, via transaction online int/ext any on | invalid combination (disk-only vs. memory) online int omit off |+memory=int disk=int, via savevm online int no/ext off | unsupported for now online int int off |+memory=int disk=int, via savevm online ext omit off |*memory=ext disk=default, via migrate+trans online ext no off |+memory=ext disk=no, via migrate online ext int off | unsupported for now online ext ext off |*memory=ext disk=ext, via migrate+transaction * docs/schemas/domainsnapshot.rng (memory): New RNG element. * docs/formatsnapshot.html.in: Document it. * src/conf/snapshot_conf.h (virDomainSnapshotDef): New fields. * src/conf/domain_conf.c (virDomainSnapshotDefFree) (virDomainSnapshotDefParseString, virDomainSnapshotDefFormat): Manage new fields. * tests/domainsnapshotxml2xmltest.c: New test. * tests/domainsnapshotxml2xmlin/*.xml: Update existing tests. * tests/domainsnapshotxml2xmlout/*.xml: Likewise. --- docs/formatsnapshot.html.in | 11 ++++ docs/schemas/domainsnapshot.rng | 23 ++++++++ src/conf/snapshot_conf.c | 66 +++++++++++++++++----- src/conf/snapshot_conf.h | 4 ++ tests/domainsnapshotxml2xmlin/external_vm.xml | 10 ++++ tests/domainsnapshotxml2xmlin/noparent.xml | 9 +++ tests/domainsnapshotxml2xmlout/all_parameters.xml | 1 + tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 1 + tests/domainsnapshotxml2xmlout/external_vm.xml | 43 ++++++++++++++ tests/domainsnapshotxml2xmlout/full_domain.xml | 1 + tests/domainsnapshotxml2xmlout/metadata.xml | 1 + tests/domainsnapshotxml2xmlout/noparent.xml | 1 + .../noparent_nodescription.xml | 1 + .../noparent_nodescription_noactive.xml | 1 + tests/domainsnapshotxml2xmltest.c | 1 + 15 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/external_vm.xml create mode 100644 tests/domainsnapshotxml2xmlin/noparent.xml create mode 100644 tests/domainsnapshotxml2xmlout/external_vm.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index ec5ebf3..2af32bc 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -106,6 +106,17 @@ description is omitted when initially creating the snapshot, then this field will be empty. </dd> + <dt><code>memory</code></dt> + <dd>On input, this is an optional request for how to handle VM + state. For an offline domain or a disk-only snapshot, + attribute <code>snapshot</code> must be <code>no</code>, since + there is no VM state saved; otherwise, the attribute can + be <code>internal</code> if the VM state is piggy-backed with + other internal disk state, or <code>external</code> along with + a second attribute <code>file</code> giving the absolute path + of the file holding the VM state. <span class="since">Since + 0.10.2</span> + </dd> <dt><code>disks</code></dt> <dd>On input, this is an optional listing of specific instructions for disk snapshots; it is needed when making a diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 0ef0631..230833d 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -31,6 +31,29 @@ </element> </optional> <optional> + <element name='memory'> + <choice> + <attribute name='snapshot'> + <choice> + <value>no</value> + <value>internal</value> + </choice> + </attribute> + <group> + <optional> + <attribute name='snapshot'> + <value>external</value> + </attribute> + </optional> + <attribute name='file'> + <ref name='absFilePath'/> + </attribute> + </group> + </choice> + <empty/> + </element> + </optional> + <optional> <element name='disks'> <zeroOrMore> <ref name='disksnapshot'/> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index e13cdd6..7a8a564 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -95,6 +95,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + VIR_FREE(def->file); for (i = 0; i < def->ndisks; i++) virDomainSnapshotDiskDefClear(&def->disks[i]); VIR_FREE(def->disks); @@ -174,6 +175,9 @@ virDomainSnapshotDefParseString(const char *xmlStr, int active; char *tmp; int keepBlanksDefault = xmlKeepBlanksDefault(0); + char *memorySnapshot = NULL; + char *memoryFile = NULL; + bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -200,15 +204,11 @@ virDomainSnapshotDefParseString(const char *xmlStr, virReportError(VIR_ERR_XML_ERROR, "%s", _("a redefined snapshot must have a name")); goto cleanup; - } else { - ignore_value(virAsprintf(&def->name, "%lld", - (long long)tv.tv_sec)); } - } - - if (def->name == NULL) { - virReportOOMError(); - goto cleanup; + if (virAsprintf(&def->name, "%lld", (long long)tv.tv_sec) < 0) { + virReportOOMError(); + goto cleanup; + } } def->description = virXPathString("string(./description)", ctxt); @@ -239,6 +239,8 @@ virDomainSnapshotDefParseString(const char *xmlStr, state); goto cleanup; } + offline = (def->state == VIR_DOMAIN_SHUTOFF || + def->state == VIR_DOMAIN_DISK_SNAPSHOT); /* Older snapshots were created with just <domain>/<uuid>, and * lack domain/@type. In that case, leave dom NULL, and @@ -266,11 +268,42 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->creationTime = tv.tv_sec; } + memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); + memoryFile = virXPathString("string(./memory/@file)", ctxt); + if (memorySnapshot) { + def->memory = virDomainSnapshotLocationTypeFromString(memorySnapshot); + if (def->memory <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown memory snapshot setting '%s'"), + memorySnapshot); + goto cleanup; + } + if (memoryFile && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + virReportError(VIR_ERR_XML_ERROR, + _("memory filename '%s' requires external snapshot"), + memoryFile); + goto cleanup; + } + } else if (memoryFile) { + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + def->memory = (offline ? + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); + } + if (offline && def->memory && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("memory state cannot be saved with offline snapshot")); + goto cleanup; + } + def->file = memoryFile; + memoryFile = NULL; + if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS || - (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE && - def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { def->ndisks = i; if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { virReportOOMError(); @@ -302,6 +335,8 @@ cleanup: VIR_FREE(creation); VIR_FREE(state); VIR_FREE(nodes); + VIR_FREE(memorySnapshot); + VIR_FREE(memoryFile); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); @@ -523,8 +558,13 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); - /* For now, only output <disks> on disk-snapshot */ - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + if (def->memory) { + virBufferAsprintf(&buf, " <memory snapshot='%s'", + virDomainSnapshotLocationTypeToString(def->memory)); + virBufferEscapeString(&buf, " file='%s'", def->file); + virBufferAddLit(&buf, "/>\n"); + } + if (def->ndisks) { virBufferAddLit(&buf, " <disks>\n"); for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 135fe7c..5390193 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -66,6 +66,9 @@ struct _virDomainSnapshotDef { long long creationTime; /* in seconds */ int state; /* enum virDomainSnapshotState */ + int memory; /* enum virDomainMemorySnapshot */ + char *file; /* memory state file when snapshot is external */ + size_t ndisks; /* should not exceed dom->ndisks */ virDomainSnapshotDiskDef *disks; @@ -93,6 +96,7 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, + VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, diff --git a/tests/domainsnapshotxml2xmlin/external_vm.xml b/tests/domainsnapshotxml2xmlin/external_vm.xml new file mode 100644 index 0000000..3bcd150 --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/external_vm.xml @@ -0,0 +1,10 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <state>running</state> + <memory snapshot='external' file='/dev/HostVG/GuestMemory'/> + <parent> + <name>earlier_snap</name> + </parent> + <creationTime>1272917631</creationTime> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlin/noparent.xml b/tests/domainsnapshotxml2xmlin/noparent.xml new file mode 100644 index 0000000..cbac0d8 --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/noparent.xml @@ -0,0 +1,9 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <state>running</state> + <creationTime>1272917631</creationTime> + <domain> + <uuid>9d37b878-a7cc-9f9a-b78f-49b3abad25a8</uuid> + </domain> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/all_parameters.xml b/tests/domainsnapshotxml2xmlout/all_parameters.xml index eb2ee85..4178ac6 100644 --- a/tests/domainsnapshotxml2xmlout/all_parameters.xml +++ b/tests/domainsnapshotxml2xmlout/all_parameters.xml @@ -6,6 +6,7 @@ <name>earlier_snap</name> </parent> <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> <domain> <uuid>9d37b878-a7cc-9f9a-b78f-49b3abad25a8</uuid> </domain> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index 0a4b179..57aef16 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -6,6 +6,7 @@ <name>earlier_snap</name> </parent> <creationTime>1272917631</creationTime> + <memory snapshot='no'/> <disks> <disk name='hda' snapshot='no'/> <disk name='hdb' snapshot='no'/> diff --git a/tests/domainsnapshotxml2xmlout/external_vm.xml b/tests/domainsnapshotxml2xmlout/external_vm.xml new file mode 100644 index 0000000..8814bce --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/external_vm.xml @@ -0,0 +1,43 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <state>running</state> + <parent> + <name>earlier_snap</name> + </parent> + <creationTime>1272917631</creationTime> + <memory snapshot='external' file='/dev/HostVG/GuestMemory'/> + <disks> + <disk name='hda' snapshot='no'/> + </disks> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <metadata> + <app1:foo xmlns:app1="http://foo.org/">fooish</app1:foo> + <app2:bar xmlns:app2="http://bar.com/" maman="baz">barish</app2:bar> + </metadata> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk' snapshot='no'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + </domain> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/full_domain.xml b/tests/domainsnapshotxml2xmlout/full_domain.xml index 27cf41d..65d1469 100644 --- a/tests/domainsnapshotxml2xmlout/full_domain.xml +++ b/tests/domainsnapshotxml2xmlout/full_domain.xml @@ -6,6 +6,7 @@ <name>earlier_snap</name> </parent> <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> diff --git a/tests/domainsnapshotxml2xmlout/metadata.xml b/tests/domainsnapshotxml2xmlout/metadata.xml index 93c9f39..f961458 100644 --- a/tests/domainsnapshotxml2xmlout/metadata.xml +++ b/tests/domainsnapshotxml2xmlout/metadata.xml @@ -6,6 +6,7 @@ <name>earlier_snap</name> </parent> <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> diff --git a/tests/domainsnapshotxml2xmlout/noparent.xml b/tests/domainsnapshotxml2xmlout/noparent.xml index cbac0d8..0cbbb65 100644 --- a/tests/domainsnapshotxml2xmlout/noparent.xml +++ b/tests/domainsnapshotxml2xmlout/noparent.xml @@ -3,6 +3,7 @@ <description>!@#$%^</description> <state>running</state> <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> <domain> <uuid>9d37b878-a7cc-9f9a-b78f-49b3abad25a8</uuid> </domain> diff --git a/tests/domainsnapshotxml2xmlout/noparent_nodescription.xml b/tests/domainsnapshotxml2xmlout/noparent_nodescription.xml index 0de202d..4eb4016 100644 --- a/tests/domainsnapshotxml2xmlout/noparent_nodescription.xml +++ b/tests/domainsnapshotxml2xmlout/noparent_nodescription.xml @@ -3,5 +3,6 @@ <description>!@#$%^</description> <state>running</state> <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> <active>1</active> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/noparent_nodescription_noactive.xml b/tests/domainsnapshotxml2xmlout/noparent_nodescription_noactive.xml index 25b60f3..94d59a3 100644 --- a/tests/domainsnapshotxml2xmlout/noparent_nodescription_noactive.xml +++ b/tests/domainsnapshotxml2xmlout/noparent_nodescription_noactive.xml @@ -3,4 +3,5 @@ <description>!@#$%^</description> <state>running</state> <creationTime>1272917631</creationTime> + <memory snapshot='no'/> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index e363c99..84278d6 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -110,6 +110,7 @@ mymain(void) DO_TEST("noparent_nodescription", NULL, 1); DO_TEST("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); DO_TEST("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809", 0); + DO_TEST("external_vm", "c7a5fdbd-edaf-9455-926a-d65c16db1809", 0); virCapabilitiesFree(driver.caps); -- 1.7.11.4

Now that the XML supports listing internal snapshots, it is worth always populating the <memory> and <disks> element to match. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Always parse disk info and set memory info. --- src/qemu/qemu_driver.c | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8e8e00c..d5cea59 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11073,8 +11073,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; bool update_current = true; - unsigned int parse_flags = 0; + unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; virDomainSnapshotObjPtr other = NULL; + int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + int align_match = true; virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | @@ -11098,8 +11100,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, update_current = false; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) - parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -11120,6 +11120,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, _("cannot halt after transient domain snapshot")); goto cleanup; } + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) || + !virDomainObjIsActive(vm)) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE; if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, QEMU_EXPECTED_VIRT_TYPES, @@ -11160,6 +11163,15 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* Check that any replacement is compatible */ + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && + def->state != VIR_DOMAIN_DISK_SNAPSHOT) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk-only flag for snapshot %s requires " + "disk-snapshot state"), + def->name); + goto cleanup; + + } if (def->dom && memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) { virReportError(VIR_ERR_INVALID_ARG, @@ -11209,10 +11221,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, other->def = NULL; snap = other; } - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && def->dom) { - if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, - false) < 0) + if (def->dom) { + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + align_match = false; + } + if (virDomainSnapshotAlignDisks(def, align_location, + align_match) < 0) goto cleanup; } } else { @@ -11231,13 +11246,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, "implemented yet")); goto cleanup; } - if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, - false) < 0) - goto cleanup; - if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0) + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + align_match = false; + if (virDomainSnapshotAlignDisks(def, align_location, + align_match) < 0 || + qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0) goto cleanup; def->state = VIR_DOMAIN_DISK_SNAPSHOT; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } else { /* In a perfect world, we would allow qemu to tell us this. * The problem is that qemu only does this check @@ -11248,9 +11264,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, * the boot device. This is probably a bug in qemu, but we'll * work around it here for now. */ - if (!qemuDomainSnapshotIsAllowed(vm)) + if (!qemuDomainSnapshotIsAllowed(vm) || + virDomainSnapshotAlignDisks(def, align_location, + align_match) < 0) goto cleanup; def->state = virDomainObjGetState(vm, NULL); + def->memory = (def->state == VIR_DOMAIN_SHUTOFF ? + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); } } -- 1.7.11.4

Both system checkpoint snapshots and disk snapshots were iterating over all disks, doing a final sanity check before doing any work. But since future patches will allow offline snapshots to be either external or internal, it makes sense to share the pass over all disks, and then relax restrictions in that pass as new modes are implemented. Future patches can then handle external disks when the domain is offline, then handle offline --disk-snapshot, and finally, combine with migration to file to gain a complete external system checkpoint snapshot of an active domain without using 'savevm'. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskPrepare) (qemuDomainSnapshotIsAllowed): Merge... (qemuDomainSnapshotPrepare): ...into one function. (qemuDomainSnapshotCreateXML): Update caller. --- src/qemu/qemu_driver.c | 98 +++++++++++++++++++------------------------------- 1 file changed, 37 insertions(+), 61 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d5cea59..4790752 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10387,34 +10387,6 @@ cleanup: return ret; } -static bool -qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) -{ - int i; - - /* FIXME: we need to figure out what else here might succeed; in - * particular, if it's a raw device but on LVM, we could probably make - * that succeed as well - */ - for (i = 0; i < vm->def->ndisks; i++) { - virDomainDiskDefPtr disk = vm->def->disks[i]; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG || - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) - continue; - - if ((disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) || - (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && - STRNEQ_NULLABLE(disk->driverType, "qcow2"))) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Disk '%s' does not support snapshotting"), - disk->src); - return false; - } - } - - return true; -} static int qemuDomainSnapshotFSFreeze(struct qemud_driver *driver, @@ -10567,8 +10539,8 @@ endjob: } static int -qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, - unsigned int *flags) +qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, + unsigned int *flags) { int ret = -1; int i; @@ -10580,7 +10552,8 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData; - if (allow_reuse && !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && + allow_reuse && !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("reuse is not supported with this QEMU binary")); goto cleanup; @@ -10588,29 +10561,45 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + virDomainDiskDefPtr dom_disk = vm->def->disks[i]; switch (disk->snapshot) { case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: - if (active) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("active qemu domains require external disk " - "snapshots; disk %s requested internal"), - disk->name); - goto cleanup; + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && + dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && + (dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG || + dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) { + found = true; + break; } - if (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + if ((dom_disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) || + (dom_disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && + STRNEQ_NULLABLE(dom_disk->driverType, "qcow2"))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("internal snapshot for disk %s unsupported " "for storage type %s"), disk->name, - NULLSTR(vm->def->disks[i]->driverType)); + NULLSTR(dom_disk->driverType)); + goto cleanup; + } + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && active) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("active qemu domains require external disk " + "snapshots; disk %s requested internal"), + disk->name); goto cleanup; } found = true; break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("system checkpoint external snapshot for " + "disk %s not implemented yet"), + disk->name); + goto cleanup; + } if (!disk->driverType) { if (!(disk->driverType = strdup("qcow2"))) { virReportOOMError(); @@ -10655,11 +10644,11 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, if (!found) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots require at least one disk to be " + _("snapshots require at least one disk to be " "selected for snapshot")); goto cleanup; } - if (active) { + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && active) { if (external == 1 || qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; @@ -10918,7 +10907,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, /* For multiple disks, libvirt must pause externally to get all * snapshots to be at the same point in time, unless qemu supports * transactions. For a single disk, snapshot is atomic without - * requiring a pause. Thanks to qemuDomainSnapshotDiskPrepare, if + * requiring a pause. Thanks to qemuDomainSnapshotPrepare, if * we got to this point, the atomic flag now says whether we need * to pause, and a capability bit says whether to use transaction. */ @@ -10944,7 +10933,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, /* No way to roll back if first disk succeeds but later disks * fail, unless we have transaction support. - * Based on earlier qemuDomainSnapshotDiskPrepare, all + * Based on earlier qemuDomainSnapshotPrepare, all * disks in this list are now either SNAPSHOT_NO, or * SNAPSHOT_EXTERNAL with a valid file name and qcow2 format. */ qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -11248,31 +11237,18 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; - if (virDomainSnapshotAlignDisks(def, align_location, - align_match) < 0 || - qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0) - goto cleanup; def->state = VIR_DOMAIN_DISK_SNAPSHOT; def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } else { - /* In a perfect world, we would allow qemu to tell us this. - * The problem is that qemu only does this check - * device-by-device; so if you had a domain that booted from a - * large qcow2 device, but had a secondary raw device - * attached, you wouldn't find out that you can't snapshot - * your guest until *after* it had spent the time to snapshot - * the boot device. This is probably a bug in qemu, but we'll - * work around it here for now. - */ - if (!qemuDomainSnapshotIsAllowed(vm) || - virDomainSnapshotAlignDisks(def, align_location, - align_match) < 0) - goto cleanup; def->state = virDomainObjGetState(vm, NULL); def->memory = (def->state == VIR_DOMAIN_SHUTOFF ? VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); } + if (virDomainSnapshotAlignDisks(def, align_location, + align_match) < 0 || + qemuDomainSnapshotPrepare(vm, def, &flags) < 0) + goto cleanup; } if (snap) -- 1.7.11.4