Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_snapshot.c | 46 ++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index c1c67ac445..b5e6a87566 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2281,6 +2281,35 @@ qemuSnapshotChildrenReparent(void *payload,
 }
 
 
+static int
+qemuSnapshotDeleteSingle(virDomainObj *vm,
+                         virDomainMomentObj *snap,
+                         virQEMUDriver *driver,
+                         bool metadata_only)
+{
+    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 qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
+}
+
+
 int
 qemuSnapshotDelete(virDomainObj *vm,
                    virDomainSnapshotPtr snapshot,
@@ -2290,7 +2319,6 @@ qemuSnapshotDelete(virDomainObj *vm,
     int ret = -1;
     virDomainMomentObj *snap = NULL;
     virQEMUMomentRemove rem;
-    virQEMUMomentReparent rep;
     bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY);
     int external = 0;
     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -2358,21 +2386,7 @@ qemuSnapshotDelete(virDomainObj *vm,
             ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
         }
     } else {
-        if (snap->nchildren) {
-            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)
-                goto endjob;
-            virDomainMomentMoveChildren(snap, snap->parent);
-        }
-        ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
+        ret = qemuSnapshotDeleteSingle(vm, snap, driver, metadata_only);
     }
 
  endjob:
-- 
2.37.2

On Tue, Aug 23, 2022 at 18:32:13 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 46 ++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index c1c67ac445..b5e6a87566 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2281,6 +2281,35 @@ qemuSnapshotChildrenReparent(void *payload,
>  }
>  
>  
> +static int
> +qemuSnapshotDeleteSingle(virDomainObj *vm,
> +                         virDomainMomentObj *snap,
> +                         virQEMUDriver *driver,
> +                         bool metadata_only)
> +{

'driver' can be extracted from the the private data of 'vm', removing
the need to pass it in. Doing that will also remove the need to pass it
in via 'struct qemuSnapshotDeleteAllData' in upcoming patch.

> +    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 qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);

You actually may be better off refactoring qemuDomainSnapshotDiscard
first to remove the argument.

On Thu, Sep 01, 2022 at 03:53:31PM +0200, Peter Krempa wrote:
> On Tue, Aug 23, 2022 at 18:32:13 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/qemu/qemu_snapshot.c | 46 ++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index c1c67ac445..b5e6a87566 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -2281,6 +2281,35 @@ qemuSnapshotChildrenReparent(void *payload,
> >  }
> >  
> >  
> > +static int
> > +qemuSnapshotDeleteSingle(virDomainObj *vm,
> > +                         virDomainMomentObj *snap,
> > +                         virQEMUDriver *driver,
> > +                         bool metadata_only)
> > +{
> 
> 'driver' can be extracted from the the private data of 'vm', removing
> the need to pass it in. Doing that will also remove the need to pass it
> in via 'struct qemuSnapshotDeleteAllData' in upcoming patch.

I'll drop it from this function, good point, thanks.

> > +    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 qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
> 
> You actually may be better off refactoring qemuDomainSnapshotDiscard
> first to remove the argument.

This can be done as a followup series otherwise it will open another
can of worms and would end up in a lot of patches.

Pavel

On Tue, Aug 23, 2022 at 18:32:13 +0200, Pavel Hrdina wrote:

...

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 46 ++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 16 deletions(-)

With a commit message:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>