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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list