Re: [libvirt] [PATCH 01/20] snapshot: new XML for external system checkpoint

On 2012年10月23日 23:12, Peter Krempa wrote: > From: Eric Blake&lt;eblake(a)redhat.com&gt; > > 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.

> + } > + if (offline&& def->memory&& I think def->memory checking is redundant here. Not big deal though. > + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {

> + 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.

Others looks just very sanity, ACK.