On 4/26/19 8:42 AM, Peter Krempa wrote: > On Wed, Apr 17, 2019 at 09:09:16 -0500, Eric Blake wrote: >> A lot of this work heavily copies from the existing snapshot >> APIs. The interaction with qemu during create/delete still >> needs to be implemented, but this takes care of all the libvirt >> metadata (saving and restoring XML, and tracking the relations >> between multiple checkpoints). > > I'd prefer at least the 'qemu_conf' related stuff to be separate. > >> @@ -242,6 +244,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) >> goto error; >> if (virAsprintf(&cfg->snapshotDir, "%s/qemu/snapshot", cfg->configBaseDir) < 0) >> goto error; >> + if (virAsprintf(&cfg->checkpointDir, "%s/qemu/checkpoint", cfg->configBaseDir) < 0) >> + goto error; > > This directory will need to be versionable as we need to record > alternate histories when creating snapshots. I'm not sure how to achieve > thant though. We'll probably need to add a directory into 'snapshot' > folder for each snapshot which will copy the whole checkpoint tree. No, we don't need versioned directories. Rather, we simply rely on the fact that checkpoints, like snapshots, are already stored as a tree relationship, where one checkpoint can have more than one child where those multiple children are created across the original snapshot vs. the revert to the snapshot. We may need one extra XML line in snapshots to record which checkpoint to use as parent of any child created during a revert, but that should be sufficient. >> + /* Easiest way to clone inactive portion of vm->def is via >> + * conversion in and back out of xml. */ >> + if (!(xml = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, >> + true, true)) || >> + !(def->common.dom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, >> + VIR_DOMAIN_DEF_PARSE_INACTIVE | >> + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) >> + goto cleanup; >> + >> + if (virDomainCheckpointAlignDisks(def) < 0 || >> + qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) > > This is always done when starting the qemu instance and on every update, > thus should not be necessary. In my initial demos, I was running into instances where it was not happening correctly. But if I can remove it and things work, all the better. > > Also note that if necessary it must be guarded by > if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV) && > > With blockdev we shouldn't ever do that. > >> +static int >> +qemuDomainHasCurrentCheckpoint(virDomainPtr domain, >> + unsigned int flags) > > This API seems a bit pointless if you can call > qemuDomainCheckpointCurrent. It mirrors what snapshots provide. But I can always try to factor it out into a separate patch, and add it back in only if we need it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
