[libvirt] [PATCH 5/8] snapshot: Factor out redefine cycle validation

[Eric Blake]

The code to check whether a redefined snapshot/checkpoint XML is
attempting to create a cycle in the list of moments is lengthy, and
common between the two types of list. Therefore, it belongs in the
shared base file.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 src/conf/virdomainmomentobjlist.h   |  3 +++
 src/conf/virdomainsnapshotobjlist.h |  3 +++
 src/conf/snapshot_conf.c            | 41 ++++-------------------------
 src/conf/virdomainmomentobjlist.c   | 40 ++++++++++++++++++++++++++++
 src/conf/virdomainsnapshotobjlist.c |  9 +++++++
 tests/virsh-snapshot                |  2 +-
 6 files changed, 61 insertions(+), 37 deletions(-)

diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h
index 68f00a114f..4067e928f4 100644
--- a/src/conf/virdomainmomentobjlist.h
+++ b/src/conf/virdomainmomentobjlist.h
@@ -117,3 +117,6 @@ int virDomainMomentForEach(virDomainMomentObjListPtr moments,
                            virHashIterator iter,
                            void *data);
 int virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments);
+int virDomainMomentCheckCycles(virDomainMomentObjListPtr list,
+                               virDomainMomentDefPtr def,
+                               const char *domname);
diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index 26fa456730..fed8d22bc8 100644
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -52,6 +52,9 @@ int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
                              virHashIterator iter,
                              void *data);
 int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots);
+int virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots,
+                                 virDomainSnapshotDefPtr def,
+                                 const char *domname);

 #define VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA \
                (VIR_DOMAIN_SNAPSHOT_LIST_METADATA     | \
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index daea6c616d..3521177f0b 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -966,46 +966,15 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
 {
     virDomainSnapshotDefPtr def = *defptr;
     virDomainMomentObjPtr other;
-    virDomainSnapshotDefPtr otherdef;
+    virDomainSnapshotDefPtr otherdef = NULL;
     bool check_if_stolen;

-    /* Prevent circular chains */
-    if (def->parent.parent_name) {
-        if (STREQ(def->parent.name, def->parent.parent_name)) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("cannot set snapshot %s as its own parent"),
-                           def->parent.name);
-            return -1;
-        }
-        other = virDomainSnapshotFindByName(vm->snapshots,
-                                            def->parent.parent_name);
-        if (!other) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("parent %s for snapshot %s not found"),
-                           def->parent.parent_name, def->parent.name);
-            return -1;
-        }
-        otherdef = virDomainSnapshotObjGetDef(other);
-        while (otherdef->parent.parent_name) {
-            if (STREQ(otherdef->parent.parent_name, def->parent.name)) {
-                virReportError(VIR_ERR_INVALID_ARG,
-                               _("parent %s would create cycle to %s"),
-                               otherdef->parent.name, def->parent.name);
-                return -1;
-            }
-            other = virDomainSnapshotFindByName(vm->snapshots,
-                                                otherdef->parent.parent_name);
-            if (!other) {
-                VIR_WARN("snapshots are inconsistent for %s",
-                         vm->def->name);
-                break;
-            }
-            otherdef = virDomainSnapshotObjGetDef(other);
-        }
-    }
+    if (virDomainSnapshotCheckCycles(vm->snapshots, def, vm->def->name) < 0)
+        return -1;

     other = virDomainSnapshotFindByName(vm->snapshots, def->parent.name);
-    otherdef = other ? virDomainSnapshotObjGetDef(other) : NULL;
+    if (other)
+        otherdef = virDomainSnapshotObjGetDef(other);
     check_if_stolen = other && otherdef->parent.dom;
     if (virDomainSnapshotRedefineValidate(def, domain->uuid, other, xmlopt,
                                           flags) < 0) {
diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
index f56b516343..0ea5e9af80 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -519,3 +519,43 @@ virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments)
         moments->current = NULL;
     return act.err;
 }
+
+
+int
+virDomainMomentCheckCycles(virDomainMomentObjListPtr list,
+                           virDomainMomentDefPtr def,
+                           const char *domname)
+{
+    virDomainMomentObjPtr other;
+
+    if (def->parent_name) {
+        if (STREQ(def->name, def->parent_name)) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("cannot set moment %s as its own parent"),
+                           def->name);
+            return -1;
+        }
+        other = virDomainMomentFindByName(list, def->parent_name);
+        if (!other) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("parent %s for moment %s not found"),
+                           def->parent_name, def->name);
+            return -1;
+        }
+        while (other->def->parent_name) {
+            if (STREQ(other->def->parent_name, def->name)) {
+                virReportError(VIR_ERR_INVALID_ARG,
+                               _("parent %s would create cycle to %s"),
+                               other->def->name, def->name);
+                return -1;
+            }
+            other = virDomainMomentFindByName(list, other->def->parent_name);
+            if (!other) {
+                VIR_WARN("moments are inconsistent for domain %s",
+                         domname);
+                break;
+            }
+        }
+    }
+    return 0;
+}
diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
index 9bcc8d1036..99bc4bb0c5 100644
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
@@ -234,6 +234,15 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots)
 }


+int
+virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots,
+                             virDomainSnapshotDefPtr def,
+                             const char *domname)
+{
+    return virDomainMomentCheckCycles(snapshots->base, &def->parent, domname);
+}
+
+
 int
 virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots,
                        virDomainMomentObjPtr from,
diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot
index 8eab67c9e0..510904f470 100755
--- a/tests/virsh-snapshot
+++ b/tests/virsh-snapshot
@@ -206,7 +206,7 @@ Metadata:       yes
 EOF

 cat <<EOF > exp || fail=1
-error: invalid argument: parent s3 for snapshot s2 not found
+error: invalid argument: parent s3 for moment s2 not found
 error: marker
 EOF
 compare exp err || fail=1
-- 
2.20.1