[libvirt] [PATCH v2 0/7] fix snapshot --redefine bugs (incremental backup saga)
John pointed out that patch 2 of my original bug fix did too much in one patch. Now that we are in freeze for 5.1, I've proposed two variants of the same fix: patch 1 is the bare minimum to fix the bug and nothing else, while patches 3-7 are more in line with my v1 patch at doing other refactoring work along the way (but now split into multiple logical steps) that will prove useful for my other pending snapshot improvements (https://www.redhat.com/archives/libvir-list/2019-February/msg01350.html, but now 5.2 material). I'm proposing both variants for comparison, although I already suspect the answer will be 'use patch 1 for 5.1, then rebase what remains of patches 3-7 into the other snapshot cleanups for 5.2'. variant2 is cleaner than my v1 patch 2/2 in that I no longer have to use a default: label to hack around gcc's enum sanity checking within a switch statement, but it required introducing a large mechanical rename of all use of snapshot state values. Eric Blake (7): qemu: Fix snapshot redefine vs. domain state bug Revert "qemu: Fix snapshot redefine vs. domain state bug" qemu: Refactor check for _LIVE vs. _REDEFINE qemu: Factor out qemuDomainSnapshotValidate() helper snapshot: Rework virDomainSnapshotState enum qemu: Use virDomainSnapshotState for switch statements qemu: Fix snapshot redefine vs. domain state bug src/conf/snapshot_conf.h | 21 ++++- src/conf/snapshot_conf.c | 28 +++---- src/qemu/qemu_driver.c | 160 +++++++++++++++++++++++---------------- src/test/test_driver.c | 20 ++--- src/vbox/vbox_common.c | 4 +- 5 files changed, 137 insertions(+), 96 deletions(-) -- 2.20.1
The existing qemu snapshot code has a slight bug: if the domain is currently pmsuspended, you can't use the _REDEFINE flag even though the current domain state should have no bearing on being able to recreate metadata state; and conversely, you can use the _REDEFINE flag to create snapshot metadata claiming to be pmsuspended as a bypass to the normal restrictions that you can't create an original qemu snapshot in that state (the restriction against pmsuspend is specific to qemu, rather than part of the driver-agnostic snapshot_conf code). Fix this by checking the snapshot state (when redefining) instead of the domain state (which is a subset of snapshot states). Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304 Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1d5b5f8653..36426cd65a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15693,6 +15693,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; + virDomainState state; virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | @@ -15776,7 +15777,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* allow snapshots only in certain states */ - switch ((virDomainState) vm->state.state) { + state = vm->state.state; + if (redefine) + state = def->state == VIR_DOMAIN_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF : + def->state; + switch (state) { /* valid states */ case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_PAUSED: @@ -15796,7 +15801,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ case VIR_DOMAIN_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), - virDomainStateTypeToString(vm->state.state)); + virDomainStateTypeToString(state)); goto cleanup; } -- 2.20.1
This reverts commit a6ecf44211ec62786dc1dc6ff30f1c7ea1a18f8b. --- src/qemu/qemu_driver.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 36426cd65a..1d5b5f8653 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15693,7 +15693,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; - virDomainState state; virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | @@ -15777,11 +15776,7 @@ 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 : - def->state; - switch (state) { + switch ((virDomainState) vm->state.state) { /* valid states */ case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_PAUSED: @@ -15801,7 +15796,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ case VIR_DOMAIN_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), - virDomainStateTypeToString(state)); + virDomainStateTypeToString(vm->state.state)); goto cleanup; } -- 2.20.1
The current qemu code rejects the combination of the two flags VIR_DOMAIN_SNAPSHOT_CREATE_LIVE in tandem with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, but rather late in the cycle (after the snapshot was already parsed), and with a rather confusing message (complaining that live snapshots require external storage, even if the redefined snapshot already declares external storage). Hoist the rejection message to occur earlier (before parsing any XML, which also aids upcoming patches that will implement bulk redefine), and with a more typical error message about mutually exclusive flags. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1d5b5f8653..4869096273 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15707,6 +15707,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, NULL); + VIR_EXCLUSIVE_FLAGS_RET(VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + NULL); if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) @@ -15767,8 +15770,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && (!virDomainObjIsActive(vm) || - def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - redefine)) { + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live snapshot creation is supported only " "during full system snapshots")); -- 2.20.1
Straight code motion, coupled with changing goto into return -1 as needed. This change will be important to later patches adding bulk redefinition (where each snapshot in a list has to meet the same constraints), but moving it out now also makes it easier for a more immediate bug-fix related to which states are being validated in the next patch. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 99 +++++++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4869096273..a8ac9b59ee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15673,6 +15673,59 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, } +/* Validate that a snapshot object does not violate any qemu-specific + * constraints. */ +static int +qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state, + unsigned int flags) +{ + /* reject snapshot names containing slashes or starting with dot as + * snapshot definitions are saved in files named by the snapshot name */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid snapshot name '%s': " + "name can't contain '/'"), + def->name); + return -1; + } + + if (def->name[0] == '.') { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid snapshot name '%s': " + "name can't start with '.'"), + def->name); + return -1; + } + } + + /* allow snapshots only in certain states */ + switch ((virDomainState) state) { + /* valid states */ + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_PAUSED: + case VIR_DOMAIN_SHUTDOWN: + case VIR_DOMAIN_SHUTOFF: + case VIR_DOMAIN_CRASHED: + break; + + case VIR_DOMAIN_PMSUSPENDED: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("qemu doesn't support taking snapshots of " + "PMSUSPENDED guests")); + return -1; + + /* invalid states */ + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ + case VIR_DOMAIN_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainStateTypeToString(state)); + return -1; + } + return 0; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15747,25 +15800,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags))) goto cleanup; - /* reject snapshot names containing slashes or starting with dot as - * snapshot definitions are saved in files named by the snapshot name */ - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { - if (strchr(def->name, '/')) { - virReportError(VIR_ERR_XML_DETAIL, - _("invalid snapshot name '%s': " - "name can't contain '/'"), - def->name); - goto cleanup; - } - - if (def->name[0] == '.') { - virReportError(VIR_ERR_XML_DETAIL, - _("invalid snapshot name '%s': " - "name can't start with '.'"), - def->name); - goto cleanup; - } - } + if (qemuDomainSnapshotValidate(def, vm->state.state, flags) < 0) + goto cleanup; /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && @@ -15777,31 +15813,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } - /* allow snapshots only in certain states */ - switch ((virDomainState) vm->state.state) { - /* valid states */ - case VIR_DOMAIN_RUNNING: - case VIR_DOMAIN_PAUSED: - case VIR_DOMAIN_SHUTDOWN: - case VIR_DOMAIN_SHUTOFF: - case VIR_DOMAIN_CRASHED: - break; - - case VIR_DOMAIN_PMSUSPENDED: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("qemu doesn't support taking snapshots of " - "PMSUSPENDED guests")); - goto cleanup; - - /* invalid states */ - case VIR_DOMAIN_NOSTATE: - case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ - case VIR_DOMAIN_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), - virDomainStateTypeToString(vm->state.state)); - goto cleanup; - } - /* We are going to modify the domain below. Internal snapshots would use * a regular job, so we need to set the job mask to disallow query as * 'savevm' blocks the monitor. External snapshot will then modify the -- 2.20.1
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 all clients to use the new enum names anywhere snapshot state is referenced. Qemu code gains a fixme comment about some odd casts (which will be cleaned up in separate patches); 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 | 27 ++++++++++++++++----------- src/test/test_driver.c | 20 ++++++++++---------- src/vbox/vbox_common.c | 4 ++-- 5 files changed, 60 insertions(+), 40 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 a8ac9b59ee..34014ba9c7 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")); @@ -15844,9 +15844,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); @@ -15863,7 +15863,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); } @@ -16426,8 +16426,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", @@ -16450,8 +16450,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", @@ -16487,6 +16487,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: @@ -16628,7 +16633,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, @@ -16735,7 +16740,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 d95fe7c7ae..407c932019 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -6314,9 +6314,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
On 2/27/19 2:04 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 all clients to use the new enum names anywhere snapshot state is referenced. Qemu code gains a fixme comment about some odd casts (which will be cleaned up in separate patches); the rest of the patch is mechanical.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
+/** + * 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 */
Too much copy-and-paste from qemu_blockjob.h; this comment should mention 'local to snapshots'
+ VIR_SNAP_STATE_DISK_SNAPSHOT, + VIR_SNAP_STATE_LAST } virDomainSnapshotState; +verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Clean up the previous patch which abused a switch on virDomainState when contrasted to a variable containing virDomainSnapshotState, by converting the two affected switch statements to now use the right enum. No semantic changes now (the caller to qemuDomainSnapshotValidate passes in a domain state rather than a snapshot state, so the new branch is currently unreachable [will change in next patch], while qemuDomainRevertToSnapshot can't reach the new state because of the earlier rejection of external images). Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34014ba9c7..06bc1893ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15700,27 +15700,35 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state, } /* allow snapshots only in certain states */ - switch ((virDomainState) state) { + switch ((virDomainSnapshotState) state) { /* valid states */ - case VIR_DOMAIN_RUNNING: - case VIR_DOMAIN_PAUSED: - case VIR_DOMAIN_SHUTDOWN: - case VIR_DOMAIN_SHUTOFF: - case VIR_DOMAIN_CRASHED: + case VIR_SNAP_STATE_RUNNING: + case VIR_SNAP_STATE_PAUSED: + case VIR_SNAP_STATE_SHUTDOWN: + case VIR_SNAP_STATE_SHUTOFF: + case VIR_SNAP_STATE_CRASHED: break; - case VIR_DOMAIN_PMSUSPENDED: + case VIR_SNAP_STATE_DISK_SNAPSHOT: + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state)); + return -1; + } + break; + + case VIR_SNAP_STATE_PMSUSPENDED: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("qemu doesn't support taking snapshots of " "PMSUSPENDED guests")); return -1; /* invalid states */ - case VIR_DOMAIN_NOSTATE: - case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ - case VIR_DOMAIN_LAST: + case VIR_SNAP_STATE_NOSTATE: + case VIR_SNAP_STATE_BLOCKED: /* invalid state, unused in qemu */ + case VIR_SNAP_STATE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), - virDomainStateTypeToString(state)); + virDomainSnapshotStateTypeToString(state)); return -1; } return 0; @@ -16487,14 +16495,9 @@ 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: + switch ((virDomainSnapshotState) snap->def->state) { + case VIR_SNAP_STATE_RUNNING: + case VIR_SNAP_STATE_PAUSED: start_flags |= VIR_QEMU_PROCESS_START_PAUSED; @@ -16669,9 +16672,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } break; - case VIR_DOMAIN_SHUTDOWN: - case VIR_DOMAIN_SHUTOFF: - case VIR_DOMAIN_CRASHED: + case VIR_SNAP_STATE_SHUTDOWN: + case VIR_SNAP_STATE_SHUTOFF: + case VIR_SNAP_STATE_CRASHED: /* Transitions 1, 4, 7 */ /* Newer qemu -loadvm refuses to revert to the state of a snapshot * created by qemu-img snapshot -c. If the domain is running, we @@ -16728,15 +16731,17 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } break; - case VIR_DOMAIN_PMSUSPENDED: + case VIR_SNAP_STATE_PMSUSPENDED: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("qemu doesn't support reversion of snapshot taken in " "PMSUSPENDED state")); goto endjob; - case VIR_DOMAIN_NOSTATE: - case VIR_DOMAIN_BLOCKED: - case VIR_DOMAIN_LAST: + case VIR_SNAP_STATE_DISK_SNAPSHOT: + /* Rejected earlier as an external snapshot */ + case VIR_SNAP_STATE_NOSTATE: + case VIR_SNAP_STATE_BLOCKED: + case VIR_SNAP_STATE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid target domain state '%s'. Refusing " "snapshot reversion"), -- 2.20.1
The existing qemu snapshot code has a slight bug: if the domain is currently pmsuspended, you can't use the _REDEFINE flag even though the current domain state should have no bearing on being able to recreate metadata state; and conversely, you can use the _REDEFINE flag to create snapshot metadata claiming to be pmsuspended as a bypass to the normal restrictions that you can't create an original qemu snapshot in that state (the restriction against pmsuspend is specific to qemu, rather than part of the driver-agnostic snapshot_conf code). Fix this by checking the snapshot state (when redefining) instead of the domain state (which is a subset of snapshot states). Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304 Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 06bc1893ad..18acdd9816 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15674,7 +15674,9 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, /* Validate that a snapshot object does not violate any qemu-specific - * constraints. */ + * constraints. @state is virDomainState if flags implies creation, or + * virDomainSnapshotState if flags includes _REDEFINE (the latter + * enum is a superset of the former). */ static int qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state, unsigned int flags) @@ -15808,7 +15810,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags))) goto cleanup; - if (qemuDomainSnapshotValidate(def, vm->state.state, flags) < 0) + if (qemuDomainSnapshotValidate(def, redefine ? def->state : vm->state.state, + flags) < 0) goto cleanup; /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ -- 2.20.1
Mention my snapshot bug fixes, and the corresponding virsh command-line parse tweak I added while working on the snapshot bug fixes. Signed-off-by: Eric Blake <eblake@redhat.com> --- This should be valid whether we go with variant1 or variant2 from the rest of the series. docs/news.xml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index b86943ab47..0ecafffd30 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -157,6 +157,17 @@ version. </description> </change> + <change> + <summary> + Batch mode virsh and virt-admin parsing improvements + </summary> + <description> + When parsing a single-argument command_string in batch mode, + virsh and virt-admin now permit newlines in addition to + semicolons for splitting commands, and backslash-newline for + splitting long lines, to be more like shell parsing. + </description> + </change> </section> <section title="Bug fixes"> <change> @@ -315,6 +326,16 @@ attributes for devices without an image. </description> </change> + <change> + <summary> + External snapshot metadata redefinition is fixed + </summary> + <description> + Attempting to use VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE to + reinstate the metadata describing an external snapshot + created earlier for an offline domain no longer fails. + </description> + </change> </section> </release> <release version="v5.0.0" date="2019-01-15"> -- 2.20.1
On 2/27/19 3:04 PM, Eric Blake wrote:
John pointed out that patch 2 of my original bug fix did too much in one patch. Now that we are in freeze for 5.1, I've proposed two variants of the same fix: patch 1 is the bare minimum to fix the bug and nothing else, while patches 3-7 are more in line with my v1 patch at doing other refactoring work along the way (but now split into multiple logical steps) that will prove useful for my other pending snapshot improvements (https://www.redhat.com/archives/libvir-list/2019-February/msg01350.html, but now 5.2 material).
I'm proposing both variants for comparison, although I already suspect the answer will be 'use patch 1 for 5.1, then rebase what remains of patches 3-7 into the other snapshot cleanups for 5.2'.
variant2 is cleaner than my v1 patch 2/2 in that I no longer have to use a default: label to hack around gcc's enum sanity checking within a switch statement, but it required introducing a large mechanical rename of all use of snapshot state values.
I was hoping someone else would take a bite before me, but I guess not... So my choices are let's do some refactoring, add/modify some enums, and make a cleaner fix, but have a longer series of changes or do a quick short term hack for 5.1.0 and then as soon as 5.2.0 opens, revert that, and do things properly. More less - a fair assessment? There's also the has anyone outside of you noticed up to this point. I vote for doing it right, but if there's strong opinions to hack and replace, then I won't complain either. You have my - Reviewed-by: John Ferlan <jferlan@redhat.com> John
Eric Blake (7): qemu: Fix snapshot redefine vs. domain state bug Revert "qemu: Fix snapshot redefine vs. domain state bug" qemu: Refactor check for _LIVE vs. _REDEFINE qemu: Factor out qemuDomainSnapshotValidate() helper snapshot: Rework virDomainSnapshotState enum qemu: Use virDomainSnapshotState for switch statements qemu: Fix snapshot redefine vs. domain state bug
src/conf/snapshot_conf.h | 21 ++++- src/conf/snapshot_conf.c | 28 +++---- src/qemu/qemu_driver.c | 160 +++++++++++++++++++++++---------------- src/test/test_driver.c | 20 ++--- src/vbox/vbox_common.c | 4 +- 5 files changed, 137 insertions(+), 96 deletions(-)
participants (2)
-
Eric Blake -
John Ferlan