[libvirt] [PATCH 1/4] snapshot: new XML for external system checkpoint

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120911/42abda6d/attachment-0001.sig>