On Sat, Jul 06, 2019 at 22:56:12 -0500, Eric Blake wrote:
Time to actually issue the QMP transactions that create and
delete persistent checkpoints. For create, we only need one
transaction: inside, we visit all disks affected by the
checkpoint, and create a new enabled bitmap, as well as
disabling the bitmap of the parent checkpoint (if any). For
deletion, we need multiple calls: if the checkpoint to be
deleted is active, we must enable the parent; then we must
merge the existing checkpoint into the parent, and finally
we can delete the checkpoint.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_domain.c | 105 ++++++++++++++++++++++++++++++-----------
src/qemu/qemu_driver.c | 92 ++++++++++++++++++++++++++++++++++--
2 files changed, 165 insertions(+), 32 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 03a143a228..2fca7bd0b8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8960,45 +8960,93 @@ qemuDomainCheckpointDiscard(virQEMUDriverPtr driver,
char *chkFile = NULL;
int ret = -1;
virDomainMomentObjPtr parentchk = NULL;
+ virDomainCheckpointDefPtr parentdef = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ size_t i, j;
+ virJSONValuePtr arr = NULL;
- if (!metadata_only) {
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot remove checkpoint from inactive
domain"));
- goto cleanup;
- } else {
- /* TODO: Implement QMP sequence to merge bitmaps */
- // qemuDomainObjPrivatePtr priv;
- // priv = vm->privateData;
- // qemuDomainObjEnterMonitor(driver, vm);
- // /* we continue on even in the face of error */
- // qemuMonitorDeleteCheckpoint(priv->mon, chk->def->name);
- // ignore_value(qemuDomainObjExitMonitor(driver, vm));
- }
+ if (!metadata_only && !virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot remove checkpoint from inactive domain"));
+ goto cleanup;
}
if (virAsprintf(&chkFile, "%s/%s/%s.xml", cfg->checkpointDir,
vm->def->name, chk->def->name) < 0)
goto cleanup;
+ if (chk->def->parent_name) {
+ parentchk = virDomainCheckpointFindByName(vm->checkpoints,
+ chk->def->parent_name);
+ if (!parentchk) {
+ VIR_WARN("missing parent checkpoint matching name '%s'",
+ chk->def->parent_name);
+ }
+ parentdef = virDomainCheckpointObjGetDef(parentchk);
+ }
+
+ if (!metadata_only) {
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ bool success = true;
+ virDomainCheckpointDefPtr chkdef = virDomainCheckpointObjGetDef(chk);
+
+ if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+ goto cleanup;
As described in previous patch, this is undesired.
+ qemuDomainObjEnterMonitor(driver, vm);
+ for (i = 0; i < chkdef->ndisks; i++) {
+ virDomainCheckpointDiskDef *disk = &chkdef->disks[i];
+ const char *node;
+
+ if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
+ continue;
+
+ node = qemuBlockNodeLookup(vm, disk->name);
+ if (parentchk) {
+ arr = virJSONValueNewArray();
+ if (!arr ||
+ virJSONValueArrayAppendString(arr, disk->bitmap) < 0) {
+ success = false;
+ break;
+ }
+
+ for (j = 0; j < parentdef->ndisks; j++) {
+ virDomainCheckpointDiskDef *disk2;
+
+ disk2 = &parentdef->disks[j];
+ if (STRNEQ(disk->name, disk2->name))
+ continue;
+ if (chk == virDomainCheckpointGetCurrent(vm->checkpoints)
&&
As outlined before in reviews to previous patches and also discussed on
IRC, the "current" checkpoint may not be the only one with active
bitmaps, so this might not work as expected.
+ qemuMonitorEnableBitmap(priv->mon, node,
+ disk2->bitmap) < 0) {
+ success = false;
+ break;
+ }
+ if (qemuMonitorMergeBitmaps(priv->mon, node,
+ disk2->bitmap, &arr) < 0) {
+ success = false;
+ break;
+ }
+ }
+ }
+ if (qemuMonitorDeleteBitmap(priv->mon, node, disk->bitmap) < 0) {
+ success = false;
+ break;
+ }
+ }
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || !success)
+ goto cleanup;
+ }
+
if (chk == virDomainCheckpointGetCurrent(vm->checkpoints)) {
virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
- if (update_parent && chk->def->parent_name) {
- parentchk = virDomainCheckpointFindByName(vm->checkpoints,
- chk->def->parent_name);
- if (!parentchk) {
- VIR_WARN("missing parent checkpoint matching name
'%s'",
+ if (update_parent && parentchk) {
+ virDomainCheckpointSetCurrent(vm->checkpoints, parentchk);
+ if (qemuDomainCheckpointWriteMetadata(vm, parentchk, driver->caps,
+ driver->xmlopt,
+ cfg->checkpointDir) < 0) {
+ VIR_WARN("failed to set parent checkpoint '%s' as
current",
chk->def->parent_name);
- } else {
- virDomainCheckpointSetCurrent(vm->checkpoints, parentchk);
- if (qemuDomainCheckpointWriteMetadata(vm, parentchk, driver->caps,
- driver->xmlopt,
- cfg->checkpointDir) < 0) {
- VIR_WARN("failed to set parent checkpoint '%s' as
current",
- chk->def->parent_name);
- virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
- }
+ virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
}
}
}
@@ -9014,6 +9062,7 @@ qemuDomainCheckpointDiscard(virQEMUDriverPtr driver,
cleanup:
VIR_FREE(chkFile);
virObjectUnref(cfg);
+ virJSONValueFree(arr);
return ret;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 702a715902..b37307abc7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -47,6 +47,7 @@
#include "qemu_hostdev.h"
#include "qemu_hotplug.h"
#include "qemu_monitor.h"
+#include "qemu_monitor_json.h"
I'd prefer if you not include this header directly here but I'm ashamed
of myself for including it in qemu_block.c
#include "qemu_process.h"
#include "qemu_migration.h"
#include "qemu_migration_params.h"
@@ -16994,6 +16995,53 @@ qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr
caps,
return ret;
}
+static int
+qemuDomainCheckpointAddActions(virDomainObjPtr vm,
+ virJSONValuePtr actions,
+ virDomainMomentObjPtr old_current,
+ virDomainCheckpointDefPtr def)
+{
+ size_t i, j;
+ int ret = -1;
+ virDomainCheckpointDefPtr olddef;
+
+ olddef = virDomainCheckpointObjGetDef(old_current);
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainCheckpointDiskDef *disk = &def->disks[i];
+ const char *node;
+
+ if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
+ continue;
+ node = qemuBlockNodeLookup(vm, disk->name);
+ if (qemuMonitorJSONTransactionAdd(actions,
+ "block-dirty-bitmap-add",
+ "s:node", node,
+ "s:name", disk->bitmap,
+ "b:persistent", true,
+ NULL) < 0)
+ goto cleanup;
+ if (old_current) {
+ for (j = 0; j < olddef->ndisks; j++) {
+ virDomainCheckpointDiskDef *disk2;
+
+ disk2 = &olddef->disks[j];
+ if (STRNEQ(disk->name, disk2->name))
+ continue;
+ if (disk2->type == VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP &&
+ qemuMonitorJSONTransactionAdd(actions,
+
"block-dirty-bitmap-disable",
As we've discussed on IRC I think that this is an unnecessary
complication of things while we don't really know what the implications
of handling bitmaps accross multiple backing files are.
As long as you are okay to work around the implications and complex
lookup of the correct bitmap I don't mind adding it this way though.
+
"s:node", node,
+ "s:name", disk2->bitmap,
+ NULL) < 0)
+ goto cleanup;
+ }
+ }
+ }
+ ret = 0;
+
+ cleanup:
+ return ret;
+}
static virDomainCheckpointPtr
qemuDomainCheckpointCreateXML(virDomainPtr domain,
[...]
@@ -17077,10 +17135,10 @@ qemuDomainCheckpointCreateXML(virDomainPtr
domain,
def = NULL;
}
- current = virDomainCheckpointGetCurrent(vm->checkpoints);
- if (current) {
+ other = virDomainCheckpointGetCurrent(vm->checkpoints);
+ if (other) {
if (!redefine &&
- VIR_STRDUP(chk->def->parent_name, current->def->name) < 0)
+ VIR_STRDUP(chk->def->parent_name, other->def->name) < 0)
goto endjob;
if (update_current) {
virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
Does this hunk belong to the previous patch?
[...]
@@ -17328,6 +17396,22 @@
qemuDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
+ priv = vm->privateData;
+ if (!metadata_only) {
+ /* Until qemu-img supports offline bitmap deletion, we are stuck
+ * with requiring a running guest */
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot delete checkpoint for inactive
domain"));
+ goto endjob;
+ }
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("qemu binary lacks persistent bitmaps
support"));
+ goto endjob;
+ }
+ }
+
if (!(chk = qemuCheckObjFromCheckpoint(vm, checkpoint)))
goto endjob;
The rest looks good, but here this does too much with the "current"
snapshot which in my opinion will not work properly, thus I'd like to
see a fixed version of this.