[libvirt] [PATCH v3 02/18] 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 mechanical. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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