snapshot: add qemu snapshot redefine support

Redefining a qemu snapshot requires a bit of a tweak to the common
snapshot parsing code, but the end result is quite nice.

Be careful that redefinitions do not introduce circular parent
chains.  Also, we don't want to allow conversion between online
and offline existing snapshots.  We could probably do some more
validation for snapshots that don't already exist to make sure
they are even feasible, by parsing qemu-img output, but that
can come later.

* src/conf/domain_conf.h (virDomainSnapshotParseFlags): New
internal flags.
* src/conf/domain_conf.c (virDomainSnapshotDefParseString): Alter
signature to take internal flags.
* src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Update caller.
* src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support
new public flags.

src/conf/domain_conf.c

@@ -11375,8 +11375,9 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
     VIR_FREE(def);
 }
 
-virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
-                                                        int newSnapshot)
+/* flags are from virDomainSnapshotParseFlags */
+virDomainSnapshotDefPtr
+virDomainSnapshotDefParseString(const char *xmlStr, unsigned int flags)
 {
     xmlXPathContextPtr ctxt = NULL;
     xmlDocPtr xml = NULL;
@@ -11404,8 +11405,16 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
     gettimeofday(&tv, NULL);
 
     def->name = virXPathString("string(./name)", ctxt);
-    if (def->name == NULL)
-        ignore_value(virAsprintf(&def->name, "%lld", (long long)tv.tv_sec));
+    if (def->name == NULL) {
+        if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) {
+            virDomainReportError(VIR_ERR_XML_ERROR, "%s",
+                                 _("a redefined snapshot must have a name"));
+            goto cleanup;
+        } else {
+            ignore_value(virAsprintf(&def->name, "%lld",
+                                     (long long)tv.tv_sec));
+        }
+    }
 
     if (def->name == NULL) {
         virReportOOMError();
@@ -11414,7 +11423,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
 
     def->description = virXPathString("string(./description)", ctxt);
 
-    if (!newSnapshot) {
+    if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) {
         if (virXPathLongLong("string(./creationTime)", ctxt,
                              &def->creationTime) < 0) {
             virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -11440,7 +11449,11 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
                                  state);
             goto cleanup;
         }
+    } else {
+        def->creationTime = tv.tv_sec;
+    }
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) {
         if (virXPathInt("string(./active)", ctxt, &active) < 0) {
             virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                                  _("Could not find 'active' element"));
@@ -11448,8 +11461,6 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
         }
         def->current = active != 0;
     }
-    else
-        def->creationTime = tv.tv_sec;
 
     ret = def;
 

src/conf/domain_conf.h

@@ -1407,8 +1407,13 @@ struct _virDomainSnapshotObjList {
     virHashTable *objs;
 };
 
+typedef enum {
+    VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0,
+    VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 1,
+} virDomainSnapshotParseFlags;
+
 virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
-                                                        int newSnapshot);
+                                                        unsigned int flags);
 void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def);
 char *virDomainSnapshotDefFormat(char *domain_uuid,
                                  virDomainSnapshotDefPtr def,

src/esx/esx_driver.c

@@ -4217,7 +4217,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc,
         return NULL;
     }
 
-    def = virDomainSnapshotDefParseString(xmlDesc, 1);
+    def = virDomainSnapshotDefParseString(xmlDesc, 0);
 
     if (def == NULL) {
         return NULL;

src/qemu/qemu_driver.c

@@ -297,6 +297,8 @@ static void qemuDomainSnapshotLoad(void *payload,
     virDomainSnapshotObjPtr snap = NULL;
     virDomainSnapshotObjPtr current = NULL;
     char ebuf[1024];
+    unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE |
+                          VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL);
 
     virDomainObjLock(vm);
     if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) {
@@ -338,7 +340,7 @@ static void qemuDomainSnapshotLoad(void *payload,
             continue;
         }
 
-        def = virDomainSnapshotDefParseString(xmlStr, 0);
+        def = virDomainSnapshotDefParseString(xmlStr, flags);
         if (def == NULL) {
             /* Nothing we can do here, skip this one */
             VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"),
@@ -8619,8 +8621,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
     virDomainSnapshotPtr snapshot = NULL;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     virDomainSnapshotDefPtr def = NULL;
+    bool update_current = true;
+    unsigned int parse_flags = 0;
 
-    virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL);
+    virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
+                  VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT |
+                  VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL);
+
+    if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) &&
+         !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) ||
+        (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA))
+        update_current = false;
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)
+        parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE;
 
     qemuDriverLock(driver);
     virUUIDFormat(domain->uuid, uuidstr);
