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.