snapshot: Rework virDomainSnapshotState enum

The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.

Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update most clients to use the new
enum names anywhere snapshot state is referenced. The exception is
two switch statements in qemu code, which instead gain a fixme
comment about odd type usage (which will be cleaned up in the next
patch). The rest of the patch is fairly mechanical (I actually did
it by temporarily s/state/xstate/ in snapshot_conf.h to let the
compiler find which spots in the code used the field, did the
obvious search and replace in those functions, then undid the rename).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 41236d9932a01201a9ecd2157a367a3b34f4e77f..054721012cad839bf45c1f094de3e670a17f23a8 100644 (file)
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -58,7 +58,7 @@ VIR_ENUM_IMPL(virDomainSnapshotLocation, VIR_DOMAIN_SNAPSHOT_LOCATION_LAST,
 );
 
 /* virDomainSnapshotState is really virDomainState plus one extra state */
-VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST,
+VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_LAST,
               "nostate",
               "running",
               "blocked",
@@ -257,8 +257,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
                            state);
             goto cleanup;
         }
-        offline = (def->state == VIR_DOMAIN_SHUTOFF ||
-                   def->state == VIR_DOMAIN_DISK_SNAPSHOT);
+        offline = (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ||
+                   def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT);
 
         /* Older snapshots were created with just <domain>/<uuid>, and
          * lack domain/@type.  In that case, leave dom NULL, and
@@ -879,14 +879,14 @@ static int virDomainSnapshotObjListCopyNames(void *payload,
 
     if (data->flags & VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS) {
         if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE) &&
-            obj->def->state == VIR_DOMAIN_SHUTOFF)
+            obj->def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF)
             return 0;
         if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) &&
-            obj->def->state == VIR_DOMAIN_DISK_SNAPSHOT)
+            obj->def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT)
             return 0;
         if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE) &&
-            obj->def->state != VIR_DOMAIN_SHUTOFF &&
-            obj->def->state != VIR_DOMAIN_DISK_SNAPSHOT)
+            obj->def->state != VIR_DOMAIN_SNAPSHOT_SHUTOFF &&
+            obj->def->state != VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT)
             return 0;
     }
 
@@ -1225,7 +1225,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
     int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
     bool align_match = true;
     virDomainSnapshotObjPtr other;
-    bool external = def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+    bool external = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT ||
         virDomainSnapshotDefIsExternal(def);
 
     /* Prevent circular chains */
@@ -1282,10 +1282,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
 
     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)) {
+        if ((other->def->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+             other->def->state == VIR_DOMAIN_SNAPSHOT_PAUSED) !=
+            (def->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+             def->state == VIR_DOMAIN_SNAPSHOT_PAUSED)) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("cannot change between online and offline "
                              "snapshot state in snapshot %s"),
@@ -1293,8 +1293,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
             goto cleanup;
         }
 
-        if ((other->def->state == VIR_DOMAIN_DISK_SNAPSHOT) !=
-            (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
+        if ((other->def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT) !=
+            (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT)) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("cannot change between disk only and "
                              "full system in snapshot %s"),

diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 7a175dfc961203b025e1aea5b6fd82429d079815..5f2cda8848d90eca7a397d17ea1163bc761f1fe0 100644 (file)
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -1,7 +1,7 @@
 /*
  * snapshot_conf.h: domain snapshot XML processing
  *
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -36,11 +36,26 @@ typedef enum {
     VIR_DOMAIN_SNAPSHOT_LOCATION_LAST
 } virDomainSnapshotLocation;
 
+/**
+ * This enum has to map all known domain states from the public enum
+ * virDomainState, before adding one additional state possible only
+ * for snapshots.
+ */
 typedef enum {
-    /* Inherit the VIR_DOMAIN_* states from virDomainState.  */
-    VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
-    VIR_DOMAIN_SNAPSHOT_STATE_LAST
+    /* Mapped to public enum */
+    VIR_DOMAIN_SNAPSHOT_NOSTATE = VIR_DOMAIN_NOSTATE,
+    VIR_DOMAIN_SNAPSHOT_RUNNING = VIR_DOMAIN_RUNNING,
+    VIR_DOMAIN_SNAPSHOT_BLOCKED = VIR_DOMAIN_BLOCKED,
+    VIR_DOMAIN_SNAPSHOT_PAUSED = VIR_DOMAIN_PAUSED,
+    VIR_DOMAIN_SNAPSHOT_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
+    VIR_DOMAIN_SNAPSHOT_SHUTOFF = VIR_DOMAIN_SHUTOFF,
+    VIR_DOMAIN_SNAPSHOT_CRASHED = VIR_DOMAIN_CRASHED,
+    VIR_DOMAIN_SNAPSHOT_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
+    /* Additional enum values local to snapshots */
+    VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT,
+    VIR_DOMAIN_SNAPSHOT_LAST
 } virDomainSnapshotState;
+verify((int)VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
 
 /* Stores disk-snapshot information */
 typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 95068d91b63fe6982dc83882c12088d01f8fa629..96415a38ca25b73039188f03633e269fc89d8996 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15010,7 +15010,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
         case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
             found_internal = true;
 
-            if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && active) {
+            if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && active) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("active qemu domains require external disk "
                                  "snapshots; disk %s requested internal"),
@@ -15087,7 +15087,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
     }
 
     /* disk snapshot requires at least one disk */
-    if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && !external) {
+    if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && !external) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("disk-only snapshots require at least "
                          "one disk to be selected for snapshot"));
@@ -15769,8 +15769,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     /* allow snapshots only in certain states */
     state = vm->state.state;
     if (redefine)
-        state = def->state == VIR_DOMAIN_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF :
+        state = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF :
             def->state;
