On Wed, Jul 24, 2019 at 12:56:08AM -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 first ancestor checkpoint (if any) that also had a bitmap. For deletion, we need multiple calls: for each disk, if there is an ancestor checkpoint with a bitmap, then the bitmap must be merged (including activating the ancestor bitmap), all before deleting the bitmap from the checkpoint being removed.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_domain.c | 122 ++++++++++++++++++++++++++++------------- src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++++- 2 files changed, 180 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d71b7d1984..062a08ed97 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9255,48 +9255,97 @@ qemuDomainCheckpointDiscard(virQEMUDriverPtr driver, bool update_parent, bool metadata_only) { - char *chkFile = NULL; - int ret = -1; - virDomainMomentObjPtr parentchk = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainMomentObjPtr parent = NULL; + virDomainMomentObjPtr moment; + virDomainCheckpointDefPtr parentdef = NULL; + size_t i, j; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOFREE(char *) chkFile = 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")); + return -1; }
This semi-answerss my previous question on earlier patch. I guess we just shouldn't add the commented out lines at all in teh first place, given you are deleting them entirely here.
@@ -17096,7 +17166,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) + goto endjob; + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorTransaction(priv->mon, &actions); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) + goto endjob; }
Ah, and this now answers my other question. Makes me wonder if there's really a benefit to splitting the code across two patches, as opposed to just one. As long as it git bisect's with a clean built though its not a show stopper. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|