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.
> ---
> Diff to v1:
> -added probing of backing file type
> -switched to virCommand
> +qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver,
> + virDomainObjPtr vm,
> + virDomainSnapshotObjPtr snap,
> + bool reuse)
> +{
> + int i = 0;
Unused assignment, given...
> + char *backingFileArg = NULL;
> + virDomainSnapshotDiskDefPtr snapdisk;
> + virDomainDiskDefPtr defdisk;
> + virCommandPtr cmd = NULL;
> + const char *qemuImgPath;
> +
> + int ret = -1;
> +
> + if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
> + return -1;
> +
> + for (i = 0; i < snap->def->ndisks; i++) {
...this initialization.
> + snapdisk = &(snap->def->disks[i]);
> + defdisk = snap->def->dom->disks[snapdisk->index];
> +
> + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
> + /* check if destination file exists */
> + if (reuse && virFileExists(snapdisk->file)) {
> + VIR_DEBUG("Skipping snapshot creation: File exists: %s",
> + snapdisk->file);
> + continue;
> + }
Missing the converse check: if the file exists but reuse is not okay,
then it had better be a block device or an empty file; similar to online
disk snapshots, we don't want to allow the user to accidentally
overwrite files that contain data.
> +
> + if (!snapdisk->format)
> + snapdisk->format = VIR_STORAGE_FILE_QCOW2;
> +
> + /* probe the disk format */
> + if (defdisk->format <= 0) {
> + defdisk->format = virStorageFileProbeFormat(defdisk->src,
> + driver->user,
> + driver->group);
I don't think we need to probe here. If we don't know the current disk
format, then if driver->allowDiskFormatProbing, just omit the
backing_fmt listing; if not, error out and prevent the snapshot creation.
> + if (defdisk->format < 0)
> + goto cleanup;
> + }
> +
> + if (virAsprintf(&backingFileArg, "backing_file=%s,backing_fmt=%s",
> + defdisk->src,
> + virStorageFileFormatTypeToString(defdisk->format)) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
Rather than allocating into a temporary, it may be easier to rearrange
things to use virCommandAddArgFormat.
> +
> + if (!(cmd = virCommandNewArgList(qemuImgPath,
> + "create",
> + "-f",
> + virStorageFileFormatTypeToString(snapdisk->format),
> + "-o",
> + backingFileArg,
> + snapdisk->file,
> + NULL)))
> + goto cleanup;
> +
> + if (virCommandRun(cmd, NULL) < 0)
> + goto cleanup;
> +
> + virCommandFree(cmd);
> + VIR_FREE(backingFileArg);
> + cmd = NULL;
> + }
> + }
> +
> + /* update disk definitions */
This covers rollbacks if qemu-img fails, but leaves things a bit awkward
if we hit OOM. In particular...
> + for (i = 0; i < snap->def->ndisks; i++) {
> + snapdisk = &(snap->def->disks[i]);
> + defdisk = vm->def->disks[snapdisk->index];
> +
> + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
> + VIR_FREE(defdisk->src);
> + if (!(defdisk->src = strdup(snapdisk->file))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + defdisk->format = snapdisk->format;
> + }
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + /* unlink images if creation has failed */
> + if (ret < 0 && i > 0) {
> + for (; i > 0; i--) {
> + snapdisk = &(snap->def->disks[i]);
> + if (unlink(snapdisk->file) < 0)
> + VIR_WARN("Failed to remove snapshot image '%s'",
> + snapdisk->file);
...you end up corrupting the diskdef to point to an unlinked file on OOM.
> + }
> + }
> +
> + VIR_FREE(backingFileArg);
> + virCommandFree(cmd);
> +
> + return ret;
> +}
> +
>
> /* The domain is expected to be locked and active. */
> static int
> @@ -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.
> @@ -11540,8 +11637,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> }
> } else {
> /* inactive */
> - if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0)
> - goto cleanup;
> + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) {
> + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, false) < 0)
> + goto cleanup;
> + } else {
> + if (qemuDomainSnapshotCreateInactiveInternal(driver, vm, snap) < 0)
> + goto cleanup;
> + }
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. 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.
--
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