+    /* FIXME: state should be virDomainSnapshotState, with the switch
+     * statement handling of VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT (the only
+     * enum value added beyond what virDomainState supports). But for
+     * now it doesn't matter, because we slammed the extra snapshot
+     * state into a safe domain state. */
     switch (state) {
         /* valid states */
     case VIR_DOMAIN_RUNNING:
@@ -15826,9 +15831,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
             align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
             align_match = false;
             if (virDomainObjIsActive(vm))
-                def->state = VIR_DOMAIN_DISK_SNAPSHOT;
+                def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT;
             else
-                def->state = VIR_DOMAIN_SHUTOFF;
+                def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
             def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
         } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
             def->state = virDomainObjGetState(vm, NULL);
@@ -15845,7 +15850,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
                 goto endjob;
             }
 
-            def->memory = (def->state == VIR_DOMAIN_SHUTOFF ?
+            def->memory = (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ?
                            VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
                            VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
         }
@@ -16408,8 +16413,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
         goto endjob;
 
     if (!vm->persistent &&
-        snap->def->state != VIR_DOMAIN_RUNNING &&
-        snap->def->state != VIR_DOMAIN_PAUSED &&
+        snap->def->state != VIR_DOMAIN_SNAPSHOT_RUNNING &&
+        snap->def->state != VIR_DOMAIN_SNAPSHOT_PAUSED &&
         (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                   VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -16432,8 +16437,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
             goto endjob;
         }
         if (virDomainObjIsActive(vm) &&
-            !(snap->def->state == VIR_DOMAIN_RUNNING
-              || snap->def->state == VIR_DOMAIN_PAUSED) &&
+            !(snap->def->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+              snap->def->state == VIR_DOMAIN_SNAPSHOT_PAUSED) &&
             (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
             virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
@@ -16469,6 +16474,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
 
     cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
 
+    /* FIXME: This cast should be to virDomainSnapshotState, with
+     * better handling of VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT (the only enum
+     * value added beyond what virDomainState supports). But for now
+     * it doesn't matter, because of the above rejection of revert to
+     * external snapshots. */
     switch ((virDomainState) snap->def->state) {
     case VIR_DOMAIN_RUNNING:
     case VIR_DOMAIN_PAUSED:
@@ -16610,7 +16620,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
 
         /* Touch up domain state.  */
         if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) &&
-            (snap->def->state == VIR_DOMAIN_PAUSED ||
+            (snap->def->state == VIR_DOMAIN_SNAPSHOT_PAUSED ||
              (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
             /* Transitions 3, 6, 9 */
             virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
@@ -16717,7 +16727,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Invalid target domain state '%s'. Refusing "
                          "snapshot reversion"),
-                       virDomainStateTypeToString(snap->def->state));
+                       virDomainSnapshotStateTypeToString(snap->def->state));
         goto endjob;
     }
 

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ce0df1f8e3018b456d921835ccf82dfe9e8521a4..490a423b9697cb94fe028a9340a6c06b7525123a 100644 (file)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6271,9 +6271,9 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
         align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
         align_match = false;
         if (virDomainObjIsActive(vm))
-            def->state = VIR_DOMAIN_DISK_SNAPSHOT;
+            def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT;
         else
-            def->state = VIR_DOMAIN_SHUTOFF;
+            def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
         def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
     } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
         def->state = virDomainObjGetState(vm, NULL);
@@ -6281,7 +6281,7 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
         align_match = false;
     } else {
         def->state = virDomainObjGetState(vm, NULL);
-        def->memory = def->state == VIR_DOMAIN_SHUTOFF ?
+        def->memory = def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ?
                       VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
                       VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
     }
@@ -6572,8 +6572,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
         goto cleanup;
 
     if (!vm->persistent &&
-        snap->def->state != VIR_DOMAIN_RUNNING &&
-        snap->def->state != VIR_DOMAIN_PAUSED &&
+        snap->def->state != VIR_DOMAIN_SNAPSHOT_RUNNING &&
+        snap->def->state != VIR_DOMAIN_SNAPSHOT_PAUSED &&
         (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                   VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -6590,8 +6590,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
             goto cleanup;
         }
         if (virDomainObjIsActive(vm) &&
-            !(snap->def->state == VIR_DOMAIN_RUNNING
-              || snap->def->state == VIR_DOMAIN_PAUSED) &&
+            !(snap->def->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+              snap->def->state == VIR_DOMAIN_SNAPSHOT_PAUSED) &&
             (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
             virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
@@ -6612,8 +6612,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
     if (!config)
         goto cleanup;
 
-    if (snap->def->state == VIR_DOMAIN_RUNNING ||
-        snap->def->state == VIR_DOMAIN_PAUSED) {
+    if (snap->def->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+        snap->def->state == VIR_DOMAIN_SNAPSHOT_PAUSED) {
         /* Transitions 2, 3, 5, 6, 8, 9 */
         bool was_running = false;
         bool was_stopped = false;
@@ -6672,7 +6672,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
 
         /* Touch up domain state.  */
         if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) &&
-            (snap->def->state == VIR_DOMAIN_PAUSED ||
+            (snap->def->state == VIR_DOMAIN_SNAPSHOT_PAUSED ||
              (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
             /* Transitions 3, 6, 9 */
             virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 0a27deeaf874a7908e8355fa42d2d7f46994358b..f8ae23bafb11f007bb7e7a0b6c8a1eb31c5d0efe 100644 (file)
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -6316,9 +6316,9 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
         goto cleanup;
     }
     if (online)
-        def->state = VIR_DOMAIN_RUNNING;
+        def->state = VIR_DOMAIN_SNAPSHOT_RUNNING;
     else
-        def->state = VIR_DOMAIN_SHUTOFF;
+        def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
 
     virUUIDFormat(dom->uuid, uuidstr);
     memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);