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