[libvirt] [PATCHv2 15/20] snapshot: qemu: Add support for external inactive snapshots

[Peter Krempa]

On 11/06/12 01:06, Eric Blake wrote:
> On 11/02/2012 10:49 PM, Eric Blake wrote:
>> On 11/01/2012 10:22 AM, Peter Krempa wrote:
>>> This patch adds support for external disk snapshots of inactive domains.
>>> The snapshot is created by calling
>>>   qemu-img create -o backing_file=/path/to/disk /path/snapshot_file -f
>>>   backing_file=/path/to/backing/file,backing_fmt=format_of_backing_file
>>
>> Still didn't match the code:
>> qemu-img create -f format_of_snapshot -o
>> backing_file=/path/to/backing,backing_fmt=format_of_backing
>> /path/to/snapshot
>>
>> If disk->format is unspecified, then what we do should depend on
>> driver->allowDiskFormatProbing; if true, omit the argument (it's okay
>> for qemu to do the probing); if false, error out.
>>
>>> on each of the disks selected for snapshotting.
>>> ---
>
>>> @@ -11462,12 +11565,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>>>               goto cleanup;
>>>
>>>           if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
>>> -            if (!virDomainObjIsActive(vm)) {
>>> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> -                               _("disk snapshots of inactive domains not "
>>> -                                 "implemented yet"));
>>> -                goto cleanup;
>>> -            }
>>>               align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>>>               align_match = false;
>>>               def->state = VIR_DOMAIN_DISK_SNAPSHOT;
>>
>> If the domain is offline, I'd treat def->state as VIR_DOMAIN_SHUTOFF,
>> saving VIR_DOMAIN_DISK_SNAPSHOT for the case where we know the domain
>> was running at the time but no memory was saved.
>
>
>> This isn't quite right.  For offline snapshots, we can mix and match
>> internal and external snapshots at will, which means we should call both
>> functions, and ensure that the internal version does nothing unless an
>> internal snapshot is requested.
>
> Or, we can make life simpler by refusing to mix things (and maybe
> revisit that restriction in a later patch, if people want to do it after
> all).
>
>>   Also, shouldn't you be passing (flags &
>> VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXTERNAL) != 0 rather than false?
>>
>> I'll work up some proposed fixes; some of them can be deferred to
>> followup patches, so I'll try to come up with the minimum squash for
>> what I would ack.
>
> Here's the first round of things to squash - I noticed that my earlier
> suggestion of checking for _LIVE and _REDEFINE being mutually exclusive
> is done too late - that needs to be done _before_ the point where we
> alter the current snapshot when update_current is true.  Also, this
> tries to keep things so that def->state == VIR_DOMAIN_DISK_SNAPSHOT is
> only possible for a running domain; an offline domain is always recorded
> as VIR_DOMAIN_SHUTOFF, whether the snapshots are internal or external.
>
> I'm not sure how much of this should be done as an independent patch; in
> fact, it may make sense to split this into multiple patches since I'm
> attempting to address multiple issues.
>

I'll be posting a v3 of the rest of the series soon. I made heavy 
changes to this patch so you should probably wait until then so we don't 
do any duplicate work.

Peter