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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)