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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list