On 09/11/2012 08:33 AM, Peter Krempa wrote:
On 09/11/12 02:01, Eric Blake wrote:
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.
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:
Hm, things would be a little cleaner without the DISK_ONLY flag.
Yeah, but we're stuck with back-compat issues where we can't make the flag disappear.
@@ -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; + } }
This hunk doesn't seem to be part of this patch. I'd push it separately.
Will split.
+ if (memoryFile && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + virReportError(VIR_ERR_XML_ERROR, + _("memory filename '%s' requires external snapshot"),
This error message sounds a little bit awkward. I'd write something along "memory filename is supported only with external snapshot".
The other question that comes into my mind is what happens if the user requests an external snapshot but specifies no filename. I suppose you plan hypervisor specific behavior.
The function virDomainSnapshotAlignDisks generates a suitable file name when none was provided; I think a similar function should be used to generate a suitable file name for memory file location if none was provided, if we think we can always come up with a suitable location. Then again, with disks, the filename generation is: take the existing disk name, make sure it is a regular file (if it is a block device, abort), then strip any trailing suffix after '.' and replace it with a new suffix that copies the snapshot name. But with memory, we don't have any starting filename with which to create a new filename by appending a suffix. I don't think we have a suitable location, so I can make this always an error in v2.
Otherwise looks good. ACK.
I'll hold off a bit before pushing, to see how the rest of the series review goes. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org