On Tue, Aug 23, 2022 at 18:32:18 +0200, Pavel Hrdina wrote:
This changes the snapshot delete call order. Previously we did snapshot XML reparenting before the actual snapshot deletion.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 64 +++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 31 deletions(-)
Okay, I now understand why you've needed to extrac the code, but it doesn't make it less confusing for the future in any way. You need to pick a better name and add comment for the function.
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b94506c177..cbacb05c16 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2281,6 +2281,34 @@ qemuSnapshotChildrenReparent(void *payload, }
+static int +qemuSnapshotDiscardMetadata(virDomainObj *vm, + virDomainMomentObj *snap, + virQEMUDriver *driver)
Since you need to move the function please put it in the correct place at the time you've extracted it. Additionally as mentioned @driver can be fetched from @vm so you can make the header simpler.
+{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + if (snap->nchildren) { + virQEMUMomentReparent rep; + + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); + } + + return 0; +} + + /* Discard one snapshot (or its metadata), without reparenting any children. */ static int qemuSnapshotDiscard(virQEMUDriver *driver, @@ -2323,6 +2351,11 @@ qemuSnapshotDiscard(virQEMUDriver *driver, } }
+ if (update_parent && + qemuSnapshotDiscardMetadata(vm, snap, driver) < 0) {
Hmm, so you are planning on moving also ...
+ return -1; + } + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, snap->def->name);
... this code into that function? That will make sense at the end, but I think you approached it from the wrong end. I'd start with extracting the metadata deletion first and then move the reparenting of children into it later.