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 mechanical.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/conf/snapshot_conf.h | 21 ++++++++++++++++++---
 src/conf/snapshot_conf.c | 28 ++++++++++++++--------------
 src/qemu/qemu_driver.c   | 34 ++++++++++++++++++++++------------
 src/test/test_driver.c   | 20 ++++++++++----------
 src/vbox/vbox_common.c   |  4 ++--
 5 files changed, 66 insertions(+), 41 deletions(-)

diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 7a175dfc96..9084f5fb8b 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -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_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
+    VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
+    VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
+    VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
+    VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
+    VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
+    VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
+    VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
+    /* Additional enum values local to qemu */
+    VIR_SNAP_STATE_DISK_SNAPSHOT,
+    VIR_SNAP_STATE_LAST
 } virDomainSnapshotState;
+verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);

 /* Stores disk-snapshot information */
 typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 41236d9932..299fc2101b 100644
--- 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_SNAP_STATE_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_SNAP_STATE_SHUTOFF ||
+                   def->state == VIR_SNAP_STATE_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_SNAP_STATE_SHUTOFF)
             return 0;
         if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) &&
-            obj->def->state == VIR_DOMAIN_DISK_SNAPSHOT)
+            obj->def->state == VIR_SNAP_STATE_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_SNAP_STATE_SHUTOFF &&
+            obj->def->state != VIR_SNAP_STATE_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_SNAP_STATE_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_SNAP_STATE_RUNNING ||
+             other->def->state == VIR_SNAP_STATE_PAUSED) !=
+            (def->state == VIR_SNAP_STATE_RUNNING ||
+             def->state == VIR_SNAP_STATE_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_SNAP_STATE_DISK_SNAPSHOT) !=
+            (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("cannot change between disk only and "
                              "full system in snapshot %s"),
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7e2b5b9106..2abe3417c9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15022,7 +15022,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_SNAP_STATE_DISK_SNAPSHOT && active) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("active qemu domains require external disk "
                                  "snapshots; disk %s requested internal"),
@@ -15099,7 +15099,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
     }

     /* disk snapshot requires at least one disk */
-    if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && !external) {
+    if (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT && !external) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("disk-only snapshots require at least "
                          "one disk to be selected for snapshot"));
@@ -15781,8 +15781,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_SNAP_STATE_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF :
             def->state;
+    /* FIXME: state should be virDomainSnapshotState, with the switch
+     * statement handling of VIR_SNAP_STATE_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:
@@ -15838,9 +15843,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_SNAP_STATE_DISK_SNAPSHOT;
             else
-                def->state = VIR_DOMAIN_SHUTOFF;
+                def->state = VIR_SNAP_STATE_SHUTOFF;
             def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
         } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
             def->state = virDomainObjGetState(vm, NULL);
@@ -15857,7 +15862,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
                 goto endjob;
             }

-            def->memory = (def->state == VIR_DOMAIN_SHUTOFF ?
+            def->memory = (def->state == VIR_SNAP_STATE_SHUTOFF ?
                            VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
                            VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
         }
@@ -16420,8 +16425,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_SNAP_STATE_RUNNING &&
+        snap->def->state != VIR_SNAP_STATE_PAUSED &&
         (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                   VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -16444,8 +16449,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_SNAP_STATE_RUNNING ||
+              snap->def->state == VIR_SNAP_STATE_PAUSED) &&
             (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
             virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
@@ -16481,6 +16486,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,

     cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;

+    /* FIXME: This cast should be to virDomainSnapshotState, with
+     * better handling of VIR_SNAP_STATE_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:
@@ -16622,7 +16632,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_SNAP_STATE_PAUSED ||
              (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
             /* Transitions 3, 6, 9 */
             virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
@@ -16729,7 +16739,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 ce0df1f8e3..4a6555e0ea 100644
--- 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_SNAP_STATE_DISK_SNAPSHOT;
         else
-            def->state = VIR_DOMAIN_SHUTOFF;
+            def->state = VIR_SNAP_STATE_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_SNAP_STATE_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_SNAP_STATE_RUNNING &&
+        snap->def->state != VIR_SNAP_STATE_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_SNAP_STATE_RUNNING ||
+              snap->def->state == VIR_SNAP_STATE_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_SNAP_STATE_RUNNING ||
+        snap->def->state == VIR_SNAP_STATE_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_SNAP_STATE_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 0a27deeaf8..9c2bd556bd 100644
--- 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_SNAP_STATE_RUNNING;
     else
-        def->state = VIR_DOMAIN_SHUTOFF;
+        def->state = VIR_SNAP_STATE_SHUTOFF;

     virUUIDFormat(dom->uuid, uuidstr);
     memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
-- 
2.20.1

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

