From f43eb6807ed3ceaaf6a068c2f58d82668345e454 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 26 Feb 2019 14:14:36 -0600 Subject: [PATCH] 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 Reviewed-by: John Ferlan --- src/conf/snapshot_conf.c | 28 ++++++++++++++-------------- src/conf/snapshot_conf.h | 23 +++++++++++++++++++---- src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------ src/test/test_driver.c | 20 ++++++++++---------- src/vbox/vbox_common.c | 4 ++-- 5 files changed, 67 insertions(+), 42 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 41236d9932..054721012c 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_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 /, 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 7a175dfc96..5f2cda8848 100644 --- 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 95068d91b6..96415a38ca 100644 --- 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 ce0df1f8e3..490a423b96 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_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 0a27deeaf8..f8ae23bafb 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_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); -- GitLab