@@ -8647,22 +8660,87 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
     if (!qemuDomainSnapshotIsAllowed(vm))
         goto cleanup;
 
-    if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1)))
+    if (!(def = virDomainSnapshotDefParseString(xmlDesc, parse_flags)))
         goto cleanup;
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) {
+        virDomainSnapshotObjPtr other = NULL;
+
+        /* Prevent circular chains */
+        if (def->parent) {
+            if (STREQ(def->name, def->parent)) {
+                qemuReportError(VIR_ERR_INVALID_ARG,
+                                _("cannot set snapshot %s as its own parent"),
+                                def->name);
+                goto cleanup;
+            }
+            other = virDomainSnapshotFindByName(&vm->snapshots, def->parent);
+            if (!other) {
+                qemuReportError(VIR_ERR_INVALID_ARG,
+                                _("parent %s for snapshot %s not found"),
+                                def->parent, def->name);
+                goto cleanup;
+            }
+            while (other->def->parent) {
+                if (STREQ(other->def->parent, def->name)) {
+                    qemuReportError(VIR_ERR_INVALID_ARG,
+                                    _("parent %s would create cycle to %s"),
+                                    other->def->name, def->name);
+                    goto cleanup;
+                }
+                other = virDomainSnapshotFindByName(&vm->snapshots,
+                                                    other->def->parent);
+                if (!other) {
+                    VIR_WARN("snapshots are inconsistent for %s",
+                             vm->def->name);
+                    break;
+                }
+            }
+        }
+
+        /* Check that any replacement is compatible */
+        other = virDomainSnapshotFindByName(&vm->snapshots, def->name);
+        if (other) {
+            if ((other->def->state == VIR_DOMAIN_RUNNING ||
+                 other->def->state == VIR_DOMAIN_PAUSED) !=
+                (def->state == VIR_DOMAIN_RUNNING ||
+                 def->state == VIR_DOMAIN_PAUSED)) {
+                qemuReportError(VIR_ERR_INVALID_ARG,
+                                _("cannot change between online and offline "
+                                  "snapshot state in snapshot %s"),
+                                def->name);
+                goto cleanup;
+            }
+            /* XXX Ensure ABI compatibility before replacing anything.  */
+            if (other == vm->current_snapshot) {
+                update_current = true;
+                vm->current_snapshot = NULL;
+            }
+            virDomainSnapshotObjListRemove(&vm->snapshots, other);
+        } else {
+            /* XXX Should we do some feasibility checks, like parsing
+             * qemu-img output to check that def->name matches at
+             * least one qcow2 snapshot name?  */
+        }
+    }
+
     if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def)))
         goto cleanup;
     def = NULL;
 
-    snap->def->state = virDomainObjGetState(vm, NULL);
+    if (update_current)
         snap->def->current = true;
+    if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE))
+        snap->def->state = virDomainObjGetState(vm, NULL);
     if (vm->current_snapshot) {
+        if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) {
             snap->def->parent = strdup(vm->current_snapshot->def->name);
             if (snap->def->parent == NULL) {
                 virReportOOMError();
                 goto cleanup;
             }
-        if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+        }
+        if (update_current) {
             vm->current_snapshot->def->current = false;
             if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot,
                                                 driver->snapshotDir) < 0)
@@ -8672,7 +8750,13 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
     }
 
     /* actually do the snapshot */
-    if (!virDomainObjIsActive(vm)) {
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) {
+        /* XXX Should we validate that the redefined snapshot even
+         * makes sense, such as checking whether the requested parent
+         * snapshot exists and is not creating a loop, or that
+         * qemu-img recognizes the snapshot name in at least one of
+         * the domain's disks?  */
+    } else if (!virDomainObjIsActive(vm)) {
         if (qemuDomainSnapshotCreateInactive(vm, snap) < 0)
             goto cleanup;
     } else {
@@ -8694,7 +8778,7 @@ cleanup:
                                                 driver->snapshotDir) < 0)
                 VIR_WARN("unable to save metadata for snapshot %s",
                          snap->def->name);
-            else
+            else if (update_current)
                 vm->current_snapshot = snap;
         } else if (snap) {
             virDomainSnapshotObjListRemove(&vm->snapshots, snap);

src/vbox/vbox_tmpl.c

@@ -5655,7 +5655,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
     /* VBox has no snapshot metadata, so this flag is trivial.  */
     virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL);
 
-    if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1)))
+    if (!(def = virDomainSnapshotDefParseString(xmlDesc, 0)))
         goto cleanup;
 
     vboxIIDFromUUID(&domiid, dom->uuid);