On 3/4/19 10:34 PM, Eric Blake wrote:
> 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 mechanical.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/conf/snapshot_conf.h | 21 ++++++++++++++++++---
>  src/conf/snapshot_conf.c | 28 ++++++++++++++--------------
>  src/qemu/qemu_driver.c   | 34 ++++++++++++++++++++++------------
>  src/test/test_driver.c   | 20 ++++++++++----------
>  src/vbox/vbox_common.c   |  4 ++--
>  5 files changed, 66 insertions(+), 41 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

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

On Mon, Mar 04, 2019 at 09:34:29PM -0600, Eric Blake wrote:
>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 mechanical.
>
>Signed-off-by: Eric Blake <eblake@redhat.com>
>---
> src/conf/snapshot_conf.h | 21 ++++++++++++++++++---
> src/conf/snapshot_conf.c | 28 ++++++++++++++--------------
> src/qemu/qemu_driver.c   | 34 ++++++++++++++++++++++------------
> src/test/test_driver.c   | 20 ++++++++++----------
> src/vbox/vbox_common.c   |  4 ++--
> 5 files changed, 66 insertions(+), 41 deletions(-)
>
>diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
>index 7a175dfc96..9084f5fb8b 100644
>--- a/src/conf/snapshot_conf.h
>+++ b/src/conf/snapshot_conf.h
>@@ -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_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
>+    VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
>+    VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
>+    VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
>+    VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
>+    VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
>+    VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
>+    VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
>+    /* Additional enum values local to qemu */

As Eric Blake pointed out in an earlier iteration:
s/qemu/snapshots/

>+    VIR_SNAP_STATE_DISK_SNAPSHOT,
>+    VIR_SNAP_STATE_LAST
> } virDomainSnapshotState;
>+verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
>
> /* Stores disk-snapshot information */
> typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
>diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>index 41236d9932..299fc2101b 100644
>--- 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_SNAP_STATE_LAST,

VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix

>               "nostate",
>               "running",
>               "blocked",

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

On 3/7/19 9:20 AM, Ján Tomko wrote:
> On Mon, Mar 04, 2019 at 09:34:29PM -0600, Eric Blake wrote:
>> 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 mechanical.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> src/conf/snapshot_conf.h | 21 ++++++++++++++++++---
>> src/conf/snapshot_conf.c | 28 ++++++++++++++--------------
>> src/qemu/qemu_driver.c   | 34 ++++++++++++++++++++++------------
>> src/test/test_driver.c   | 20 ++++++++++----------
>> src/vbox/vbox_common.c   |  4 ++--
>> 5 files changed, 66 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
>> index 7a175dfc96..9084f5fb8b 100644
>> --- a/src/conf/snapshot_conf.h
>> +++ b/src/conf/snapshot_conf.h
>> @@ -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_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
>> +    VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
>> +    VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
>> +    VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
>> +    VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
>> +    VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
>> +    VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
>> +    VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
>> +    /* Additional enum values local to qemu */
> 
> As Eric Blake pointed out in an earlier iteration:
> s/qemu/snapshots/
> 
>> +    VIR_SNAP_STATE_DISK_SNAPSHOT,
>> +    VIR_SNAP_STATE_LAST
>> } virDomainSnapshotState;
>> +verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
>>
>> /* Stores disk-snapshot information */
>> typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 41236d9932..299fc2101b 100644
>> --- 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_SNAP_STATE_LAST,
> 
> VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix
> 

<sigh>, true, but we could go with virSnapState to ensure we match the
typedef nomenclature with the enum symbol names or change the _SNAP_ to
_SNAPSHOT_ w/ virSnapshotState for the typedef - whatever is easiest.

John

>>               "nostate",
>>               "running",
>>               "blocked",
> 
> Jano
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

On 3/7/19 9:26 AM, John Ferlan wrote:

>>> +    VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
>>> +    /* Additional enum values local to qemu */
>>
>> As Eric Blake pointed out in an earlier iteration:
>> s/qemu/snapshots/

D'oh - it's a bad sign when I miss incorporating my own review comments.


>>>
>>> /* virDomainSnapshotState is really virDomainState plus one extra
>>> state */
>>> -VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST,
>>> +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_SNAP_STATE_LAST,
>>
>> VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix
>>

Indeed, but it is also longer, and runs into line-wrap issues (I'm not
opposed to it, though).

> 
> <sigh>, true, but we could go with virSnapState to ensure we match the
> typedef nomenclature with the enum symbol names or change the _SNAP_ to
> _SNAPSHOT_ w/ virSnapshotState for the typedef - whatever is easiest.

I concur with having consistency one way or another. Unless I hear a
strong preference, I'll go ahead and spell the values out in full (and
wrap lines) rather than trying to abbreviate the values but not the enum
name.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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