From: Eric Blake <eblake(a)redhat.com&gt; 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 ecaafe9..45d55b5 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 16c844d..6f77026 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -94,6 +94,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); @@ -182,6 +183,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) { @@ -208,15 +212,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); @@ -247,6 +247,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 @@ -274,11 +276,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(); @@ -310,6 +343,8 @@ cleanup: VIR_FREE(creation); VIR_FREE(state); VIR_FREE(nodes); + VIR_FREE(memorySnapshot); + VIR_FREE(memoryFile); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); @@ -531,8 +566,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 c00347f..00775ae 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.12.4

On 10/23/2012 09:41 PM, Osier Yang wrote: When I first posted this patch, I was asked to split this hunk into a separate commit. Consider that done. Here, def->memory==0==VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT is different than an explicit 'none' (value 1). The check is necessary for back-compat, since the absence of <memory> in the snapshot XML must do the same behavior as always, and that behavior differed for checkpoint vs. disk-only snapshots. VM state includes both memory _and_ device state; but then you could argue that device state is just a special subset of memory state. Sure, I'll do that rewording. Thanks for the review, and thanks to Peter for helping with the rebase work. I'll push this one shortly, while I work on reviewing Peter's patches later in the series. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Eric Blake <eblake(a)redhat.com&gt; There were not previous callers with require_match set to true. I originally implemented this bool with the intent of supporting ESX snapshot semantics, where the choice of internal vs. external vs. non-checkpointable must be made at domain start, but as ESX has not been wired up to use it yet, we might as well fix it to work with our next qemu patch for now, and worry about any further improvements (changing the bool to a flags argument) if the ESX driver decides to use this function in the future. * src/conf/snapshot_conf.c (virDomainSnapshotAlignDisks): Alter logic when require_match is true to deal with new XML. --- src/conf/snapshot_conf.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 6f77026..691950a 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -368,9 +368,8 @@ disksorter(const void *a, const void *b) * the domain, with a fallback to a passed in default. Convert paths * to disk targets for uniformity. Issue an error and return -1 if * any def->disks[n]->name appears more than once or does not map to - * dom->disks. If require_match, also require that existing - * def->disks snapshot states do not override explicit def->dom - * settings. */ + * dom->disks. If require_match, also ensure that there is no + * conflicting requests for both internal and external snapshots. */ int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, int default_snapshot, @@ -416,7 +415,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, _("no disk named '%s'"), disk->name); goto cleanup; } - disk_snapshot = def->dom->disks[idx]->snapshot; if (virBitmapGetBit(map, idx, &inuse) < 0 || inuse) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -426,15 +424,22 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, } ignore_value(virBitmapSetBit(map, idx)); disk->index = idx; - if (!disk_snapshot) - disk_snapshot = default_snapshot; + + disk_snapshot = def->dom->disks[idx]->snapshot; if (!disk->snapshot) { - disk->snapshot = disk_snapshot; - } else if (disk_snapshot && require_match && - disk->snapshot != disk_snapshot) { + if (disk_snapshot && + (!require_match || + disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) + disk->snapshot = disk_snapshot; + else + disk->snapshot = default_snapshot; + } else if (require_match && + disk->snapshot != default_snapshot && + !(disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && + disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) { const char *tmp; - tmp = virDomainSnapshotLocationTypeToString(disk_snapshot); + tmp = virDomainSnapshotLocationTypeToString(default_snapshot); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' must use snapshot mode '%s'"), disk->name, tmp); -- 1.7.12.4

From: Eric Blake <eblake(a)redhat.com&gt; 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 8af316f..cbabd62 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11159,8 +11159,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 | @@ -11184,8 +11186,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); @@ -11206,6 +11206,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, @@ -11246,6 +11249,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, @@ -11295,10 +11307,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 { @@ -11317,13 +11332,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 @@ -11334,9 +11350,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.12.4

From: Eric Blake <eblake(a)redhat.com&gt; 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 | 92 +++++++++++++++++++------------------------------- 1 file changed, 34 insertions(+), 58 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cbabd62..df02783 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10516,34 +10516,6 @@ cleanup: } -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 && - disk->format > 0 && disk->format != VIR_STORAGE_FILE_QCOW2)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Disk '%s' does not support snapshotting"), - disk->src); - return false; - } - } - - return true; -} /* this function expects the driver lock to be held by the caller */ static int @@ -10697,8 +10669,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; @@ -10710,7 +10682,8 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData; - if (reuse && !qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && + reuse && !qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("reuse is not supported with this QEMU binary")); goto cleanup; @@ -10718,15 +10691,16 @@ 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]->format > 0 && vm->def->disks[i]->format != VIR_STORAGE_FILE_QCOW2) { @@ -10738,10 +10712,25 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, vm->def->disks[i]->format)); 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->format) { disk->format = VIR_STORAGE_FILE_QCOW2; } else if (disk->format != VIR_STORAGE_FILE_QCOW2 && @@ -10789,11 +10778,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->caps, QEMU_CAPS_TRANSACTION)) { *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; @@ -11004,7 +10993,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. */ @@ -11030,7 +11019,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); @@ -11334,31 +11323,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.12.4

