Re: [libvirt] [PATCH 1/4] snapshot: new XML for external system checkpoint
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)