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