The code that saves domain memory by migration to file can be reused while doing external snapshots of a machine. This patch extracts the common code and places it in a separate function. --- src/qemu/qemu_driver.c | 225 ++++++++++++++++++++++++++++--------------------- 1 file changed, 127 insertions(+), 98 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df02783..025633f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2605,7 +2605,7 @@ bswap_header(struct qemud_save_header *hdr) { /* return -errno on failure, or 0 on success */ static int -qemuDomainSaveHeader(int fd, const char *path, char *xml, +qemuDomainSaveHeader(int fd, const char *path, const char *xml, struct qemud_save_header *header) { int ret = 0; @@ -2752,98 +2752,38 @@ cleanup: return fd; } -/* This internal function expects the driver lock to already be held on - * entry and the vm must be active + locked. Vm will be unlocked and - * potentially free'd after this returns (eg transient VMs are freed - * shutdown). So 'vm' must not be referenced by the caller after - * this returns (whether returning success or failure). - */ +/* Helper function to execute a migration to file with a correct save header + * the caller needs to make sure that the processors are stopped and do all other + * actions besides saving memory */ static int -qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, - virDomainObjPtr vm, const char *path, - int compressed, const char *xmlin, unsigned int flags) +qemuDomainSaveMemory(struct qemud_driver *driver, + virDomainObjPtr vm, + const char *path, + const char *xml, + int compressed, + bool was_running, + unsigned int flags, + enum qemuDomainAsyncJob asyncJob) { - char *xml = NULL; struct qemud_save_header header; bool bypassSecurityDriver = false; - int ret = -1; - int rc; - virDomainEventPtr event = NULL; - qemuDomainObjPrivatePtr priv; bool needUnlink = false; - size_t len; - unsigned long long offset; - unsigned long long pad; + int ret = -1; int fd = -1; int directFlag = 0; virFileWrapperFdPtr wrapperFd = NULL; unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; - - if (qemuProcessAutoDestroyActive(driver, vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is marked for auto destroy")); - goto cleanup; - } + unsigned long long pad; + unsigned long long offset; + size_t len; memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_PARTIAL, sizeof(header.magic)); header.version = QEMUD_SAVE_VERSION; + header.was_running = was_running ? 1 : 0; header.compressed = compressed; - priv = vm->privateData; - - if (qemuDomainObjBeginAsyncJobWithDriver(driver, vm, - QEMU_ASYNC_JOB_SAVE) < 0) - goto cleanup; - - memset(&priv->job.info, 0, sizeof(priv->job.info)); - priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED; - - /* Pause */ - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - header.was_running = 1; - if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, - QEMU_ASYNC_JOB_SAVE) < 0) - goto endjob; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto endjob; - } - } - /* libvirt.c already guaranteed these two flags are exclusive. */ - if (flags & VIR_DOMAIN_SAVE_RUNNING) - header.was_running = 1; - else if (flags & VIR_DOMAIN_SAVE_PAUSED) - header.was_running = 0; - - /* Get XML for the domain. Restore needs only the inactive xml, - * including secure. We should get the same result whether xmlin - * is NULL or whether it was the live xml of the domain moments - * before. */ - if (xmlin) { - virDomainDefPtr def = NULL; - - if (!(def = virDomainDefParseString(driver->caps, xmlin, - QEMU_EXPECTED_VIRT_TYPES, - VIR_DOMAIN_XML_INACTIVE))) { - goto endjob; - } - if (!virDomainDefCheckABIStability(vm->def, def)) { - virDomainDefFree(def); - goto endjob; - } - xml = qemuDomainDefFormatLive(driver, def, true, true); - } else { - xml = qemuDomainDefFormatLive(driver, vm->def, true, true); - } - if (!xml) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("failed to get domain xml")); - goto endjob; - } len = strlen(xml) + 1; offset = sizeof(header) + len; @@ -2860,7 +2800,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, ((offset + pad) % QEMU_MONITOR_MIGRATE_TO_FILE_BS)); if (VIR_EXPAND_N(xml, len, pad) < 0) { virReportOOMError(); - goto endjob; + goto cleanup; } offset += pad; header.xml_len = len; @@ -2878,22 +2818,21 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, fd = qemuOpenFile(driver, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag, &needUnlink, &bypassSecurityDriver); if (fd < 0) - goto endjob; + goto cleanup; + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) - goto endjob; + goto cleanup; /* Write header to file, followed by XML */ - if (qemuDomainSaveHeader(fd, path, xml, &header) < 0) { - VIR_FORCE_CLOSE(fd); - goto endjob; - } + if (qemuDomainSaveHeader(fd, path, xml, &header) < 0) + goto cleanup; /* Perform the migration */ if (qemuMigrationToFile(driver, vm, fd, offset, path, qemuCompressProgramName(compressed), bypassSecurityDriver, - QEMU_ASYNC_JOB_SAVE) < 0) - goto endjob; + asyncJob) < 0) + goto cleanup; /* Touch up file header to mark image complete. */ @@ -2903,26 +2842,120 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, * that's acceptable. */ if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); - goto endjob; + goto cleanup; } + if (virFileWrapperFdClose(wrapperFd) < 0) - goto endjob; - fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL); - if (fd < 0) - goto endjob; + goto cleanup; + + if ((fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL)) < 0) + goto cleanup; memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); + if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { virReportSystemError(errno, _("unable to write %s"), path); - goto endjob; + goto cleanup; } + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); - goto endjob; + goto cleanup; } ret = 0; +cleanup: + VIR_FORCE_CLOSE(fd); + virFileWrapperFdFree(wrapperFd); + + if (ret != 0 && needUnlink) + unlink(path); + + return ret; +} + +/* This internal function expects the driver lock to already be held on + * entry and the vm must be active + locked. Vm will be unlocked and + * potentially free'd after this returns (eg transient VMs are freed + * shutdown). So 'vm' must not be referenced by the caller after + * this returns (whether returning success or failure). + */ +static int +qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, + virDomainObjPtr vm, const char *path, + int compressed, const char *xmlin, unsigned int flags) +{ + char *xml = NULL; + bool was_running = false; + int ret = -1; + int rc; + virDomainEventPtr event = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (qemuProcessAutoDestroyActive(driver, vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is marked for auto destroy")); + goto cleanup; + } + + if (qemuDomainObjBeginAsyncJobWithDriver(driver, vm, + QEMU_ASYNC_JOB_SAVE) < 0) + + memset(&priv->job.info, 0, sizeof(priv->job.info)); + priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED; + + /* Pause */ + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + was_running = true; + if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, + QEMU_ASYNC_JOB_SAVE) < 0) + goto endjob; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + } + } + + /* libvirt.c already guaranteed these two flags are exclusive. */ + if (flags & VIR_DOMAIN_SAVE_RUNNING) + was_running = true; + else if (flags & VIR_DOMAIN_SAVE_PAUSED) + was_running = false; + + /* Get XML for the domain. Restore needs only the inactive xml, + * including secure. We should get the same result whether xmlin + * is NULL or whether it was the live xml of the domain moments + * before. */ + if (xmlin) { + virDomainDefPtr def = NULL; + + if (!(def = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) { + goto endjob; + } + if (!virDomainDefCheckABIStability(vm->def, def)) { + virDomainDefFree(def); + goto endjob; + } + xml = qemuDomainDefFormatLive(driver, def, true, true); + } else { + xml = qemuDomainDefFormatLive(driver, vm->def, true, true); + } + if (!xml) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("failed to get domain xml")); + goto endjob; + } + + ret = qemuDomainSaveMemory(driver, vm, path, xml, compressed, + was_running, flags, QEMU_ASYNC_JOB_SAVE); + if (ret < 0) + goto endjob; + /* Shut it down */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0); virDomainAuditStop(vm, "saved"); @@ -2938,7 +2971,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, endjob: if (vm) { if (ret != 0) { - if (header.was_running && virDomainObjIsActive(vm)) { + if (was_running && virDomainObjIsActive(vm)) { rc = qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_SAVE_CANCELED, QEMU_ASYNC_JOB_SAVE); @@ -2951,11 +2984,7 @@ endjob: } cleanup: - VIR_FORCE_CLOSE(fd); - virFileWrapperFdFree(wrapperFd); VIR_FREE(xml); - if (ret != 0 && needUnlink) - unlink(path); if (event) qemuDomainEventQueue(driver, event); if (vm) -- 1.7.12.4

The default behavior while creating external checkpoints is to let the guest run while the memory state is caputred. This leads to a larger save file but minimizes the time needed to take the checkpoint. This patch adds a flag that causes the guest to be paused before taking the snapshot. *include/libvirt/libvirt.h.in: - add new paused reason: VIR_DOMAIN_PAUSED_SNAPSHOT - add new flag for takin snapshot: VIR_DOMAIN_SNAPSHOT_CREATE_PAUSE *tools/virsh-domain-monitor.c: - add string representation for VIR_DOMAIN_PAUSED_SNAPSHOT *tools/virsh-snapshot.c: - add support for VIR_DOMAIN_SNAPSHOT_CREATE_PAUSE *tools/virsh.pod: - add docs for --pause option added to use VIR_DOMAIN_SNAPSHOT_CREATE_PAUSE flag --- include/libvirt/libvirt.h.in | 4 ++++ tools/virsh-domain-monitor.c | 2 ++ tools/virsh-snapshot.c | 6 ++++++ tools/virsh.pod | 12 ++++++++++-- 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 52555f8..815a491 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -179,6 +179,7 @@ typedef enum { VIR_DOMAIN_PAUSED_WATCHDOG = 6, /* paused due to a watchdog event */ VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* paused after restoring from snapshot */ VIR_DOMAIN_PAUSED_SHUTTING_DOWN = 8, /* paused during shutdown process */ + VIR_DOMAIN_PAUSED_SNAPSHOT = 9, /* paused while creating a snaphot */ #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_PAUSED_LAST @@ -3673,6 +3674,9 @@ typedef enum { the domain */ VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC = (1 << 7), /* atomically avoid partial changes */ + VIR_DOMAIN_SNAPSHOT_CREATE_PAUSE = (1 << 8), /* pause the guest + before taking + checkpoint */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b264f15..faf23fe 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -223,6 +223,8 @@ vshDomainStateReasonToString(int state, int reason) return N_("from snapshot"); case VIR_DOMAIN_PAUSED_SHUTTING_DOWN: return N_("shutting down"); + case VIR_DOMAIN_PAUSED_SNAPSHOT: + return N_("creating snapshot"); case VIR_DOMAIN_PAUSED_UNKNOWN: case VIR_DOMAIN_PAUSED_LAST: ; diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 53de2b3..895cd42 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -127,6 +127,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")}, {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")}, {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")}, + {"pause", VSH_OT_BOOL, 0, N_("pause guest before taking snapshot")}, {NULL, 0, 0, NULL} }; @@ -155,6 +156,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE; if (vshCommandOptBool(cmd, "atomic")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; + if (vshCommandOptBool(cmd, "pause")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_PAUSE; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) @@ -263,6 +266,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")}, {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")}, {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")}, + {"pause", VSH_OT_BOOL, 0, N_("pause guest before taking snapshot")}, {"diskspec", VSH_OT_ARGV, 0, N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")}, {NULL, 0, 0, NULL} @@ -292,6 +296,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE; if (vshCommandOptBool(cmd, "atomic")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; + if (vshCommandOptBool(cmd, "pause")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_PAUSE; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) diff --git a/tools/virsh.pod b/tools/virsh.pod index 8a30ce2..09ad8c1 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2589,7 +2589,7 @@ used to represent properties of snapshots. =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] -[I<--quiesce>] [I<--atomic>]} +[I<--quiesce>] [I<--atomic>] [I<--pause>]} Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. Normally, the only properties settable for a domain snapshot @@ -2642,6 +2642,10 @@ this. If this flag is not specified, then some hypervisors may fail after partially performing the action, and B<dumpxml> must be used to see whether any partial changes occurred. +If I<--pause> is specified, libvirt pauses the guest before taking the +snapshot to decrease size of the memory image in case of external +checkpoints. Afterwards the guest is resumed. + Existence of snapshot metadata will prevent attempts to B<undefine> a persistent domain. However, for transient domains, snapshot metadata is silently lost when the domain quits running (whether @@ -2650,7 +2654,7 @@ by command such as B<destroy> or by internal guest action). =item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>] [I<--halt>] [I<--reuse-external>]} [I<name>] [I<description>] [I<--disk-only> [I<--quiesce>] [I<--atomic>] -[[I<--diskspec>] B<diskspec>]...] +[I<--pause>] [[I<--diskspec>] B<diskspec>]...] Create a snapshot for domain I<domain> with the given <name> and <description>; if either value is omitted, libvirt will choose a @@ -2695,6 +2699,10 @@ this. If this flag is not specified, then some hypervisors may fail after partially performing the action, and B<dumpxml> must be used to see whether any partial changes occurred. +If I<--pause> is specified, libvirt pauses the guest before taking the +snapshot to decrease size of the memory image in case of external +checkpoints. Afterwards the guest is resumed. + =item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info>] | [I<snapshotname>]} -- 1.7.12.4

On 10/23/2012 09:12 AM, Peter Krempa wrote: I'd rather see the case labels with _NONE and _LAST at the bottom; float this up to be next to _DUMP. Ditto. Otherwise, looks reasonable. I'll have to see later on if there are any race cases. Remember, if the _LIVE flag is not used, we pause up front; but when the _LIVE flag IS used, qemu will pause itself when the migration completes; and since this is all about state recovery when libvirtd restarts, it is feasible that the migration completed instead of being canceled, and that the reason we see the guest paused is because qemu left it that way rather than us explicitly pausing it. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 10/23/2012 09:12 AM, Peter Krempa wrote: Outdated comment, but looks like you've touched that up on your git tree. The guest will pause either because _LIVE was not present (we paused up front), or because qemu will pause the domain when live migration converges. Then, while the guest is paused, we take the disk snapshot, then resume the guest, and voila! external snapshot! Hmm, wonder if we should document somewhere that: virDomainSave(dom, "/path/to/file") is now shorthand for: virDomainSnapshotCreate(dom, " <domainsnapshot> <memory file='/path/to/file'/> <disks> <disk name='vda' snapshot='no'/> <!-- etc. for each disk --> </disks> </domainsnapshot> ", VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) It also makes me wonder if virDomainSaveFlags should learn a _LIVE flag. Consider what happens when you save domain memory but not disks, and then let the domain continue running - since you didn't snapshot disks, then any modification made to the disks will invalidate the saved memory. That said, it is possible to create a domain that only uses read-only disks and stores everything in tmpfs. In fact, one of my favorite test cases while working on block-commit was a domain based on a live iso image - precisely because I can then attach any number (including 0) of other disks which won't be touched by the live OS, at which point you really can make a case for a live ram-only snapshot. Hmm, here we are starting an async job that can be canceled any time we drop locks. But the biggest part of this job is waiting for the migration to file to complete; once that part is done, we will drop locks several more times while issuing a 'transaction' or a series of disk snapshot monitor commands, and I don't think the cancellation code is prepared to handle that case. I wonder if we need to separate this into two jobs - one async to do the migration to file, and one sync to do the disk snapshots, so that we only allow cancellation during the first half (the second half is fast enough that we aren't starving access to the domain in the process). I haven't checked closely - does this new async job allow the user to pause and continue the guest at will? I know with live migration we allow the client to pause the guest if they are tired of waiting on things to converge (in fact, virsh even has a migrate option that auto-pauses the guest if things take too long). Hmm, while I know we can pause a live migration, what I don't know is if we allow resuming a guest while migration is ongoing. I'm worried that we are missing the interaction that allows a user to pause a live snapshot if things aren't converging fast enough. I would be okay if our design is that, for the initial implementation, we reject pause and resume attempts during a migration, and add that ability later, but it is something we need to keep in mind. And it does make the question of whether to resume a bit trickier - it is no longer a matter of whether the guest was running before we started the snapshot, but also whether the guest has been paused later on during a live snapshot. Similar to live migration, I'm assuming that it is always safe to pause, but that once things are paused, we probably want to reject resuming until the snapshot operation completes. I looked in your tree to see how you changed things for the _LIVE flag: + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE) || + (!memory && atomic && !transaction)) { That's not quite the right logic. You want to pause if there is a memory snapshot but no _LIVE flag, or if there are multiple disks but no transaction support. Something like: if ((memory && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE)) || (atomic && !transaction)) Be aware that at this point, even if _LIVE was specified, the domain is now paused (qemu pauses the domain once things converge) - you may need to account for this later. Yep, should have read further - the comment looks right. s/PAUSE/LIVE/, obviously We should fail internal checkpoint if _LIVE is present, since qemu's 'savevm' does not support a live mode. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Two other places were left with the old code to look up snapshots. Change them to use the snapshot lookup helper. --- src/qemu/qemu_driver.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 51b0391..287e2f8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11873,13 +11873,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); - if (!snap) { - virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, - _("no domain snapshot with matching name '%s'"), - snapshot->name); + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) goto cleanup; - } if (!vm->persistent && snap->def->state != VIR_DOMAIN_RUNNING && @@ -12241,13 +12236,8 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); - if (!snap) { - virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, - _("no domain snapshot with matching name '%s'"), - snapshot->name); + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) goto cleanup; - } if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY)) { if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && -- 1.7.12.4

The current snapshot reverting api supported changing the state of the machine after the snapshot was reverted to either started or paused. This patch adds the ability to revert the state but to stopped state. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 9 +++++---- tools/virsh-snapshot.c | 3 +++ tools/virsh.pod | 9 +++++---- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 815a491..ec03e4f 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3775,6 +3775,7 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */ VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */ + VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED = 1 << 3, /* Revert into stopped state */ } virDomainSnapshotRevertFlags; /* Revert the domain to a point-in-time snapshot. The diff --git a/src/libvirt.c b/src/libvirt.c index 33cf7cb..2d09eb0 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18691,11 +18691,12 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto error; } - if ((flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && - (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + if ((!!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) + + !!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) + + !!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED)) > 1) { virReportInvalidArg(flags, - _("running and paused flags in %s are mutually exclusive"), - __FUNCTION__); + _("running, paused and stopped flags in %s are " + "mutually exclusive"), __FUNCTION__); goto error; } diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 895cd42..b48faa3 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1467,6 +1467,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = { {"current", VSH_OT_BOOL, 0, N_("revert to current snapshot")}, {"running", VSH_OT_BOOL, 0, N_("after reverting, change state to running")}, {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")}, + {"stopped", VSH_OT_BOOL, 0, N_("after reverting, change state to stopped")}, {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")}, {NULL, 0, 0, NULL} }; @@ -1486,6 +1487,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING; if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED; + if (vshCommandOptBool(cmd, "stopped")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED; /* We want virsh snapshot-revert --force to work even when talking * to older servers that did the unsafe revert by default but * reject the flag, so we probe without the flag, and only use it diff --git a/tools/virsh.pod b/tools/virsh.pod index 09ad8c1..430b36e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2796,7 +2796,7 @@ Output the name of the parent snapshot, if any, for the given I<snapshot>, or for the current snapshot with I<--current>. =item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} -[{I<--running> | I<--paused>}] [I<--force>] +[{I<--running> | I<--paused>} | I<--stopped>] [I<--force>] Revert the given domain to the snapshot specified by I<snapshot>, or to the current snapshot with I<--current>. Be aware @@ -2807,9 +2807,10 @@ the original snapshot was taken. Normally, reverting to a snapshot leaves the domain in the state it was at the time the snapshot was created, except that a disk snapshot with -no vm state leaves the domain in an inactive state. Passing either the -I<--running> or I<--paused> flag will perform additional state changes -(such as booting an inactive domain, or pausing a running domain). Since +no vm state leaves the domain in an inactive state. Passing one of the +I<--running>, I<--paused> or I<--stopped> flag will perform additional +state changes such as booting an inactive domain, pausing a running domain +or shutting the domain down after the snapshot is reverted. Since transient domains cannot be inactive, it is required to use one of these flags when reverting to a disk snapshot of a transient domain. -- 1.7.12.4

This patch adds support for external disk snapshots of inactive domains. The snapshot is created by calling qemu-img create -o backing_file=/path/to/disk /path/snapshot_file on each of the disks selected for snapshotting. --- src/qemu/qemu_driver.c | 108 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 97 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ff3c57f..2ac079b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10627,13 +10627,100 @@ qemuDomainSnapshotFSThaw(struct qemud_driver *driver, /* The domain is expected to be locked and inactive. */ static int -qemuDomainSnapshotCreateInactive(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap) +qemuDomainSnapshotCreateInactiveInternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) { return qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-c", false); } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool reuse) +{ + const char *qemuimgarg[] = { NULL, "create", "-f", NULL, "-o", NULL, NULL, NULL }; + int i = 0; + char *backingFileArg = NULL; + virDomainSnapshotDiskDefPtr snapdisk; + virDomainDiskDefPtr defdisk; + + int ret = -1; + + qemuimgarg[0] = qemuFindQemuImgBinary(driver); + if (qemuimgarg[0] == NULL) { + /* qemuFindQemuImgBinary set the error */ + return -1; + } + + for (i = 0; i < snap->def->ndisks; i++) { + snapdisk = &(snap->def->disks[i]); + defdisk = snap->def->dom->disks[snapdisk->index]; + + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + VIR_FREE(backingFileArg); + + /* remove old disk image */ + if (reuse && virFileExists(snapdisk->file) && + unlink(snapdisk->file) < 0) { + virReportSystemError(errno, + _("Failed to remove image file '%s'"), + snapdisk->file); + goto cleanup; + } + + if (virAsprintf(&backingFileArg, "backing_file=%s", defdisk->src) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!snapdisk->format) + snapdisk->format = VIR_STORAGE_FILE_QCOW2; + + qemuimgarg[3] = virStorageFileFormatTypeToString(snapdisk->format); + qemuimgarg[5] = backingFileArg; + qemuimgarg[6] = snapdisk->file; + + if (virRun(qemuimgarg, NULL) < 0) + goto cleanup; + } + } + + /* update disk definitions */ + for (i = 0; i < snap->def->ndisks; i++) { + snapdisk = &(snap->def->disks[i]); + defdisk = vm->def->disks[snapdisk->index]; + + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + VIR_FREE(defdisk->src); + if (!(defdisk->src = strdup(snapdisk->file))) { + virReportOOMError(); + goto cleanup; + } + defdisk->format = snapdisk->format; + } + } + + ret = 0; + +cleanup: + /* unlink images if creation has failed */ + if (ret < 0 && i > 0) { + for (; i > 0; i--) { + snapdisk = &(snap->def->disks[i]); + if (unlink(snapdisk->file) < 0) + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->file); + } + } + + VIR_FREE(backingFileArg); + + return ret; +} + /* The domain is expected to be locked and active. */ static int @@ -11412,12 +11499,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots of inactive domains not " - "implemented yet")); - goto cleanup; - } align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; def->state = VIR_DOMAIN_DISK_SNAPSHOT; @@ -11480,8 +11561,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } } else { /* inactive */ - if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) - goto cleanup; + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, false) < 0) + goto cleanup; + } else { + if (qemuDomainSnapshotCreateInactiveInternal(driver, vm, snap) < 0) + goto cleanup; + } } /* If we fail after this point, there's not a whole lot we can -- 1.7.12.4

Some of the functions were moved to other files but the private symbol file wasn't tweaked to reflect that. --- src/libvirt_private.syms | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 60f9c7f..5f591f3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -422,6 +422,7 @@ virDomainLifecycleCrashTypeFromString; virDomainLifecycleCrashTypeToString; virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; +virDomainList; virDomainLiveConfigHelperMethod; virDomainLoadAllConfigs; virDomainLockFailureTypeFromString; @@ -494,24 +495,6 @@ virDomainSmartcardTypeFromString; virDomainSmartcardTypeToString; virDomainSmbiosModeTypeFromString; virDomainSmbiosModeTypeToString; -virDomainSnapshotAlignDisks; -virDomainSnapshotAssignDef; -virDomainSnapshotDefFormat; -virDomainSnapshotDefFree; -virDomainSnapshotDefParseString; -virDomainSnapshotDropParent; -virDomainSnapshotFindByName; -virDomainSnapshotForEach; -virDomainSnapshotForEachChild; -virDomainSnapshotForEachDescendant; -virDomainSnapshotLocationTypeFromString; -virDomainSnapshotLocationTypeToString; -virDomainSnapshotObjListGetNames; -virDomainSnapshotObjListNum; -virDomainSnapshotObjListRemove; -virDomainSnapshotStateTypeFromString; -virDomainSnapshotStateTypeToString; -virDomainSnapshotUpdateRelations; virDomainSoundDefFree; virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; @@ -1084,6 +1067,29 @@ sexpr_u64; sexpr2string; string2sexpr; + +# snapshot_conf.h +virDomainListSnapshots; +virDomainSnapshotAlignDisks; +virDomainSnapshotAssignDef; +virDomainSnapshotDefFormat; +virDomainSnapshotDefFree; +virDomainSnapshotDefParseString; +virDomainSnapshotDropParent; +virDomainSnapshotFindByName; +virDomainSnapshotForEach; +virDomainSnapshotForEachChild; +virDomainSnapshotForEachDescendant; +virDomainSnapshotLocationTypeFromString; +virDomainSnapshotLocationTypeToString; +virDomainSnapshotObjListGetNames; +virDomainSnapshotObjListNum; +virDomainSnapshotObjListRemove; +virDomainSnapshotStateTypeFromString; +virDomainSnapshotStateTypeToString; +virDomainSnapshotUpdateRelations; + + # storage_conf.h virStoragePartedFsTypeTypeToString; virStoragePoolDefFormat; @@ -1323,11 +1329,6 @@ virConsoleOpen; virDBusGetSystemBus; -# virdomainlist.h -virDomainList; -virDomainListSnapshots; - - # virfile.h virFileLoopDeviceAssociate; virFileClose; -- 1.7.12.4

This patch adds an event to be raised if a PM suspended guest is restored from a snapshot. --- examples/domain-events/events-c/event-test.c | 3 +++ include/libvirt/libvirt.h.in | 2 ++ 2 files changed, 5 insertions(+) diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 39bea49..f0e3fde 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -204,6 +204,9 @@ static const char *eventDetailToString(int event, int detail) { case VIR_DOMAIN_EVENT_PMSUSPENDED_DISK: ret = "Disk"; break; + case VIR_DOMAIN_EVENT_PMSUSPENDED_FROM_SNAPSHOT: + ret = "from snapshot"; + break; } break; } diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index ec03e4f..91cae71 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3124,6 +3124,8 @@ typedef enum { typedef enum { VIR_DOMAIN_EVENT_PMSUSPENDED_MEMORY = 0, /* Guest was PM suspended to memory */ VIR_DOMAIN_EVENT_PMSUSPENDED_DISK = 1, /* Guest was PM suspended to disk */ + VIR_DOMAIN_EVENT_PMSUSPENDED_FROM_SNAPSHOT = 2, /* Snapshot of a machine + in S3/S4 was reverted */ #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_PMSUSPENDED_LAST -- 1.7.12.4

