On Wed, Apr 17, 2019 at 09:09:17 -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@xxxxxxxxxx>
> ---
> 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 739074e17d..16dec68302 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8720,45 +8720,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) {
> + parentchk = virDomainCheckpointFindByName(vm->checkpoints,
> + chk->def->parent);
> + if (!parentchk) {
> + VIR_WARN("missing parent checkpoint matching name '%s'",
> + chk->def->parent);
> + }
> + 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 in the previous patches this should not be necessary. If it is, other
places need to be fixed. Or ultimately it needs to be disabled with
BLOCKDEV enabled.
> + 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) &&
> + 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) {
Off topic:
Do we need to delete bitmaps manually prior to doing 'blockdev-del' when
detaching disks? Or said differently, does the bitmap hold a ref to the
underlying node? If yes, the blockdev code will need to be adapted.
> + 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) {
> - parentchk = virDomainCheckpointFindByName(vm->checkpoints,
> - chk->def->parent);
> - 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);
> - } 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);
> - virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> - }
All the deleted code was added in previous commit.
> + virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> }
> }
> }
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 144cb51d89..86310b6e92 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
[...]
> @@ -17056,6 +17057,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",
> + "s:node", node,
> + "s:name", disk2->bitmap,
> + NULL) < 0)
> + goto cleanup;
> + }
> + }
> + }
> + ret = 0;
> +
> + cleanup:
Nothing to clean up.
> + return ret;
> +}
[...]
> @@ -17134,10 +17192,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, current->def->name) < 0)
> + VIR_STRDUP(chk->def->parent, other->def->name) < 0)
> goto endjob;
> if (update_current) {
> virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
This should be squashed into previous patch.
> @@ -17154,7 +17212,15 @@ qemuDomainCheckpointCreateXML(virDomainPtr domain,
> * makes sense, such as checking that qemu-img recognizes the
> * checkpoint bitmap name in at least one of the domain's disks? */
> } else {
> - /* TODO: issue QMP transaction command */
> + if (!(actions = virJSONValueNewArray()))
> + goto endjob;
> + if (qemuDomainCheckpointAddActions(vm, actions, other,
> + virDomainCheckpointObjGetDef(chk)) < 0)
This function can also allocate the array and return it directly.
> + goto endjob;
> + qemuDomainObjEnterMonitor(driver, vm);
> + ret = qemuMonitorTransaction(priv->mon, &actions);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)
> + goto endjob;
> }
>
> /* If we fail after this point, there's not a whole lot we can do;
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
