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