[libvirt] [PATCH 0/2] fix snapshot --redefine bugs (incremental backup saga)
I ran into these while trying to debug my additions of bulk dumpxml/redefine for use with both snapshots and checkpoints. It is a long-standing bug (a fact not made any nicer by our delay in implementing revert to external snapshots, oh well), but one that has escaped notice until now. Eric Blake (2): snapshot: Permit redefine of offline external snapshot qemu: Fix snapshot redefine vs. domain state bug src/conf/snapshot_conf.c | 11 ++-- src/qemu/qemu_driver.c | 117 ++++++++++++++++++++++++--------------- 2 files changed, 76 insertions(+), 52 deletions(-) -- 2.20.1
Due to historical back-compat, 'virsh snapshot-create-as' favors internal snapshots (but can't be used on domains with raw storage), while 'virsh snapshot-create-as --disk-only) favors external snapshots. What's more, snapshots created with --disk-only while the domain was running are marked as snapshot state 'disk-snapshot', while snapshots created while the domain was offline are marked as snapshot state 'shutdown' (a 'disk-snapshot' image might not be quiescent, while a 'shutdown' snapshot always is). But this leads to some interesting problems: if we create a --disk-only snapshot of an offline guest, and then immediately try to 'virsh snapshot-create --redefine' using the resulting XML to overwrite the existing snapashot in place, things silently succeed, but 'virsh snapshot-create --redefine --disk-only' fails because the snapshot state is not 'disk-only'. Worse, if we delete the snapshot metadata first and then try to recreate things, omitting --disk-only fails because the verification code wants to force the default of an internal snapshot (which doesn't work with raw disks), and using --disk-only fails because the verification code doesn't see the 'disk-only' state in the snapshot xml - making it impossible to recreate the snapshot metadata. Ideally, the presence or absence of the --disk-only flag, and the presence or absence of an existing snapshot being overwritten, shouldn't matter; if the XML is valid for one situation, it should always be valid to redefine the metadata for that snapshot. Fix things by uniformly declaring that a redefined snapshot in the 'shutdown' state can permit a disk-only external snapshot (we got it right in only one out of three places in the function). See also https://bugzilla.redhat.com/1680304; this fixes the domain-agnostic problems mentioned there, but another patch is needed to fix further oddities with the qemu driver. I did not check for sure, but this has probably been broken even prior to when commit 670e86bf (1.1.4) factored redefine prep out of qemu code into the common snapshot_conf code. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 489c25f511..3372c4df86 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1225,6 +1225,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; virDomainSnapshotObjPtr other; + bool offline = def->state == VIR_DOMAIN_DISK_SNAPSHOT || + virDomainSnapshotDefIsExternal(def); /* Prevent circular chains */ if (def->parent) { @@ -1259,14 +1261,12 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } /* Check that any replacement is compatible */ - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && - def->state != VIR_DOMAIN_DISK_SNAPSHOT) { + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !offline) { virReportError(VIR_ERR_INVALID_ARG, _("disk-only flag for snapshot %s requires " "disk-snapshot state"), def->name); goto cleanup; - } if (def->dom && @@ -1315,8 +1315,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || - virDomainSnapshotDefIsExternal(def)) { + if (offline) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; } @@ -1346,7 +1345,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, *snap = other; } else { if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || + if (offline || def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; -- 2.20.1
On 2/23/19 3:00 PM, Eric Blake wrote:
Due to historical back-compat, 'virsh snapshot-create-as' favors internal snapshots (but can't be used on domains with raw storage), while 'virsh snapshot-create-as --disk-only) favors external
s/)/'
snapshots. What's more, snapshots created with --disk-only while the domain was running are marked as snapshot state 'disk-snapshot', while snapshots created while the domain was offline are marked as snapshot state 'shutdown' (a 'disk-snapshot' image might not be quiescent, while a 'shutdown' snapshot always is).
But this leads to some interesting problems: if we create a --disk-only snapshot of an offline guest, and then immediately try to 'virsh snapshot-create --redefine' using the resulting XML to overwrite the existing snapashot in place, things silently succeed, but 'virsh snapshot-create --redefine --disk-only' fails because the snapshot state is not 'disk-only'. Worse, if we delete the snapshot metadata first and then try to recreate things, omitting --disk-only fails because the verification code wants to force the default of an internal snapshot (which doesn't work with raw disks), and using --disk-only fails because the verification code doesn't see the 'disk-only' state in the snapshot xml - making it impossible to recreate the snapshot metadata. Ideally, the presence or absence of the --disk-only flag, and the presence or absence of an existing snapshot being overwritten, shouldn't matter; if the XML is valid for one situation, it should always be valid to redefine the metadata for that snapshot.
Fix things by uniformly declaring that a redefined snapshot in the 'shutdown' state can permit a disk-only external snapshot (we got it right in only one out of three places in the function).
See also https://bugzilla.redhat.com/1680304; this fixes the domain-agnostic problems mentioned there, but another patch is needed to fix further oddities with the qemu driver. I did not check for sure, but this has probably been broken even prior to when commit 670e86bf (1.1.4) factored redefine prep out of qemu code into the common snapshot_conf code.
Perhaps 709b0f37c (1.0.2)? or older e260e401a5 (1.0.0) The 1.0.2 change seems to be the source of hunk 2 and 3 below, while 1.0.0 is the source of hunk 1.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
Your explanation seems reasonable and although I don't think I could begin to (re) explain all the various states, conditions, [in]consistencies, etc... Still, consider it Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 factoring out the snapshot validation into a helper routine (which will also be useful in a later patch adding bulk redefine), and by adjusting the state being checked to the one supplied by the snapshot XML for redefinition, vs. the current domain state for original creation. However, since snapshots have one additional state beyond virDomainState (namely, the "disk-snapshot" state), and given the way virDomainSnapshotState was defined as an extension enum on top of virDomainState, we lose out on gcc's ability to force type-based validation that all switch branches are covered. In the process, observe that the qemu code already rejects _LIVE with _REDEFINE, but that this rejection occurs after parsing, and with a rather confusing message. Hoist that particular exclusive flag check earlier, so it gets a better error message, and is not inadvertently skipped when bulk redefine is added later (as that addition will occur prior to parsing). Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304 Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 117 +++++++++++++++++++++++++---------------- 1 file changed, 71 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe1b7801e9..1f37107a20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15669,6 +15669,70 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, } +/* Validate that a snapshot object does not violate any qemu-specific + * 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) +{ + /* 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 (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_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_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 */ + default: + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state)); + return -1; + } + + return 0; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15703,6 +15767,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)) @@ -15740,62 +15807,20 @@ 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, redefine ? def->state : 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 && (!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 " "with external checkpoints")); 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
On 2/23/19 3:00 PM, Eric Blake wrote:
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 factoring out the snapshot validation into a helper routine (which will also be useful in a later patch adding bulk redefine), and by adjusting the state being checked to the one supplied by the snapshot XML for redefinition, vs. the current domain state for original creation. However, since snapshots have one additional state beyond virDomainState (namely, the "disk-snapshot" state), and given the way virDomainSnapshotState was defined as an extension enum on top of virDomainState, we lose out on gcc's ability to force type-based validation that all switch branches are covered.
Would it be worth adding some sort of compiler warning that VIR_DOMAIN_SNAPSHOT_STATE_LAST must be VIR_DOMAIN_DISK_SNAPSHOT + 1. That would cause future additions to virDomainSnapshotState to rework the math - verify seems to be used for some places. Perhaps something worthwhile for any other one of these overlaid structs.
In the process, observe that the qemu code already rejects _LIVE with _REDEFINE, but that this rejection occurs after parsing, and with a rather confusing message. Hoist that particular exclusive flag check earlier, so it gets a better error message, and is not inadvertently skipped when bulk redefine is added later (as that addition will occur prior to parsing).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 117 +++++++++++++++++++++++++---------------- 1 file changed, 71 insertions(+), 46 deletions(-)
It seems there are multiple things going on, some code refactoring and then a couple bug fixes. IDC if they're combined - separating for easier backporting is always nice...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe1b7801e9..1f37107a20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15669,6 +15669,70 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, }
+/* Validate that a snapshot object does not violate any qemu-specific + * 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) +{ + /* 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 (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_DISK_SNAPSHOT: + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state));
I assume this is part of the fix - is there perhaps a way or a desire to distinguish between this and the invalid states below? That is a slightly different message to more easily know what went wrong without having to dig into the code.
+ return -1; + } + 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 */ + default: + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state)); + return -1; + } + + return 0; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15703,6 +15767,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);
And perhaps this is a 3rd bug fix in the same patch .
if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) @@ -15740,62 +15807,20 @@ 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, redefine ? def->state : vm->state.state,
Is passing of a different state value the 2nd bug?
+ flags) < 0) + goto cleanup;
/* 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)) {
Was this part of what I called the 3rd bug? Probably would be good to separate if you think that's wise/feasible. Since this is a bug fix it is a change that could be accepted post freeze. I'll let you decide how to proceed - Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live snapshot creation is supported only " "with external checkpoints")); 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
On 2/25/19 6:01 PM, John Ferlan wrote:
On 2/23/19 3:00 PM, Eric Blake wrote:
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 factoring out the snapshot validation into a helper routine (which will also be useful in a later patch adding bulk redefine), and by adjusting the state being checked to the one supplied by the snapshot XML for redefinition, vs. the current domain state for original creation. However, since snapshots have one additional state beyond virDomainState (namely, the "disk-snapshot" state), and given the way virDomainSnapshotState was defined as an extension enum on top of virDomainState, we lose out on gcc's ability to force type-based validation that all switch branches are covered.
Would it be worth adding some sort of compiler warning that VIR_DOMAIN_SNAPSHOT_STATE_LAST must be VIR_DOMAIN_DISK_SNAPSHOT + 1. That would cause future additions to virDomainSnapshotState to rework the math - verify seems to be used for some places. Perhaps something worthwhile for any other one of these overlaid structs.
Well, we already have: VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_STATE_LAST Another possibility might be fully flushing out the enum (like we've done in qemu_blockjob.h with qemuBlockjobState and qemuBlockJobType.
In the process, observe that the qemu code already rejects _LIVE with _REDEFINE, but that this rejection occurs after parsing, and with a rather confusing message. Hoist that particular exclusive flag check earlier, so it gets a better error message, and is not inadvertently skipped when bulk redefine is added later (as that addition will occur prior to parsing).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 117 +++++++++++++++++++++++++---------------- 1 file changed, 71 insertions(+), 46 deletions(-)
It seems there are multiple things going on, some code refactoring and then a couple bug fixes. IDC if they're combined - separating for easier backporting is always nice...
Now that we've hit rc1, I'll split this patch into smaller pieces, so we can more easily decide which portions are important for 5.1 and ensure ease of backports as needed.
Probably would be good to separate if you think that's wise/feasible. Since this is a bug fix it is a change that could be accepted post freeze. I'll let you decide how to proceed -
Reviewed-by: John Ferlan <jferlan@redhat.com>
Look for a v2 to come shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
John Ferlan