On 7/8/19 11:33 AM, Peter Krempa wrote:
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@redhat.com> ---
+/* 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?
Consider it done.
+ 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; + }
Indeed; fixed for v10.
+ + /* 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.
And thanks to forced RNG validation, this whole function goes away.
+ /* 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!!!
Well, it _was_ necessary with -drive if you had just done 'virsh start $dom', but not necessary for libvirtd restarted with an already running domain. I have a patch for that in v10. I suspect we may also have problems with hot-plug or media changes (floppies and cdroms), which may need to update node names when not CAPS_BLOCKDEV, but I didn't spend time chasing those down so far.
+ 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.
Okay.
+ 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.
v10 should be landing on the list shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org