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