[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 10/23/2012 09:41 PM, Osier Yang wrote:
> On 2012年10月23日 23:12, Peter Krempa wrote:
>> From: Eric Blake<eblake 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   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]