[libvirt] [PATCH] atomic snapshots, round 3
Depends on round 2 being applied first: https://www.redhat.com/archives/libvir-list/2012-March/msg00761.html This is the next part of my overall RFC: https://www.redhat.com/archives/libvir-list/2012-March/msg00578.html Eric Blake (1): snapshot: improve qemu handling of reused snapshot targets src/libvirt.c | 11 +++++++---- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++----------- src/qemu/qemu_monitor.c | 14 ++++++++------ src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 6 +++++- src/qemu/qemu_monitor_json.h | 4 +++- 6 files changed, 48 insertions(+), 24 deletions(-) -- 1.7.7.6
The oVirt developers have stated that the real reasons they want to have qemu reuse existing volumes when creating a snapshot are: 1. the management framework is set up so that creation has to be done from a central node for proper resource tracking, and having libvirt and/or qemu create things violates the framework, and 2. qemu defaults to creating snapshots with an absolute path to the backing file, but oVirt wants to manage a backing chain that uses just relative names, to allow for easier migration of a chain across storage locations. When 0.9.10 added VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT (commit 4e9953a4), it only addressed point 1, but libvirt was still using O_TRUNC which violates point 2. Meanwhile, the new qemu 'transaction' monitor command includes a new optional mode argument that will force qemu to reuse the metadata of the file it just opened (with the burden on the caller to have valid metadata there in the first place). So, this tweaks the meaning of the flag to cover both points as intended for use by oVirt. It is not strictly backward-compatible to 0.9.10 behavior, but it can be argued that the O_TRUNC of 0.9.10 was a bug. Note that this flag is all-or-nothing, and only selects between 'existing' and the default 'absolute-paths'. A more flexible approach that would allow per-disk selections, as well as adding support for the 'no-backing-file' mode, would be possible by extending the <domainsnapshot> xml to have a per-disk mode, but until we have a management application expressing a need for that additional complexity, it is not worth doing. * src/libvirt.c (virDomainSnapshotCreateXML): Tweak documentation. * src/qemu/qemu_monitor.h (qemuMonitorDiskSnapshot): Add parameters. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskSnapshot): Likewise. * src/qemu/qemu_monitor.c (qemuMonitorDiskSnapshot): Pass them through. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskSnapshot): Use new monitor command arguments. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): Adjust callers. (qemuDomainSnapshotDiskPrepare): Allow qed, modify rules on reuse. --- src/libvirt.c | 11 +++++++---- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++----------- src/qemu/qemu_monitor.c | 14 ++++++++------ src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 6 +++++- src/qemu/qemu_monitor_json.h | 4 +++- 6 files changed, 48 insertions(+), 24 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 534c8ac..7df3667 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17113,10 +17113,13 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well. * * By default, if the snapshot involves external files, and any of the - * destination files already exist as a regular file, the snapshot is - * rejected to avoid losing contents of those files. However, if - * @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, then existing - * destination files are instead truncated and reused. + * destination files already exist as a non-empty regular file, the + * snapshot is rejected to avoid losing contents of those files. + * However, if @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, + * then the destination files must already exist and contain content + * identical to the source files (this allows a management app to + * pre-create files with relative backing file names, rather than the + * default of creating with absolute backing file names). * * Be aware that although libvirt prefers to report errors up front with * no other effect, some hypervisors have certain types of failures where diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2bc8d5d..bc21df0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9725,6 +9725,12 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData; + if (allow_reuse && !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reuse is not supported with this QEMU binary")); + goto cleanup; + } + for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; @@ -9755,8 +9761,8 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, virReportOOMError(); goto cleanup; } - } else if (STRNEQ(disk->driverType, "qcow2")) { - /* XXX We should also support QED */ + } else if (STRNEQ(disk->driverType, "qcow2") && + STRNEQ(disk->driverType, "qed")) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot format for disk %s " "is unsupported: %s"), @@ -9770,7 +9776,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name, disk->file); goto cleanup; } - } else if (!(S_ISBLK(st.st_mode) || allow_reuse)) { + } else if (!(S_ISBLK(st.st_mode) || !st.st_size || allow_reuse)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot file for disk %s already " "exists and is not a block device: %s"), @@ -9823,7 +9829,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk, virDomainDiskDefPtr persistDisk, - virJSONValuePtr actions) + virJSONValuePtr actions, + bool reuse) { qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; @@ -9845,19 +9852,20 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || !(source = strdup(snap->file)) || - (STRNEQ_NULLABLE(disk->driverType, "qcow2") && - !(driverType = strdup("qcow2"))) || + (STRNEQ_NULLABLE(disk->driverType, snap->driverType) && + !(driverType = strdup(snap->driverType))) || (persistDisk && (!(persistSource = strdup(source)) || - (STRNEQ_NULLABLE(persistDisk->driverType, "qcow2") && - !(persistDriverType = strdup("qcow2")))))) { + (STRNEQ_NULLABLE(persistDisk->driverType, snap->driverType) && + !(persistDriverType = strdup(snap->driverType)))))) { virReportOOMError(); goto cleanup; } /* create the stub file and set selinux labels; manipulate disk in * place, in a way that can be reverted on failure. */ - fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT, + fd = qemuOpenFile(driver, source, + O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT), &need_unlink, NULL); if (fd < 0) goto cleanup; @@ -9883,7 +9891,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = NULL; /* create the actual snapshot */ - ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source); + ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source, + driverType, reuse); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); if (ret < 0) goto cleanup; @@ -10004,6 +10013,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, bool persist = false; int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool atomic = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) return -1; @@ -10079,7 +10089,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], vm->def->disks[i], - persistDisk, actions); + persistDisk, actions, + reuse); if (ret < 0) break; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1d54249..bbe0f98 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2624,12 +2624,13 @@ int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name) * at file. */ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, - const char *device, const char *file) + const char *device, const char *file, + const char *format, bool reuse) { int ret; - VIR_DEBUG("mon=%p, actions=%p, device=%s, file=%s", - mon, actions, device, file); + VIR_DEBUG("mon=%p, actions=%p, device=%s, file=%s, format=%s, reuse=%d", + mon, actions, device, file, format, reuse); if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2638,11 +2639,12 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, } if (mon->json) { - ret = qemuMonitorJSONDiskSnapshot(mon, actions, device, file); + ret = qemuMonitorJSONDiskSnapshot(mon, actions, device, file, format, + reuse); } else { - if (actions) { + if (actions || STRNEQ(format, "qcow2") || reuse) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("actions not supported with text monitor")); + _("text monitor lacks several snapshot features")); return -1; } ret = qemuMonitorTextDiskSnapshot(mon, device, file); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7e91951..909e9ac 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -495,7 +495,9 @@ int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, const char *device, - const char *file); + const char *file, + const char *format, + bool reuse); int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ba07e84..0e8bcce 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3060,7 +3060,8 @@ cleanup: int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, - const char *device, const char *file) + const char *device, const char *file, + const char *format, bool reuse) { int ret; virJSONValuePtr cmd; @@ -3070,6 +3071,9 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, "blockdev-snapshot-sync", "s:device", device, "s:snapshot-file", file, + "s:format", format, + reuse ? "s:mode" : NULL, + reuse ? "existing" : NULL, NULL); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index e127870..a0f67aa 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -228,7 +228,9 @@ int qemuMonitorJSONDeleteSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, const char *device, - const char *file); + const char *file, + const char *format, + bool reuse); int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions); int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, -- 1.7.7.6
On 03/20/2012 03:29 PM, Eric Blake wrote:
The oVirt developers have stated that the real reasons they want to have qemu reuse existing volumes when creating a snapshot are: 1. the management framework is set up so that creation has to be done from a central node for proper resource tracking, and having libvirt and/or qemu create things violates the framework, and 2. qemu defaults to creating snapshots with an absolute path to the backing file, but oVirt wants to manage a backing chain that uses just relative names, to allow for easier migration of a chain across storage locations.
/* create the stub file and set selinux labels; manipulate disk in * place, in a way that can be reverted on failure. */ - fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT, + fd = qemuOpenFile(driver, source, + O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT), &need_unlink, NULL); if (fd < 0) goto cleanup;
I just realized this is a pointless open in the reuse case, since the very next statement closes the fd (in other words, we are using open for the side-effect of creation, but reuse says no creation is necessary). I plan on squashing this: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index ca81e75..4286b2a 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -9884,12 +9884,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, /* create the stub file and set selinux labels; manipulate disk in * place, in a way that can be reverted on failure. */ - fd = qemuOpenFile(driver, source, - O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT), - &need_unlink, NULL); - if (fd < 0) - goto cleanup; - VIR_FORCE_CLOSE(fd); + if (!reuse) { + fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT, + &need_unlink, NULL); + if (fd < 0) + goto cleanup; + VIR_FORCE_CLOSE(fd); + } origsrc = disk->src; disk->src = source; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Mar 20, 2012 at 15:29:37 -0600, Eric Blake wrote:
The oVirt developers have stated that the real reasons they want to have qemu reuse existing volumes when creating a snapshot are: 1. the management framework is set up so that creation has to be done from a central node for proper resource tracking, and having libvirt and/or qemu create things violates the framework, and 2. qemu defaults to creating snapshots with an absolute path to the backing file, but oVirt wants to manage a backing chain that uses just relative names, to allow for easier migration of a chain across storage locations.
When 0.9.10 added VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT (commit 4e9953a4), it only addressed point 1, but libvirt was still using O_TRUNC which violates point 2. Meanwhile, the new qemu 'transaction' monitor command includes a new optional mode argument that will force qemu to reuse the metadata of the file it just opened (with the burden on the caller to have valid metadata there in the first place). So, this tweaks the meaning of the flag to cover both points as intended for use by oVirt. It is not strictly backward-compatible to 0.9.10 behavior, but it can be argued that the O_TRUNC of 0.9.10 was a bug.
I would agree. It makes more sense if REUSE reuses the whole file including its content instead of reusing just the file name and some metadata.
Note that this flag is all-or-nothing, and only selects between 'existing' and the default 'absolute-paths'. A more flexible approach that would allow per-disk selections, as well as adding support for the 'no-backing-file' mode, would be possible by extending the <domainsnapshot> xml to have a per-disk mode, but until we have a management application expressing a need for that additional complexity, it is not worth doing.
Every opportunity to not increase the complexity of snapshot API even moreis always welcome :-) The patch looks like it's doing what you say it's doing, so ACK (with the additional patch you plan to squash in). Jirka
On 03/23/2012 11:06 AM, Jiri Denemark wrote:
I would agree. It makes more sense if REUSE reuses the whole file including its content instead of reusing just the file name and some metadata.
Note that this flag is all-or-nothing, and only selects between 'existing' and the default 'absolute-paths'. A more flexible approach that would allow per-disk selections, as well as adding support for the 'no-backing-file' mode, would be possible by extending the <domainsnapshot> xml to have a per-disk mode, but until we have a management application expressing a need for that additional complexity, it is not worth doing.
Every opportunity to not increase the complexity of snapshot API even moreis always welcome :-)
The patch looks like it's doing what you say it's doing, so ACK (with the additional patch you plan to squash in).
Thanks; I've pushed round 3 now. Round 4 (XML <mirror>) and 5 (snapshot delete bits) are tabled, perhaps permanently, while I instead work on virDomainBlockRebase for live block migration with pull. I still plan on hammering these more over the next week, but any future fixes will be followup patches. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Dňa 20.3.2012 22:29, Eric Blake wrote / napísal(a):
The oVirt developers have stated that the real reasons they want to have qemu reuse existing volumes when creating a snapshot are: 1. the management framework is set up so that creation has to be done from a central node for proper resource tracking, and having libvirt and/or qemu create things violates the framework, and 2. qemu defaults to creating snapshots with an absolute path to the backing file, but oVirt wants to manage a backing chain that uses just relative names, to allow for easier migration of a chain across storage locations.
When 0.9.10 added VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT (commit 4e9953a4), it only addressed point 1, but libvirt was still using O_TRUNC which violates point 2. Meanwhile, the new qemu 'transaction' monitor command includes a new optional mode argument that will force qemu to reuse the metadata of the file it just opened (with the burden on the caller to have valid metadata there in the first place). So, this tweaks the meaning of the flag to cover both points as intended for use by oVirt. It is not strictly backward-compatible to 0.9.10 behavior, but it can be argued that the O_TRUNC of 0.9.10 was a bug.
Note that this flag is all-or-nothing, and only selects between 'existing' and the default 'absolute-paths'. A more flexible approach that would allow per-disk selections, as well as adding support for the 'no-backing-file' mode, would be possible by extending the<domainsnapshot> xml to have a per-disk mode, but until we have a management application expressing a need for that additional complexity, it is not worth doing.
* src/libvirt.c (virDomainSnapshotCreateXML): Tweak documentation. * src/qemu/qemu_monitor.h (qemuMonitorDiskSnapshot): Add parameters. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskSnapshot): Likewise. * src/qemu/qemu_monitor.c (qemuMonitorDiskSnapshot): Pass them through. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskSnapshot): Use new monitor command arguments. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): Adjust callers. (qemuDomainSnapshotDiskPrepare): Allow qed, modify rules on reuse. --- src/libvirt.c | 11 +++++++---- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++----------- src/qemu/qemu_monitor.c | 14 ++++++++------ src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 6 +++++- src/qemu/qemu_monitor_json.h | 4 +++- 6 files changed, 48 insertions(+), 24 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 534c8ac..7df3667 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17113,10 +17113,13 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well. * * By default, if the snapshot involves external files, and any of the - * destination files already exist as a regular file, the snapshot is - * rejected to avoid losing contents of those files. However, if - * @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, then existing - * destination files are instead truncated and reused. + * destination files already exist as a non-empty regular file, the + * snapshot is rejected to avoid losing contents of those files. + * However, if @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, + * then the destination files must already exist and contain content + * identical to the source files (this allows a management app to + * pre-create files with relative backing file names, rather than the + * default of creating with absolute backing file names). * * Be aware that although libvirt prefers to report errors up front with * no other effect, some hypervisors have certain types of failures where diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2bc8d5d..bc21df0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9725,6 +9725,12 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData;
+ if (allow_reuse&& !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reuse is not supported with this QEMU binary")); + goto cleanup; + } + for (i = 0; i< def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk =&def->disks[i];
@@ -9755,8 +9761,8 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, virReportOOMError(); goto cleanup; } - } else if (STRNEQ(disk->driverType, "qcow2")) { - /* XXX We should also support QED */ + } else if (STRNEQ(disk->driverType, "qcow2")&& + STRNEQ(disk->driverType, "qed")) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot format for disk %s " "is unsupported: %s"), @@ -9770,7 +9776,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name, disk->file); goto cleanup; } - } else if (!(S_ISBLK(st.st_mode) || allow_reuse)) { + } else if (!(S_ISBLK(st.st_mode) || !st.st_size || allow_reuse)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot file for disk %s already " "exists and is not a block device: %s"), @@ -9823,7 +9829,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk, virDomainDiskDefPtr persistDisk, - virJSONValuePtr actions) + virJSONValuePtr actions, + bool reuse) { qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; @@ -9845,19 +9852,20 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
if (virAsprintf(&device, "drive-%s", disk->info.alias)< 0 || !(source = strdup(snap->file)) || - (STRNEQ_NULLABLE(disk->driverType, "qcow2")&& - !(driverType = strdup("qcow2"))) || + (STRNEQ_NULLABLE(disk->driverType, snap->driverType)&& + !(driverType = strdup(snap->driverType))) ||
If the source and snapshod drivers are equal, nothing gets assigned to driverType.
(persistDisk&& (!(persistSource = strdup(source)) || - (STRNEQ_NULLABLE(persistDisk->driverType, "qcow2")&& - !(persistDriverType = strdup("qcow2")))))) { + (STRNEQ_NULLABLE(persistDisk->driverType, snap->driverType)&& + !(persistDriverType = strdup(snap->driverType)))))) { virReportOOMError(); goto cleanup; }
/* create the stub file and set selinux labels; manipulate disk in * place, in a way that can be reverted on failure. */ - fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT, + fd = qemuOpenFile(driver, source, + O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT), &need_unlink, NULL); if (fd< 0) goto cleanup; @@ -9883,7 +9891,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = NULL;
/* create the actual snapshot */ - ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source); + ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source, + driverType, reuse); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret>= 0); if (ret< 0) goto cleanup; @@ -10004,6 +10013,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, bool persist = false; int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool atomic = (flags& VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + bool reuse = (flags& VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY)< 0) return -1; @@ -10079,7 +10089,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], vm->def->disks[i], - persistDisk, actions); + persistDisk, actions, + reuse); if (ret< 0) break; }
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ba07e84..0e8bcce 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3060,7 +3060,8 @@ cleanup:
int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, - const char *device, const char *file) + const char *device, const char *file, + const char *format, bool reuse) { int ret; virJSONValuePtr cmd; @@ -3070,6 +3071,9 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, "blockdev-snapshot-sync", "s:device", device, "s:snapshot-file", file, + "s:format", format,
and you pass NULL as format the json framework creates an invalid json string like: {"execute":"transaction","arguments": {"actions": [ {"type":"blockdev-snapshot-sync","data": {"device":"drive-ide0-0-0","snapshot-file":"/var/lib/libvirt/images/test.1332842808","format":null} << - here the "format":null } ]} ,"id":"libvirt-6"} which makes qemu sad :( I'll post a patch soon. Peter
participants (3)
-
Eric Blake -
Jiri Denemark -
Peter Krempa