This patch adds limited support for deleting external snaphots. The machine must not be active and only whole subtrees of snapshots can be deleted as reparenting was not yet implemented for external snapshots. --- src/qemu/qemu_domain.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_driver.c | 23 ++++++++-- 2 files changed, 135 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3642bc1..ecf2b2d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1731,13 +1731,102 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver, op, try_all, def->ndisks); } -/* Discard one snapshot (or its metadata), without reparenting any children. */ -int -qemuDomainSnapshotDiscard(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap, - bool update_current, - bool metadata_only) +/* Discard one external snapshot (or its metadata), without reparenting any children */ +static int +qemuDomainSnapshotDiscardExternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool update_current, + bool metadata_only) +{ + char *snapFile = NULL; + int i; + int ret = -1; + virDomainSnapshotObjPtr parentsnap = NULL; + + if (!metadata_only) { + if (virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Can't delete snapshot '%s'. Domain '%s' is active."), + snap->def->name, vm->def->name); + goto cleanup; + } + + if (snap->def->file) { + if (unlink(snap->def->file) < 0) { + virReportSystemError(errno, + _("Failed to remove memory checkpoint " + "save file '%s'"), snap->def->file); + goto cleanup; + } + VIR_FREE(snap->def->file); + } + + for (i = 0; i < snap->def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &(snap->def->disks[i]); + if (disk->file && unlink(disk->file) < 0) { + virReportSystemError(errno, + _("Failed to remove disk snapshot file '%s'"), + disk->file); + goto cleanup; + } + VIR_FREE(disk->file); + } + } + + if (snap == vm->current_snapshot) { + if (update_current && snap->def->parent) { + parentsnap = virDomainSnapshotFindByName(vm->snapshots, + snap->def->parent); + if (!parentsnap) { + VIR_WARN("missing parent snapshot matching name '%s'", + snap->def->parent); + } else { + parentsnap->def->current = true; + + if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, + driver->snapshotDir) < 0) { + VIR_WARN("failed to set parent snapshot '%s' as current", + snap->def->parent); + parentsnap->def->current = false; + parentsnap = NULL; + } + } + } + vm->current_snapshot = parentsnap; + } + + if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, + vm->def->name, snap->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (unlink(snapFile) < 0) + VIR_WARN("Failed to unlink %s", snapFile); + virDomainSnapshotObjListRemove(vm->snapshots, snap); + + ret = 0; + +cleanup: + if (ret < 0 && !snapFile && + qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0) + VIR_WARN("Failed to store modified snapshot config"); + + VIR_FREE(snapFile); + + return ret; +} + +/* Discard one internal snapshot (or its metadata), + * without reparenting any children. + */ +static int +qemuDomainSnapshotDiscardInternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool update_current, + bool metadata_only) { char *snapFile = NULL; int ret = -1; @@ -1798,6 +1887,25 @@ cleanup: return ret; } +/* Discard one snapshot (or its metadata), without reparenting any children. */ +int +qemuDomainSnapshotDiscard(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool update_current, + bool metadata_only) +{ + int ret; + + if (virDomainSnapshotIsExternal(snap)) + ret = qemuDomainSnapshotDiscardExternal(driver, vm, snap, update_current, metadata_only); + else + ret = qemuDomainSnapshotDiscardInternal(driver, vm, snap, update_current, metadata_only); + + return ret; +} + + /* Hash iterator callback to discard multiple snapshots. */ void qemuDomainSnapshotDiscardAll(void *payload, const void *name ATTRIBUTE_UNUSED, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02bffb4..196004a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12295,6 +12295,13 @@ qemuDomainSnapshotReparentChildren(void *payload, return; } + if (virDomainSnapshotIsExternal(snap)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Reparenting of external snapshots is not implemented")); + rep->err = -1; + return; + } + VIR_FREE(snap->def->parent); snap->parent = rep->parent; @@ -12352,10 +12359,20 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, virDomainSnapshotForEachDescendant(snap, qemuDomainSnapshotCountExternal, &external); - if (external) { + if (external && + virDomainObjIsActive(vm)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("deletion of %d external disk snapshots not " - "supported yet"), external); + _("deletion of %d external disk snapshots is " + "supported only on inactive guests"), external); + goto cleanup; + } + + if (external && + !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only deletion of external snapshot subtrees is " + "implemented")); goto cleanup; } } -- 1.7.12.4