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@xxxxxxxxxx>
>> ---
>> +/* 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
