On 10/23/2012 09:41 PM, Osier Yang wrote: > On 2012年10月23日 23:12, Peter Krempa wrote: >> From: Eric Blake<[email protected]> >> >> 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 [email protected] +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
