On 10/23/2012 09:41 PM, Osier Yang wrote:
On 2012年10月23日 23:12, Peter Krempa wrote:
From: Eric Blake<eblake@redhat.com>
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.
@@ -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; + }
Okay, coding style improvements.
When I first posted this patch, I was asked to split this hunk into a separate commit. Consider that done.
+ } + if (offline&& def->memory&&
I think def->memory checking is redundant here. Not big deal though.
+ def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {
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.
+ virReportError(VIR_ERR_XML_ERROR, "%s", + _("memory state cannot be saved with offline snapshot"));
"VM state" is used both in the commit message and docs. Better to keep consistent, I prefer "memory state" more, as it's more clear. "VM state" could include disk state too. Correct me if I'm not right.
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.
Others looks just very sanity, ACK.
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org