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

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



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.

-- 
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]