[libvirt RFC 15/24] qemu_snapshot: call qemuSnapshotDiscardMetadata from qemuSnapshotDiscard

Peter Krempa pkrempa at redhat.com
Thu Sep 1 14:20:25 UTC 2022


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


More information about the libvir-list mailing list