On 11/07/2012 04:06 PM, Peter Krempa wrote:
> This patch adds support for external disk snapshots of inactive domains.
> The snapshot is created by calling using qemu-img by calling:
>
> qemu-img create -f format_of_snapshot -o
> backing_file=/path/to/src,backing_fmt=format_of_backing_image
> /path/to/snapshot
>
> +qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver,
> + virDomainObjPtr vm,
> + virDomainSnapshotObjPtr snap,
> + bool reuse)
> + /* no-op if reuse is true and file exists and is valid */
> + if (reuse) {
> + if (stat(snapdisk->file, &st) < 0) {
> + if (errno != ENOENT) {
> + virReportSystemError(errno,
> + _("unable to stat snapshot image %s"),
> + snapdisk->file);
> + goto cleanup;
> + }
> + } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) {
> + /* the existing image is reused */
> + continue;
Hey, I just realized that the logic I want here (if reuse, then check
that it exists; if !reuse, then check that we aren't nuking unrelated
data) was already run during qemuDomainSnapshotPrepare. So we can
simplify this - if we got here, then we already know that the file
exists and is ready to reuse, so we can simplify this code.
> +
> + /* update disk definitions */
> + 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))) {
> + /* we cannot rollback here in a sane way */
> + virReportOOMError();
True, but OOM is enough of a corner case that I'm not too bothered by this.
> + return -1;
> + }
> + defdisk->format = snapdisk->format;
> + }
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + virCommandFree(cmd);
> +
> + /* 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)
Possible NULL deref, if you have a disk with no snapshot earlier in the
array than a disk with an external snapshot. Also, we don't want to
unlink() pre-existing block devices, so it may make more sense to pay
attention to whether we actually created a file.
ACK with this squashed in:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 17de98e..cea1c2f 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -10677,31 +10677,26 @@
qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver,
virDomainDiskDefPtr defdisk;
virCommandPtr cmd = NULL;
const char *qemuImgPath;
- struct stat st;
+ virBitmapPtr created;
int ret = -1;
if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
return -1;
- for (i = 0; i < snap->def->ndisks; i++) {
+ if (!(created = virBitmapNew(snap->def->ndisks))) {
+ virReportOOMError();
+ return -1;
+ }
+
+ /* If reuse is true, then qemuDomainSnapshotPrepare already
+ * ensured that the new files exist, and it was up to the user to
+ * create them correctly. */
+ for (i = 0; i < snap->def->ndisks && !reuse; i++) {
snapdisk = &(snap->def->disks[i]);
defdisk = snap->def->dom->disks[snapdisk->index];
-
- /* no-op if reuse is true and file exists and is valid */
- if (reuse) {
- if (stat(snapdisk->file, &st) < 0) {
- if (errno != ENOENT) {
- virReportSystemError(errno,
- _("unable to stat snapshot
image %s"),
- snapdisk->file);
- goto cleanup;
- }
- } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) {
- /* the existing image is reused */
- continue;
- }
- }
+ if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ continue;
if (!snapdisk->format)
snapdisk->format = VIR_STORAGE_FILE_QCOW2;
@@ -10736,6 +10731,9 @@ qemuDomainSnapshotCreateInactiveExternal(struct
qemud_driver *driver,
/* adds cmd line args: /path/to/target/file */
virCommandAddArg(cmd, snapdisk->file);
+ if (!virFileExists(snapdisk->file))
+ ignore_value(virBitmapSetBit(created, i));
+
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
@@ -10753,7 +10751,8 @@ qemuDomainSnapshotCreateInactiveExternal(struct
qemud_driver *driver,
if (!(defdisk->src = strdup(snapdisk->file))) {
/* we cannot rollback here in a sane way */
virReportOOMError();
- return -1;
+ i = snap->def->ndisks;
+ goto cleanup;
}
defdisk->format = snapdisk->format;
}
@@ -10765,14 +10764,16 @@ cleanup:
virCommandFree(cmd);
/* unlink images if creation has failed */
- if (ret < 0 && i > 0) {
- for (; i > 0; i--) {
- snapdisk = &(snap->def->disks[i]);
+ if (ret < 0) {
+ ssize_t bit = -1;
+ while ((bit = virBitmapNextSetBit(created, bit)) >= 0) {
+ snapdisk = &(snap->def->disks[bit]);
if (unlink(snapdisk->file) < 0)
VIR_WARN("Failed to remove snapshot image '%s'",
snapdisk->file);
}
}
+ virBitmapFree(created);
return ret;
}
struct stat st;
--
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