Extract code that deletes children of specific snapshot to separate
function.

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 47239e9e9c..3d5467457b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2311,18 +2311,76 @@ qemuSnapshotDeleteSingle(virDomainObj *vm,
 }
 
 
-int
-qemuSnapshotDelete(virDomainObj *vm,
-                   virDomainSnapshotPtr snapshot,
-                   unsigned int flags)
+/**
+ * qemuSnapshotDeleteChildren:
+ * @vm: domain object
+ * @snap: snapshot object
+ * @metadata_only: boolean
+ * @flags: bitwise-OR of virDomainSnapshotDeleteFlags
+ *
+ * Delete children snapshots of snapshot proved by @snap. Based on what @flags
+ * are provided it will delete children snapshots including @snap or only
+ * children snapshots. If @metadata_only is true only libvirt metadata files
+ * are deleted but the actual snapshots are left intact.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+static int
+qemuSnapshotDeleteChildren(virDomainObj *vm,
+                           virDomainMomentObj *snap,
+                           bool metadata_only,
+                           unsigned int flags)
 {
-    virQEMUDriver *driver = snapshot->domain->conn->privateData;
-    int ret = -1;
-    virDomainMomentObj *snap = NULL;
     virQEMUMomentRemove rem;
+    qemuDomainObjPrivate *priv = vm->privateData;
+    virQEMUDriver *driver = priv->driver;
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
+    rem.driver = driver;
+    rem.vm = vm;
+    rem.metadata_only = metadata_only;
+    rem.err = 0;
+    rem.current = virDomainSnapshotGetCurrent(vm->snapshots);
+    rem.found = false;
+    rem.momentDiscard = qemuDomainSnapshotDiscard;
+    virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll,
+                                     &rem);
+    if (rem.err < 0)
+        return -1;
+    if (rem.found) {
+        qemuSnapshotSetCurrent(vm, snap);
+
+        if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
+            if (qemuDomainSnapshotWriteMetadata(vm, snap,
+                                                driver->xmlopt,
+                                                cfg->snapshotDir) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("failed to set snapshot '%s' as current"),
+                               snap->def->name);
+                virDomainSnapshotSetCurrent(vm->snapshots, NULL);
+                return -1;
+            }
+        }
+    }
+
+    if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
+        virDomainMomentDropChildren(snap);
+        return 0;
+    }
+
+    return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
+}
+
+
+int
+qemuSnapshotDelete(virDomainObj *vm,
+                   virDomainSnapshotPtr snapshot,
+                   unsigned int flags)
+{
+    int ret = -1;
+    virDomainMomentObj *snap = NULL;
     bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY);
     int external = 0;
-    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 
     virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
                   VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY |
@@ -2353,39 +2411,7 @@ qemuSnapshotDelete(virDomainObj *vm,
 
     if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
                  VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
-        rem.driver = driver;
-        rem.vm = vm;
-        rem.metadata_only = metadata_only;
-        rem.err = 0;
-        rem.current = virDomainSnapshotGetCurrent(vm->snapshots);
-        rem.found = false;
-        rem.momentDiscard = qemuDomainSnapshotDiscard;
-        virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll,
-                                         &rem);
-        if (rem.err < 0)
-            goto endjob;
-        if (rem.found) {
-            qemuSnapshotSetCurrent(vm, snap);
-
-            if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
-                if (qemuDomainSnapshotWriteMetadata(vm, snap,
-                                                    driver->xmlopt,
-                                                    cfg->snapshotDir) < 0) {
-                    virReportError(VIR_ERR_INTERNAL_ERROR,
-                                   _("failed to set snapshot '%s' as current"),
-                                   snap->def->name);
-                    virDomainSnapshotSetCurrent(vm->snapshots, NULL);
-                    goto endjob;
-                }
-            }
-        }
-
-        if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
-            virDomainMomentDropChildren(snap);
-            ret = 0;
-        } else {
-            ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
-        }
+        ret = qemuSnapshotDeleteChildren(vm, snap, metadata_only, flags);
     } else {
         ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only);
     }
-- 
2.38.1

On Thu, Dec 08, 2022 at 14:30:50 +0100, Pavel Hrdina wrote:
> Extract code that deletes children of specific snapshot to separate
> function.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 108 ++++++++++++++++++++++++---------------
>  1 file changed, 67 insertions(+), 41 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 47239e9e9c..3d5467457b 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2311,18 +2311,76 @@ qemuSnapshotDeleteSingle(virDomainObj *vm,
>  }
>  
>  
> -int
> -qemuSnapshotDelete(virDomainObj *vm,
> -                   virDomainSnapshotPtr snapshot,
> -                   unsigned int flags)
> +/**
> + * qemuSnapshotDeleteChildren:
> + * @vm: domain object
> + * @snap: snapshot object
> + * @metadata_only: boolean

'boolean' is quite obvious from the function prototype, but it's not
that obvious what it actually means.

@metadata_only: If true, delete only snapshot metadata, leave data
intact.

> + * @flags: bitwise-OR of virDomainSnapshotDeleteFlags

In fact the only thing you use from @flags is
VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY. Additionally you already
pass 'metadata_only' as boolean which is originally also part of 'flags.

Preferrably extract VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY as a
boolean too and pass it to this function instead of flags.

> + *
> + * Delete children snapshots of snapshot proved by @snap. Based on what @flags

s/proved/provided/ ?

> + * are provided it will delete children snapshots including @snap or only
> + * children snapshots. If @metadata_only is true only libvirt metadata files
> + * are deleted but the actual snapshots are left intact.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +static int
> +qemuSnapshotDeleteChildren(virDomainObj *vm,
> +                           virDomainMomentObj *snap,
> +                           bool metadata_only,
> +                           unsigned int flags)

With the above:

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