On 2/23/19 4:24 PM, Eric Blake wrote:
> Implement the new flags for bulk snapshot dump and redefine. This
> borrows from ideas in the test driver, but is further complicated
> by the fact that qemu must write snapshot XML to disk, and thus must
> do additional validation after the initial parse to ensure the user
> didn't attempt to rename a snapshot with "../" or similar.
>
> Of note: all prior callers of qemuDomainSnapshotDiscardAllMetadata()
> were at points where it did not matter if vm->current_snapshot and
> the metaroot in vm->snapshots were left pointing to stale memory,
> because we were about to delete the entire vm object; but now it is
> important to reset things properly so that the domain still shows
> as having no snapshots on failure.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.h | 2 +-
> src/qemu/qemu_domain.c | 35 +++++++++++++++++------
> src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 89 insertions(+), 12 deletions(-)
>
NB: I couldn't get this one to git am -3 apply - I didn't chase the
conflict though.
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 7c6b50184c..37c9813ec5 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -683,7 +683,7 @@ int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
> virDomainSnapshotObjPtr snapshot,
> virCapsPtr caps,
> virDomainXMLOptionPtr xmlopt,
> - char *snapshotDir);
> + const char *snapshotDir);
Theoretically separable.
>
> int qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cb1665c8f9..cf8b6e8eaf 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7704,6 +7704,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver,
>
> static int
> qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> virDomainDefPtr def,
> virCPUDefPtr origCPU,
> unsigned int flags,
> @@ -7713,8 +7714,10 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
> virDomainDefPtr copy = NULL;
> virCapsPtr caps = NULL;
> virQEMUCapsPtr qemuCaps = NULL;
> + bool snapshots = flags & VIR_DOMAIN_XML_SNAPSHOTS;
>
> - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1);
> + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU |
> + VIR_DOMAIN_XML_SNAPSHOTS, -1);
>
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
> @@ -7881,7 +7884,14 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
> }
>
> format:
> - ret = virDomainDefFormatInternal(def, caps, NULL, NULL,
> + if (snapshots && !vm) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("snapshots XML requested but not provided"));
Error msg is a bit odd - the error is that a snapshot listing was
desired, but consumer didn't pass the @vm object.
> + goto cleanup;
> + }
> + ret = virDomainDefFormatInternal(def, caps,
> + snapshots ? vm->snapshots : NULL,
> + snapshots ? vm->current_snapshot : NULL,
> virDomainDefFormatConvertXMLFlags(flags),
> buf, driver->xmlopt);
Perhaps this one should [be | have been] turned into a
virDomainDefFormatFull (or whatever new name is chosen if one is
chosen). I think it shows the hazards of exposing *Internal to many
consumers.
>
> @@ -7899,19 +7909,21 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
> unsigned int flags,
> virBufferPtr buf)
> {
> - return qemuDomainDefFormatBufInternal(driver, def, NULL, flags, buf);
> + return qemuDomainDefFormatBufInternal(driver, NULL, def, NULL, flags, buf);
> }
>
>
> static char *
> qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> virDomainDefPtr def,
> virCPUDefPtr origCPU,
> unsigned int flags)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> - if (qemuDomainDefFormatBufInternal(driver, def, origCPU, flags, &buf) < 0)
> + if (qemuDomainDefFormatBufInternal(driver, vm, def, origCPU, flags,
> + &buf) < 0)
Existing; however, considering earlier comment about snapshot list being
filled into the buffer until error, but the *ListFormat not clearing the
buffer on error, I think another patch should be added here to do the
virBufferFreeAndReset(&buf); before return NULL.
> return NULL;
>
> return virBufferContentAndReset(&buf);
> @@ -7923,7 +7935,7 @@ qemuDomainDefFormatXML(virQEMUDriverPtr driver,
> virDomainDefPtr def,
> unsigned int flags)
> {
> - return qemuDomainDefFormatXMLInternal(driver, def, NULL, flags);
> + return qemuDomainDefFormatXMLInternal(driver, NULL, def, NULL, flags);
> }
>
>
> @@ -7942,7 +7954,7 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver,
> origCPU = priv->origCPU;
> }
>
> - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags);
> + return qemuDomainDefFormatXMLInternal(driver, vm, def, origCPU, flags);
> }
>
> char *
> @@ -7959,7 +7971,7 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver,
> if (compatible)
> flags |= VIR_DOMAIN_XML_MIGRATABLE;
>
> - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags);
> + return qemuDomainDefFormatXMLInternal(driver, NULL, def, origCPU, flags);
> }
>
>
> @@ -8386,7 +8398,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
> virDomainSnapshotObjPtr snapshot,
> virCapsPtr caps,
> virDomainXMLOptionPtr xmlopt,
> - char *snapshotDir)
> + const char *snapshotDir)
> {
> char *newxml = NULL;
> int ret = -1;
> @@ -8605,6 +8617,7 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
> virDomainObjPtr vm)
> {
> virQEMUSnapRemove rem;
> + virDomainSnapshotObjPtr snap;
>
> rem.driver = driver;
> rem.vm = vm;
> @@ -8612,6 +8625,12 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
> rem.err = 0;
> virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll,
> &rem);
> + if (rem.current)
> + vm->current_snapshot = NULL;
> + /* Update the metaroot to match that all children were dropped */
> + snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
> + snap->nchildren = 0;
> + snap->first_child = NULL;
>
> return rem.err;
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b3f4dc6f5c..4df9e18e4c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7337,8 +7337,8 @@ static char
> virDomainObjPtr vm;
> char *ret = NULL;
>
> - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU,
> - NULL);
> + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU |
> + VIR_DOMAIN_XML_SNAPSHOTS, NULL);
>
> if (!(vm = qemuDomObjFromDomain(dom)))
> goto cleanup;
> @@ -15733,6 +15733,31 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state,
> return 0;
> }
>
> +/* Struct and hash-iterator callback used when bulk redefining snapshots */
> +struct qemuDomainSnapshotBulk {
> + virDomainObjPtr vm;
> + virQEMUDriverPtr driver;
> + const char *snapshotDir;
> + unsigned int flags;
> +};
> +
> +static int
> +qemuDomainSnapshotBulkRedefine(void *payload, const void *name ATTRIBUTE_UNUSED,
one arg each line
> + void *opaque)
> +{
> + virDomainSnapshotObjPtr snap = payload;
> + struct qemuDomainSnapshotBulk *data = opaque;
> +
> + if (qemuDomainSnapshotValidate(snap->def, snap->def->state,
> + data->flags) < 0)
> + return -1;
> + if (qemuDomainSnapshotWriteMetadata(data->vm, snap, data->driver->caps,
> + data->driver->xmlopt,
> + data->snapshotDir) < 0)
> + return -1;
> + return 0;
> +}
> +
> static virDomainSnapshotPtr
> qemuDomainSnapshotCreateXML(virDomainPtr domain,
> const char *xmlDesc,
> @@ -15762,7 +15787,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
> VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
> VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
> - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
> + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
> + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL);
>
> VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
> VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
> @@ -15794,6 +15820,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> goto cleanup;
> }
>
> + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) {
> + struct qemuDomainSnapshotBulk bulk = {
> + .vm = vm,
> + .driver = driver,
> + .snapshotDir = cfg->snapshotDir,
> + .flags = flags,
> + };
> +
> + if (virDomainSnapshotDefParseList(xmlDesc, vm->def->uuid,
> + vm->snapshots, &vm->current_snapshot,
> + caps, driver->xmlopt,
> + parse_flags) < 0)
> + goto cleanup;
> + /* Validate and save the snapshots to disk. Since we don't get
> + * here unless there were no snapshots beforehand, just delete
> + * everything if anything failed, ignoring further errors. */
> + if (virDomainSnapshotForEach(vm->snapshots,
> + qemuDomainSnapshotBulkRedefine,
> + &bulk) < 0) {
> + virErrorPtr orig_err = virSaveLastError();
> +
> + qemuDomainSnapshotDiscardAllMetadata(driver, vm);
> + virSetError(orig_err);
> + virFreeError(orig_err);
> + goto cleanup;
> + }
> + /* Return is arbitrary, so use the first root */
> + snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
Similar to test driver - this isn't used during cleanup.
John
> + snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name);
> + goto cleanup;
> + }
> +
> if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot halt after transient domain snapshot"));
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
