On 2/27/19 11:27 AM, John Ferlan wrote:
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@redhat.com> --- 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.
My fault for too many patches in flight at once. As I'll be doing a v3 anyways, that one wil be cleaner.
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.
Yeah, I debated about it. Since you called me on it, I'll make the split.
@@ -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.
It's a programmer's error, not user-triggerable. (If we could assert() or abort(), that's what I'd have done instead). And really, it is no snapshots were provided, because it is vm->snapshots that we depend on.
+ 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.
It's a ripple effect - I had to decide how many interfaces needed an additional parameter, even if the caller wouldn't be using it. But your idea of an auxiliary struct may make it nicer.
+ /* 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);
Yeah, but it's not leaked, and it let me avoid a long line. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org