[PATCH 2/2] snapshot: simplify redefinition of disk snapshot

[Eric Blake <eblake@xxxxxxxxxx>]

Redefining disk-only snapshot xml should work even if the user
did not explicitly pass VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
the flag is only required for conditions where the <state>
subelement is not already present in parsing (that is, defining
a new snapshot).

Also, fix the error code of some user-visible errors (the remaining
VIR_ERR_INTERNAL_ERROR should not be user-visible, since parsing
of <active> is only done from internal code).

* src/conf/domain_conf.c (virDomainSnapshotDefParseString): Allow
disks during redefinition of disk snapshot.
---
 src/conf/domain_conf.c |   44 +++++++++++++++++++++++---------------------
 1 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f9007ce..34bf2d0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11696,25 +11696,6 @@ virDomainSnapshotDefParseString(const char *xmlStr,

     def->description = virXPathString("string(./description)", ctxt);

-    if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
-        goto cleanup;
-    if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) {
-        def->ndisks = i;
-        if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) {
-            virReportOOMError();
-            goto cleanup;
-        }
-        for (i = 0; i < def->ndisks; i++) {
-            if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0)
-                goto cleanup;
-        }
-        VIR_FREE(nodes);
-    } else if (i) {
-        virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
-                             _("unable to handle disk requests in snapshot"));
-        goto cleanup;
-    }
-
     if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) {
         if (virXPathLongLong("string(./creationTime)", ctxt,
                              &def->creationTime) < 0) {
@@ -11730,13 +11711,13 @@ virDomainSnapshotDefParseString(const char *xmlStr,
             /* there was no state in an existing snapshot; this
              * should never happen
              */
-            virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+            virDomainReportError(VIR_ERR_XML_ERROR, "%s",
                                  _("missing state from existing snapshot"));
             goto cleanup;
         }
         def->state = virDomainSnapshotStateTypeFromString(state);
         if (def->state < 0) {
-            virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+            virDomainReportError(VIR_ERR_XML_ERROR,
                                  _("Invalid state '%s' in domain snapshot XML"),
                                  state);
             goto cleanup;
@@ -11768,6 +11749,27 @@ virDomainSnapshotDefParseString(const char *xmlStr,
         def->creationTime = tv.tv_sec;
     }

+    if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
+        goto cleanup;
+    if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS ||
+        (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE &&
+         def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
+        def->ndisks = i;
+        if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) {
+            virReportOOMError();
+            goto cleanup;
+        }
+        for (i = 0; i < def->ndisks; i++) {
+            if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0)
+                goto cleanup;
+        }
+        VIR_FREE(nodes);
+    } else if (i) {
+        virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                             _("unable to handle disk requests in snapshot"));
+        goto cleanup;
+    }
+
     if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) {
         if (virXPathInt("string(./active)", ctxt, &active) < 0) {
             virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-- 
1.7.4.4

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list