On Sat, Jul 06, 2019 at 22:56:11 -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).
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_block.h | 3 +
> src/qemu/qemu_conf.h | 2 +
> src/qemu/qemu_domain.h | 15 +
> src/qemu/qemu_block.c | 12 +
> src/qemu/qemu_conf.c | 5 +
> src/qemu/qemu_domain.c | 133 ++++++++
> src/qemu/qemu_driver.c | 675 +++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 845 insertions(+)
[...]
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 0a6522577d..5f5e330479 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -1821,3 +1821,15 @@ qemuBlockStorageGetCopyOnReadProps(virDomainDiskDefPtr disk)
>
> return ret;
> }
> +
> +const char *
> +qemuBlockNodeLookup(virDomainObjPtr vm, const char *disk)
This looks like it belongs more into qemu_domain.c since it primarily
looks up the disk and not the node name.
> +{
> + size_t i;
> +
> + for (i = 0; i < vm->def->ndisks; i++) {
> + if (STREQ(vm->def->disks[i]->dst, disk))
> + return vm->def->disks[i]->src->nodeformat;
> + }
> + return NULL;
> +}
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 97f3d7f786..702a715902 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
[...]
> @@ -221,6 +222,40 @@ qemuSnapObjFromSnapshot(virDomainObjPtr vm,
> return qemuSnapObjFromName(vm, snapshot->name);
> }
>
> +/* Looks up the domain object from checkpoint and unlocks the
> + * driver. The returned domain object is locked and ref'd and the
> + * caller must call virDomainObjEndAPI() on it. */
> +static virDomainObjPtr
> +qemuDomObjFromCheckpoint(virDomainCheckpointPtr checkpoint)
> +{
> + return qemuDomObjFromDomain(checkpoint->domain);
> +}
> +
> +
> +/* Looks up checkpoint object from VM and name */
> +static virDomainMomentObjPtr
> +qemuCheckObjFromName(virDomainObjPtr vm,
> + const char *name)
> +{
> + virDomainMomentObjPtr chk = NULL;
> + chk = virDomainCheckpointFindByName(vm->checkpoints, name);
> + if (!chk)
> + virReportError(VIR_ERR_NO_DOMAIN_CHECKPOINT,
> + _("no domain checkpoint with matching name '%s'"),
> + name);
> +
> + return chk;
> +}
> +
> +
> +/* Looks up checkpoint object from VM and checkpointPtr */
> +static virDomainMomentObjPtr
> +qemuCheckObjFromCheckpoint(virDomainObjPtr vm,
> + virDomainCheckpointPtr checkpoint)
Both of the function above contain 'Check' which looks more like a verb
than a noun. Could you please expand it?
> +{
> + return qemuCheckObjFromName(vm, checkpoint->name);
> +}
> +
> static int
> qemuAutostartDomain(virDomainObjPtr vm,
> void *opaque)
> @@ -524,6 +559,123 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
> }
>
>
> +static int
> +qemuDomainCheckpointLoad(virDomainObjPtr vm,
> + void *data)
> +{
> + char *baseDir = (char *)data;
> + char *chkDir = NULL;
> + DIR *dir = NULL;
> + struct dirent *entry;
> + char *xmlStr;
> + char *fullpath;
> + virDomainCheckpointDefPtr def = NULL;
> + virDomainMomentObjPtr chk = NULL;
> + virDomainMomentObjPtr current = NULL;
> + bool cur;
> + unsigned int flags = VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE;
> + int ret = -1;
> + virCapsPtr caps = NULL;
> + int direrr;
> +
> + virObjectLock(vm);
> + if (virAsprintf(&chkDir, "%s/%s", baseDir, vm->def->name) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to allocate memory for "
> + "checkpoint directory for domain %s"),
> + vm->def->name);
> + goto cleanup;
> + }
> +
> + if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
> + goto cleanup;
> +
> + VIR_INFO("Scanning for checkpoints for domain %s in %s", vm->def->name,
> + chkDir);
> +
> + if (virDirOpenIfExists(&dir, chkDir) <= 0)
> + goto cleanup;
> +
> + while ((direrr = virDirRead(dir, &entry, NULL)) > 0) {
> + /* NB: ignoring errors, so one malformed config doesn't
> + kill the whole process */
> + VIR_INFO("Loading checkpoint file '%s'", entry->d_name);
> +
> + if (virAsprintf(&fullpath, "%s/%s", chkDir, entry->d_name) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to allocate memory for path"));
> + continue;
> + }
> +
> + if (virFileReadAll(fullpath, 1024*1024*1, &xmlStr) < 0) {
> + /* Nothing we can do here, skip this one */
> + virReportSystemError(errno,
> + _("Failed to read checkpoint file %s"),
> + fullpath);
> + VIR_FREE(fullpath);
> + continue;
> + }
> +
> + def = virDomainCheckpointDefParseString(xmlStr, caps,
> + qemu_driver->xmlopt, &cur,
> + flags);
> + if (!def || virDomainCheckpointAlignDisks(def) < 0) {
> + /* Nothing we can do here, skip this one */
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to parse checkpoint XML from file '%s'"),
> + fullpath);
> + VIR_FREE(fullpath);
> + VIR_FREE(xmlStr);
leaks 'def'
> + continue;
> + }
> +
> + chk = virDomainCheckpointAssignDef(vm->checkpoints, def);
> + if (chk == NULL) {
> + virObjectUnref(def);
> + } else if (cur) {
> + if (current)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Too many checkpoints claiming to be current for domain %s"),
> + vm->def->name);
> + current = chk;
> + }
> +
> + VIR_FREE(fullpath);
> + VIR_FREE(xmlStr);
> + }
> + if (direrr < 0)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to fully read directory %s"),
> + chkDir);
> +
> + virDomainCheckpointSetCurrent(vm->checkpoints, current);
> +
> + if (virDomainCheckpointUpdateRelations(vm->checkpoints) < 0)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Checkpoints have inconsistent relations for domain %s"),
> + vm->def->name);
> +
> + /* FIXME: qemu keeps internal track of bitmaps, which form the
> + * basis for checkpoints; it would be nice if we could update our
> + * internal state to reflect that information automatically. But
> + * qemu 3.0 did not have access to this via qemu-img for offline
> + * images (you have to use QMP commands on a running guest), and
> + * it also does not track <parent> relations which we find
> + * important in our metadata.
> + */
That would be pointless to do if qemu is not running because users can
modify the files behind our back anyways and since we don't have inotify
on all the possible files (which would be impossible) we'd have to redo
all of this anyways when starting the vm. Thus probably drop this
comment.
> +
> + virResetLastError();
> +
> + ret = 0;
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + VIR_FREE(chkDir);
> + virObjectUnref(caps);
> + virObjectUnlock(vm);
> + return ret;
> +}
> +
> +
> static int
> qemuDomainNetsRestart(virDomainObjPtr vm,
> void *data ATTRIBUTE_UNUSED)
[...]
> @@ -7710,6 +7878,7 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0)
> goto endjob;
> }
> + /* TODO: Restrict deletion if checkpoints exist? */
This should be fairly easy since you've done a similar thing in
qemuDomainRename.
> name = qemuDomainManagedSavePath(driver, vm);
> if (name == NULL)
[...]
> @@ -16728,6 +16903,492 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> return ret;
> }
>
> +
> +/* Called prior to job lock */
> +static virDomainCheckpointDefPtr
> +qemuDomainCheckpointDefParseString(virQEMUDriverPtr driver, virCapsPtr caps,
> + const char *xmlDesc, bool *current,
> + unsigned int flags)
> +{
> + virDomainCheckpointDefPtr ret = NULL;
> + unsigned int parse_flags = 0;
> + VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL;
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE)
> + parse_flags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE;
> + if (!(def = virDomainCheckpointDefParseString(xmlDesc, caps, driver->xmlopt,
> + current, parse_flags)))
> + return NULL;
> +
> + /* reject checkpoint names containing slashes or starting with dot as
> + * checkpoint definitions are saved in files named by the checkpoint name */
> + if (!(flags & VIR_DOMAIN_CHECKPOINT_CREATE_NO_METADATA)) {
> + if (strchr(def->parent.name, '/')) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("invalid checkpoint name '%s': "
> + "name can't contain '/'"),
> + def->parent.name);
That's the job for the validator.
> + return NULL;
> + }
> +
> + if (def->parent.name[0] == '.') {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("invalid checkpoint name '%s': "
> + "name can't start with '.'"),
> + def->parent.name);
> + return NULL;
> + }
> + }
> +
> + VIR_STEAL_PTR(ret, def);
> + return ret;
> +}
[...]
> +static int
> +qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr caps,
> + virDomainObjPtr vm,
> + virDomainCheckpointDefPtr def)
> +{
> + int ret = -1;
> + size_t i;
> + char *xml = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + /* 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->parent.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)
As I've pointed out multiple times already, calling qemuBlockNodeNamesDetect
should not be necessary with -drive and MUST not be done with
QEMU_CAPS_BLOCKDEV. This will mess up the libvirt metadata about the
backing chain!!!
> + goto cleanup;
> +
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk = &def->disks[i];
> +
> + if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
> + continue;
> +
> + if (vm->def->disks[i]->src->format > 0 &&
> + vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
I'd ignore the possibility of format AUTO in this case. It should not
even be possible.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("checkpoint for disk %s unsupported "
> + "for storage type %s"),
> + disk->name,
> + virStorageFileFormatTypeToString(
> + vm->def->disks[i]->src->format));
The rest looks good to me, but I want to see a fixed version of this
which does not call qemuBlockNodeNamesDetect needlessly before giving my
final ACK.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
