On 10/23/2012 09:41 PM, Osier Yang wrote:
> On 2012年10月23日 23:12, Peter Krempa wrote:
>> From: Eric Blake<eblake@xxxxxxxxxx>
>>
>> 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@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