snapshot: Factor out redefine cycle validation

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@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>

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) {

src/conf/virdomainmomentobjlist.c

@@ -519,3 +519,47 @@ virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments)
         moments->current = NULL;
     return act.err;
 }
+
+
+/* Check that inserting def into list would not create any impossible
+ * parent-child relationships (cycles or missing parents).  Return 0
+ * on success, or report an error on behalf of domname before
+ * returning -1. */
+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;
+}

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);

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,

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     | \

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