Re: [libvirt] [PATCH v2 11/11] qemu: Implement bulk snapshot operations

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

> @@ -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.

> + /* 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);