Re: [libvirt] [PATCH v9 11/13] backup: qemu: Implement metadata tracking for checkpoint APIs

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(a)redhat.com&gt; > ---

> +/* 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?

> + 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; > + }

> + > + /* 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.

> + /* 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.