[libvirt] [PATCH v3 00/18] bulk snapshot list/redefine (incremental backup saga)
While looking at my work on incremental backups, Nir raised a good point: if we want to recreate a set of known checkpoints on one machine that will be taking over a domain from another machine, my initial proposal required making multiple API calls to list the XML for each checkpoint on the source, and then more API calls to redefine each checkpoint on the destination; it also had the drawback that the list has to be presented in topological order (the API won't let you define a child checkpoint if the parent is not defined first). He asked if I could instead add bulk APIs, for getting the XML for all checkpoints at once, and then for redefining all checkpoints at once. Since my checkpoint code borrows heavily from concepts in the snapshot code, I chose to tackle the problem by starting with this series, which does the same thing for snapshots as what I plan to do for checkpoints. That is, since this patch series adds virDomainGetXMLDesc(, VIR_DOMAIN_XML_SNAPSHOTS) and virDomainSnapshotCreateXML(, VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST), the checkpoint series will add virDOmainGetXMLDesc(, VIR_DOMAIN_XML_CHECKPOINTS) and virDomainCheckpointCreateXML(, VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE_LIST) with very similar code. Also available at: https://repo.or.cz/libvirt/ericb.git/shortlog/refs/tags/snapshot-bulk-v3 Changes since v2: - address lots of comments from John - split and rearrange some patches - incorporate remaining patches from my series fixing --redefine bugs - rebase to master 001/18:[down] 'qemu: Refactor snapshot check for _LIVE vs. _REDEFINE' 002/18:[down] 'snapshot: Rework virDomainSnapshotState enum' 003/18:[down] 'qemu: Use virDomainSnapshotState for switch statements' 004/18:[0017] [FC] 'snapshot: Give virDomainSnapshotDefFormat its own flags' 005/18:[0003] [FC] 'snapshot: Refactor virDomainSnapshotDefFormat' 006/18:[0029] [FC] 'snapshot: Add virDomainSnapshotObjListFormat' 007/18:[down] 'domain: Add struct for future domain format parameters' 008/18:[down] 'snapshot: Avoid latent use-after-free when cleaning snapshots' 009/18:[0071] [FC] 'domain: Expand virDomainDefFormatInternal with snapshots' 010/18:[down] 'snapshot: Split out virDomainSnapshotRedefineValidate helper' 011/18:[down] 'snapshot: Add virDomainSnapshotObjListParse' 012/18:[0005] [FC] 'domain: Add VIR_DOMAIN_XML_SNAPSHOTS flag' 013/18:[0002] [FC] 'snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST flag' 014/18:[0018] [FC] 'virsh: Expose bulk snapshot dumpxml/redefine' 015/18:[0010] [FC] 'test: Implement bulk snapshot operations' 016/18:[down] 'qemu: Factor out qemuDomainSnapshotValidate() helper' 017/18:[down] 'qemu: Const-correct snapshot directory name' 018/18:[0033] [FC] 'qemu: Implement bulk snapshot operations' Eric Blake (18): qemu: Refactor snapshot check for _LIVE vs. _REDEFINE snapshot: Rework virDomainSnapshotState enum qemu: Use virDomainSnapshotState for switch statements snapshot: Give virDomainSnapshotDefFormat its own flags snapshot: Refactor virDomainSnapshotDefFormat snapshot: Add virDomainSnapshotObjListFormat domain: Add struct for future domain format parameters snapshot: Avoid latent use-after-free when cleaning snapshots domain: Expand virDomainDefFormatInternal with snapshots snapshot: Split out virDomainSnapshotRedefineValidate helper snapshot: Add virDomainSnapshotObjListParse domain: Add VIR_DOMAIN_XML_SNAPSHOTS flag snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST flag virsh: Expose bulk snapshot dumpxml/redefine test: Implement bulk snapshot operations qemu: Factor out qemuDomainSnapshotValidate() helper qemu: Const-correct snapshot directory name qemu: Implement bulk snapshot operations include/libvirt/libvirt-domain-snapshot.h | 3 + include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.h | 21 +- src/conf/snapshot_conf.h | 47 +- src/qemu/qemu_domain.h | 2 +- src/conf/domain_conf.c | 69 ++- src/conf/snapshot_conf.c | 513 ++++++++++++++++------ src/esx/esx_driver.c | 1 - src/libvirt-domain-snapshot.c | 23 +- src/libvirt-domain.c | 8 +- src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 2 +- src/qemu/qemu_domain.c | 47 +- src/qemu/qemu_driver.c | 234 ++++++---- src/test/test_driver.c | 52 ++- src/vbox/vbox_common.c | 13 +- src/vz/vz_driver.c | 3 +- tests/domainsnapshotxml2xmltest.c | 16 +- tools/virsh-domain.c | 7 + tools/virsh-snapshot.c | 14 + tools/virsh.pod | 18 +- 21 files changed, 790 insertions(+), 307 deletions(-) -- 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> Reviewed-by: John Ferlan <jferlan@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 36426cd65a..7e2b5b9106 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15708,6 +15708,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)) @@ -15768,8 +15771,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
On Mon, Mar 04, 2019 at 09:34:28PM -0600, Eric Blake wrote:
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> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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
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
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
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
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
Clean up the previous patch which abused switch on virDomainState while working with a variable containing virDomainSnapshotState, by converting the two affected switch statements to now use the right enum. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 67 ++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2abe3417c9..7e01b396fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15693,7 +15693,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; - virDomainState state; + virDomainSnapshotState state; virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | @@ -15779,36 +15779,36 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* allow snapshots only in certain states */ - state = vm->state.state; - if (redefine) - 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. */ + state = redefine ? def->state : vm->state.state; switch (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 (!redefine) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state)); + goto cleanup; + } + break; + + case VIR_SNAP_STATE_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: + 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)); goto cleanup; } @@ -16486,14 +16486,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; @@ -16668,9 +16663,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 @@ -16727,15 +16722,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
On 3/4/19 10:34 PM, Eric Blake wrote:
Clean up the previous patch which abused switch on virDomainState while working with a variable containing virDomainSnapshotState, by converting the two affected switch statements to now use the right enum.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 67 ++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 35 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
virDomainSnapshotDefFormat currently takes two sets of knobs: an 'unsigned int flags' argument that can currently just be VIR_DOMAIN_DEF_FORMAT_SECURE, and an 'int internal' argument used as a bool to determine whether to output an additional element. It then reuses the 'flags' knob to call into virDomainDefFormatInternal(), which takes a different set of flags. In fact, prior to commit 0ecd6851 (1.2.12), the 'flags' argument actually took the public VIR_DOMAIN_XML_SECURE, which was even more confusing. Let's borrow from the style of that earlier commit, by introducing a function for translating from the public flags (VIR_DOMAIN_SNAPSHOT_XML_SECURE was just recently introduced) into a new enum specific to snapshot formatting, and adjust all callers to use snapshot-specific enum values when formatting, and where the formatter now uses a new variable 'domainflags' to make it obvious when we are translating from snapshot flags back to domain flags. We don't even have to use the conversion function for drivers that don't accept the public VIR_DOMAIN_SNAPSHOT_XML_SECURE flag. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/conf/snapshot_conf.h | 10 ++++++++-- src/conf/snapshot_conf.c | 33 ++++++++++++++++++++++++------- src/esx/esx_driver.c | 1 - src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_driver.c | 3 +-- src/test/test_driver.c | 3 +-- src/vbox/vbox_common.c | 9 +++++---- src/vz/vz_driver.c | 3 +-- tests/domainsnapshotxml2xmltest.c | 16 ++++++++------- 10 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 9084f5fb8b..1b8bdde4f5 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -116,6 +116,13 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, } virDomainSnapshotParseFlags; +typedef enum { + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE = 1 << 0, + VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL = 1 << 1, +} virDomainSnapshotFormatFlags; + +unsigned int virDomainSnapshotFormatConvertXMLFlags(unsigned int flags); + virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, @@ -130,8 +137,7 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - unsigned int flags, - int internal); + unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, int default_snapshot, bool require_match); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 299fc2101b..0f8c9cfc8c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1,7 +1,7 @@ /* * snapshot_conf.c: 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 @@ -652,6 +652,22 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, return ret; } + +/* Converts public VIR_DOMAIN_SNAPSHOT_XML_* into + * VIR_DOMAIN_SNAPSHOT_FORMAT_* flags, and silently ignores any other + * flags. */ +unsigned int +virDomainSnapshotFormatConvertXMLFlags(unsigned int flags) +{ + unsigned int formatFlags = 0; + + if (flags & VIR_DOMAIN_SNAPSHOT_XML_SECURE) + formatFlags |= VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; + + return formatFlags; +} + + static int virDomainSnapshotDiskDefFormat(virBufferPtr buf, virDomainSnapshotDiskDefPtr disk, @@ -692,15 +708,17 @@ virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - unsigned int flags, - int internal) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; + int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; - virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | + VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL, NULL); - flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; + if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) + domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; virBufferAddLit(&buf, "<domainsnapshot>\n"); virBufferAdjustIndent(&buf, 2); @@ -742,7 +760,8 @@ virDomainSnapshotDefFormat(const char *uuidstr, } if (def->dom) { - if (virDomainDefFormatInternal(def->dom, caps, flags, &buf, xmlopt) < 0) + if (virDomainDefFormatInternal(def->dom, caps, domainflags, &buf, + xmlopt) < 0) goto error; } else if (uuidstr) { virBufferAddLit(&buf, "<domain>\n"); @@ -756,7 +775,7 @@ virDomainSnapshotDefFormat(const char *uuidstr, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; - if (internal) + if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL) virBufferAsprintf(&buf, "<active>%d</active>\n", def->current); virBufferAdjustIndent(&buf, -2); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 379c2bae73..8ddfa93847 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4204,7 +4204,6 @@ esxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(snapshot->domain->uuid, uuid_string); xml = virDomainSnapshotDefFormat(uuid_string, &def, priv->caps, priv->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), 0); cleanup: diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 04b9b67478..7ce7c85b55 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -890,6 +890,7 @@ virDomainSnapshotFindByName; virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotFormatConvertXMLFlags; virDomainSnapshotIsExternal; virDomainSnapshotLocationTypeFromString; virDomainSnapshotLocationTypeToString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1487268a89..f7353a9fac 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8410,8 +8410,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virUUIDFormat(vm->def->uuid, uuidstr); newxml = virDomainSnapshotDefFormat( uuidstr, snapshot->def, caps, xmlopt, - virDomainDefFormatConvertXMLFlags(QEMU_DOMAIN_FORMAT_LIVE_FLAGS), - 1); + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL); if (newxml == NULL) return -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7e01b396fb..76fab0421b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16278,8 +16278,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, xml = virDomainSnapshotDefFormat(uuidstr, snap->def, driver->caps, driver->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + virDomainSnapshotFormatConvertXMLFlags(flags)); cleanup: virDomainObjEndAPI(&vm); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4a6555e0ea..c5de17f366 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6209,8 +6209,7 @@ testDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, xml = virDomainSnapshotDefFormat(uuidstr, snap->def, privconn->caps, privconn->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + virDomainSnapshotFormatConvertXMLFlags(flags)); cleanup: virDomainObjEndAPI(&vm); diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9c2bd556bd..1d1c5f5113 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5375,7 +5375,10 @@ vboxSnapshotRedefine(virDomainPtr dom, VIR_FREE(currentSnapshotXmlFilePath); if (virAsprintf(¤tSnapshotXmlFilePath, "%s%s.xml", machineLocationPath, snapshotMachineDesc->currentSnapshot) < 0) goto cleanup; - char *snapshotContent = virDomainSnapshotDefFormat(NULL, def, data->caps, data->xmlopt, VIR_DOMAIN_DEF_FORMAT_SECURE, 0); + char *snapshotContent = virDomainSnapshotDefFormat(NULL, def, + data->caps, + data->xmlopt, + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE); if (snapshotContent == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to get snapshot content")); @@ -6322,9 +6325,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(dom->uuid, uuidstr); memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); - ret = virDomainSnapshotDefFormat(uuidstr, def, data->caps, data->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + ret = virDomainSnapshotDefFormat(uuidstr, def, data->caps, data->xmlopt, 0); cleanup: virDomainSnapshotDefFree(def); diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2d2eaf88a6..066d617524 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2291,8 +2291,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) xml = virDomainSnapshotDefFormat(uuidstr, snap->def, privconn->driver->caps, privconn->driver->xmlopt, - virDomainDefFormatConvertXMLFlags(flags), - 0); + virDomainSnapshotFormatConvertXMLFlags(flags)); cleanup: virDomainSnapshotObjListFree(snapshots); diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 2a07fe0789..9eb71780fc 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -78,13 +78,16 @@ testCompareXMLToXMLFiles(const char *inxml, char *actual = NULL; int ret = -1; virDomainSnapshotDefPtr def = NULL; - unsigned int flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parseflags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int formatflags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; - if (internal) - flags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; + if (internal) { + parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; + formatflags |= VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL; + } if (redefine) - flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; + parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; if (virTestLoadFile(inxml, &inXmlData) < 0) goto cleanup; @@ -94,13 +97,12 @@ testCompareXMLToXMLFiles(const char *inxml, if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, driver.xmlopt, - flags))) + parseflags))) goto cleanup; if (!(actual = virDomainSnapshotDefFormat(uuid, def, driver.caps, driver.xmlopt, - VIR_DOMAIN_DEF_FORMAT_SECURE, - internal))) + formatflags))) goto cleanup; if (!redefine) { -- 2.20.1
Split out an internal helper that produces format into a virBuffer, similar to what domain_conf.c does, and making the next patch easier to write. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/conf/snapshot_conf.c | 106 +++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 44 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 0f8c9cfc8c..bceebb2b1f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -703,92 +703,110 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, } -char * -virDomainSnapshotDefFormat(const char *uuidstr, - virDomainSnapshotDefPtr def, - virCapsPtr caps, - virDomainXMLOptionPtr xmlopt, - unsigned int flags) +/* Append XML describing def into buf. Return 0 on success, or -1 on + * failure with buf cleared. */ +static int +virDomainSnapshotDefFormatInternal(virBufferPtr buf, + const char *uuidstr, + virDomainSnapshotDefPtr def, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) { - virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | - VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL, NULL); - if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; - virBufferAddLit(&buf, "<domainsnapshot>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<domainsnapshot>\n"); + virBufferAdjustIndent(buf, 2); - virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); + virBufferEscapeString(buf, "<name>%s</name>\n", def->name); if (def->description) - virBufferEscapeString(&buf, "<description>%s</description>\n", + virBufferEscapeString(buf, "<description>%s</description>\n", def->description); - virBufferAsprintf(&buf, "<state>%s</state>\n", + virBufferAsprintf(buf, "<state>%s</state>\n", virDomainSnapshotStateTypeToString(def->state)); if (def->parent) { - virBufferAddLit(&buf, "<parent>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferEscapeString(&buf, "<name>%s</name>\n", def->parent); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</parent>\n"); + virBufferAddLit(buf, "<parent>\n"); + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "<name>%s</name>\n", def->parent); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</parent>\n"); } - virBufferAsprintf(&buf, "<creationTime>%lld</creationTime>\n", + virBufferAsprintf(buf, "<creationTime>%lld</creationTime>\n", def->creationTime); if (def->memory) { - virBufferAsprintf(&buf, "<memory snapshot='%s'", + virBufferAsprintf(buf, "<memory snapshot='%s'", virDomainSnapshotLocationTypeToString(def->memory)); - virBufferEscapeString(&buf, " file='%s'", def->file); - virBufferAddLit(&buf, "/>\n"); + virBufferEscapeString(buf, " file='%s'", def->file); + virBufferAddLit(buf, "/>\n"); } if (def->ndisks) { - virBufferAddLit(&buf, "<disks>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<disks>\n"); + virBufferAdjustIndent(buf, 2); for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefFormat(&buf, &def->disks[i], xmlopt) < 0) + if (virDomainSnapshotDiskDefFormat(buf, &def->disks[i], xmlopt) < 0) goto error; } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</disks>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</disks>\n"); } if (def->dom) { - if (virDomainDefFormatInternal(def->dom, caps, domainflags, &buf, + if (virDomainDefFormatInternal(def->dom, caps, domainflags, buf, xmlopt) < 0) goto error; } else if (uuidstr) { - virBufferAddLit(&buf, "<domain>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuidstr); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</domain>\n"); + virBufferAddLit(buf, "<domain>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</domain>\n"); } - if (virSaveCookieFormatBuf(&buf, def->cookie, + if (virSaveCookieFormatBuf(buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL) - virBufferAsprintf(&buf, "<active>%d</active>\n", def->current); + virBufferAsprintf(buf, "<active>%d</active>\n", def->current); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</domainsnapshot>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</domainsnapshot>\n"); - if (virBufferCheckError(&buf) < 0) - return NULL; + if (virBufferCheckError(buf) < 0) + goto error; - return virBufferContentAndReset(&buf); + return 0; error: - virBufferFreeAndReset(&buf); - return NULL; + virBufferFreeAndReset(buf); + return -1; +} + + +char * +virDomainSnapshotDefFormat(const char *uuidstr, + virDomainSnapshotDefPtr def, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE | + VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL, NULL); + if (virDomainSnapshotDefFormatInternal(&buf, uuidstr, def, caps, + xmlopt, flags) < 0) + return NULL; + + return virBufferContentAndReset(&buf); } /* Snapshot Obj functions */ -- 2.20.1
Add a new function to output all of the domain's snapshots in one buffer. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/conf/snapshot_conf.h | 9 +++++- src/conf/snapshot_conf.c | 61 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 1b8bdde4f5..69a7750b0b 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 @@ -138,6 +138,13 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +int virDomainSnapshotObjListFormat(virBufferPtr buf, + const char *uuidstr, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, int default_snapshot, bool require_match); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index bceebb2b1f..5641e57e43 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -809,6 +809,67 @@ virDomainSnapshotDefFormat(const char *uuidstr, return virBufferContentAndReset(&buf); } + +/* Struct and callback function used as a hash table callback; each call + * appends another snapshot XML to buf, with the caller clearing the + * buffer if any callback fails. */ +struct virDomainSnapshotFormatData { + virBufferPtr buf; + const char *uuidstr; + virCapsPtr caps; + virDomainXMLOptionPtr xmlopt; + unsigned int flags; +}; + +static int +virDomainSnapshotFormatOne(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainSnapshotObjPtr snap = payload; + struct virDomainSnapshotFormatData *data = opaque; + return virDomainSnapshotDefFormatInternal(data->buf, data->uuidstr, + snap->def, data->caps, + data->xmlopt, data->flags); +} + + +/* Format the XML for all snapshots in the list into buf. On error, + * clear the buffer and return -1. */ +int +virDomainSnapshotObjListFormat(virBufferPtr buf, + const char *uuidstr, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr current_snapshot, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + struct virDomainSnapshotFormatData data = { + .buf = buf, + .uuidstr = uuidstr, + .caps = caps, + .xmlopt = xmlopt, + .flags = flags, + }; + + virBufferAddLit(buf, "<snapshots"); + if (current_snapshot) + virBufferEscapeString(buf, " current='%s'", + current_snapshot->def->name); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + if (virDomainSnapshotForEach(snapshots, virDomainSnapshotFormatOne, + &data) < 0) { + virBufferFreeAndReset(buf); + return -1; + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</snapshots>\n"); + return 0; +} + + /* Snapshot Obj functions */ static virDomainSnapshotObjPtr virDomainSnapshotObjNew(void) { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7ce7c85b55..35e0c6d9dc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -894,6 +894,7 @@ virDomainSnapshotFormatConvertXMLFlags; virDomainSnapshotIsExternal; virDomainSnapshotLocationTypeFromString; virDomainSnapshotLocationTypeToString; +virDomainSnapshotObjListFormat; virDomainSnapshotObjListFree; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNew; -- 2.20.1
Upcoming patches will add new flags for increasing the amount of information present in <domain> dumpxml, but where the source of that information is tied to somewhere other than the active or offline domain sub-definition. Make those extensions easier by updating internal callers to pass in a struct, rather than adding new parameters for each extension, so that later patches only have to patch callers that care about a new member of the struct rather than all callers. I considered updating virDomainDefFormat(), but as there were so many existing callers, it was easier to just add a new wrapper function virDomainDefFormatFull() which takes the full struct, while making the existing interface forward on to the full one. Since all callers are being adjusted anyway, reorder the parameters of virDomainDefFormatInternal to put buf first, as that tends to be more typical. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 18 ++++++++++++++---- src/conf/domain_conf.c | 33 ++++++++++++++++++++++++++------- src/conf/snapshot_conf.c | 5 ++++- src/network/bridge_driver.c | 2 +- src/qemu/qemu_domain.c | 12 ++++++------ 5 files changed, 51 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c2dcc87ba1..5086bc342a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1,7 +1,7 @@ /* * domain_conf.h: domain XML processing * - * Copyright (C) 2006-2016 Red Hat, Inc. + * Copyright (C) 2006-2019 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * @@ -3194,17 +3194,27 @@ void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); VIR_DOMAIN_XML_MIGRATABLE) unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); +/* Struct of extra information that may be required while formatting + * domains, according to the flags in use. */ +typedef struct _virDomainDefFormatData { + virCapsPtr caps; +} virDomainDefFormatData; +typedef struct _virDomainDefFormatData *virDomainDefFormatDataPtr; + char *virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags); +char *virDomainDefFormatFull(virDomainDefPtr def, + virDomainDefFormatDataPtr data, + unsigned int flags); char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt, virDomainObjPtr obj, virCapsPtr caps, unsigned int flags); -int virDomainDefFormatInternal(virDomainDefPtr def, - virCapsPtr caps, +int virDomainDefFormatInternal(virBufferPtr buf, + virDomainDefPtr def, + virDomainDefFormatDataPtr data, unsigned int flags, - virBufferPtr buf, virDomainXMLOptionPtr xmlopt); int virDomainDiskSourceFormat(virBufferPtr buf, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bfafac407e..941d582dc1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27826,14 +27826,15 @@ virDomainVsockDefFormat(virBufferPtr buf, /* This internal version appends to an existing buffer * (possibly with auto-indent), rather than flattening * to string. - * Return -1 on failure. */ + * Return -1 on failure, with the buffer reset. */ int -virDomainDefFormatInternal(virDomainDefPtr def, - virCapsPtr caps, +virDomainDefFormatInternal(virBufferPtr buf, + virDomainDefPtr def, + virDomainDefFormatDataPtr data, unsigned int flags, - virBufferPtr buf, virDomainXMLOptionPtr xmlopt) { + virCapsPtr caps = data ? data->caps : NULL; unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *type = NULL; @@ -28678,12 +28679,27 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) char * -virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags) +virDomainDefFormat(virDomainDefPtr def, + virCapsPtr caps, + unsigned int flags) +{ + virDomainDefFormatData data = { + .caps = caps, + }; + + return virDomainDefFormatFull(def, &data, flags); +} + + +char * +virDomainDefFormatFull(virDomainDefPtr def, + virDomainDefFormatDataPtr data, + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL); - if (virDomainDefFormatInternal(def, caps, flags, &buf, NULL) < 0) + if (virDomainDefFormatInternal(&buf, def, data, flags, NULL) < 0) return NULL; return virBufferContentAndReset(&buf); @@ -28700,6 +28716,9 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt, int state; int reason; size_t i; + virDomainDefFormatData data = { + .caps = caps, + }; state = virDomainObjGetState(obj, &reason); virBufferAsprintf(&buf, "<domstatus state='%s' reason='%s' pid='%lld'>\n", @@ -28718,7 +28737,7 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt, xmlopt->privateData.format(&buf, obj) < 0) goto error; - if (virDomainDefFormatInternal(obj->def, caps, flags, &buf, xmlopt) < 0) + if (virDomainDefFormatInternal(&buf, obj->def, &data, flags, xmlopt) < 0) goto error; virBufferAdjustIndent(&buf, -2); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 5641e57e43..206b05c172 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -715,6 +715,9 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, { size_t i; int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; + virDomainDefFormatData data = { + .caps = caps, + }; if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; @@ -759,7 +762,7 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, } if (def->dom) { - if (virDomainDefFormatInternal(def->dom, caps, domainflags, buf, + if (virDomainDefFormatInternal(buf, def->dom, &data, domainflags, xmlopt) < 0) goto error; } else if (uuidstr) { diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b3ca5b8a15..86048168e9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -226,7 +226,7 @@ networkRunHook(virNetworkObjPtr obj, goto cleanup; if (virNetworkDefFormatBuf(&buf, def, 0) < 0) goto cleanup; - if (dom && virDomainDefFormatInternal(dom, NULL, 0, &buf, NULL) < 0) + if (dom && virDomainDefFormatInternal(&buf, dom, NULL, 0, NULL) < 0) goto cleanup; virBufferAdjustIndent(&buf, -2); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7353a9fac..db25e1596c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7724,18 +7724,18 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, { int ret = -1; virDomainDefPtr copy = NULL; - virCapsPtr caps = NULL; virQEMUCapsPtr qemuCaps = NULL; + virDomainDefFormatData data = { 0 }; virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1); - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + if (!(data.caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; if (!(flags & (VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_MIGRATABLE))) goto format; - if (!(copy = virDomainDefCopy(def, caps, driver->xmlopt, NULL, + if (!(copy = virDomainDefCopy(def, data.caps, driver->xmlopt, NULL, flags & VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup; @@ -7894,13 +7894,13 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, } format: - ret = virDomainDefFormatInternal(def, caps, + ret = virDomainDefFormatInternal(buf, def, &data, virDomainDefFormatConvertXMLFlags(flags), - buf, driver->xmlopt); + driver->xmlopt); cleanup: virDomainDefFree(copy); - virObjectUnref(caps); + virObjectUnref(data.caps); virObjectUnref(qemuCaps); return ret; } -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Upcoming patches will add new flags for increasing the amount of information present in <domain> dumpxml, but where the source of that information is tied to somewhere other than the active or offline domain sub-definition. Make those extensions easier by updating internal callers to pass in a struct, rather than adding new parameters for each extension, so that later patches only have to patch callers that care about a new member of the struct rather than all callers.
I considered updating virDomainDefFormat(), but as there were so many existing callers, it was easier to just add a new wrapper function virDomainDefFormatFull() which takes the full struct, while making the existing interface forward on to the full one.
Since all callers are being adjusted anyway, reorder the parameters of virDomainDefFormatInternal to put buf first, as that tends to be more typical.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 18 ++++++++++++++---- src/conf/domain_conf.c | 33 ++++++++++++++++++++++++++------- src/conf/snapshot_conf.c | 5 ++++- src/network/bridge_driver.c | 2 +- src/qemu/qemu_domain.c | 12 ++++++------ 5 files changed, 51 insertions(+), 19 deletions(-)
The structure formatting of _virDomainDefFormatData and friends is a bit non-standard from what is typically done, but not incorrect. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 3/7/19 8:01 AM, John Ferlan wrote:
Upcoming patches will add new flags for increasing the amount of information present in <domain> dumpxml, but where the source of that information is tied to somewhere other than the active or offline domain sub-definition. Make those extensions easier by updating internal callers to pass in a struct, rather than adding new parameters for each extension, so that later patches only have to patch callers that care about a new member of the struct rather than all callers.
The structure formatting of _virDomainDefFormatData and friends is a bit non-standard from what is typically done, but not incorrect.
Do you have a pointer to a struct I should be copying from? I don't mind making that sort of cosmetic cleanup for code-base consistency. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 3/7/19 1:14 PM, Eric Blake wrote:
On 3/7/19 8:01 AM, John Ferlan wrote:
Upcoming patches will add new flags for increasing the amount of information present in <domain> dumpxml, but where the source of that information is tied to somewhere other than the active or offline domain sub-definition. Make those extensions easier by updating internal callers to pass in a struct, rather than adding new parameters for each extension, so that later patches only have to patch callers that care about a new member of the struct rather than all callers.
The structure formatting of _virDomainDefFormatData and friends is a bit non-standard from what is typically done, but not incorrect.
Do you have a pointer to a struct I should be copying from? I don't mind making that sort of cosmetic cleanup for code-base consistency.
Usually it's like... typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr; struct _virDomainDef { ... }; John
Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata() are right before freeing the virDomainSnapshotObjList, so it did not matter if the list's metaroot (which points to all the defined root snapshots) is left inconsistent. But an upcoming patch will want to clear all snapshots if a bulk redefine fails partway through, in which case things must be reset. Make this work by teaching the existing virDomainSnapshotUpdateRelations() to be safe regardless of the incoming state of the metaroot (since we don't want to leak that internal detail into qemu code), then fixing the qemu code to use it after deleting all snapshots. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 7 +++++-- src/qemu/qemu_domain.c | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 206b05c172..386ec82d15 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload, } /* Populate parent link and child count of all snapshots, with all - * relations starting as 0/NULL. Return 0 on success, -1 if a parent - * is missing or if a circular relationship was requested. */ + * assigned defs having relations starting as 0/NULL. Return 0 on + * success, -1 if a parent is missing or if a circular relationship + * was requested. */ int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) { struct snapshot_set_relation act = { snapshots, 0 }; + snapshots->metaroot.nchildren = 0; + snapshots->metaroot.first_child = NULL; virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act); return act.err; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index db25e1596c..3ac79fa50b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, rem.err = 0; virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, &rem); + if (rem.current) + vm->current_snapshot = NULL; + if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err) + rem.err = -1; return rem.err; } -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata() are right before freeing the virDomainSnapshotObjList, so it did not matter if the list's metaroot (which points to all the defined root snapshots) is left inconsistent. But an upcoming patch will want to clear all snapshots if a bulk redefine fails partway through, in which case things must be reset. Make this work by teaching the existing virDomainSnapshotUpdateRelations() to be safe regardless of the incoming state of the metaroot (since we don't want to leak that internal detail into qemu code), then fixing the qemu code to use it after deleting all snapshots.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 7 +++++-- src/qemu/qemu_domain.c | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-)
I have some random personal grumbling about the lack of comments for existing code. What's described in this patch as being done would seem correct, but my knowledge of snapshot's is 'cursory'.
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 206b05c172..386ec82d15 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload, }
/* Populate parent link and child count of all snapshots, with all - * relations starting as 0/NULL. Return 0 on success, -1 if a parent - * is missing or if a circular relationship was requested. */ + * assigned defs having relations starting as 0/NULL. Return 0 on + * success, -1 if a parent is missing or if a circular relationship + * was requested. */ ^^ nit: there's an extra space here
int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) { struct snapshot_set_relation act = { snapshots, 0 };
+ snapshots->metaroot.nchildren = 0; + snapshots->metaroot.first_child = NULL; virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act); return act.err; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index db25e1596c..3ac79fa50b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, rem.err = 0; virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, &rem); + if (rem.current) + vm->current_snapshot = NULL;
If rem.err != 0, is this still something that should be done Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err) + rem.err = -1;
return rem.err; }
On 3/7/19 8:02 AM, John Ferlan wrote:
On 3/4/19 10:34 PM, Eric Blake wrote:
Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata() are right before freeing the virDomainSnapshotObjList, so it did not matter if the list's metaroot (which points to all the defined root snapshots) is left inconsistent. But an upcoming patch will want to clear all snapshots if a bulk redefine fails partway through, in which case things must be reset. Make this work by teaching the existing virDomainSnapshotUpdateRelations() to be safe regardless of the incoming state of the metaroot (since we don't want to leak that internal detail into qemu code), then fixing the qemu code to use it after deleting all snapshots.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 7 +++++-- src/qemu/qemu_domain.c | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-)
I have some random personal grumbling about the lack of comments for existing code. What's described in this patch as being done would seem correct, but my knowledge of snapshot's is 'cursory'.
Yeah, and you already tasked me with refactoring virDomainSnapshotObj and virDomainCheckpointObj to share a common, better-commented, parent class. So hopefully there are patches in the next few days to improve the situation (or at least isolate it into one file instead of the current leaky abstraction where src/qemu is poking at .first_child contents in the first place).
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 206b05c172..386ec82d15 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload, }
/* Populate parent link and child count of all snapshots, with all - * relations starting as 0/NULL. Return 0 on success, -1 if a parent - * is missing or if a circular relationship was requested. */ + * assigned defs having relations starting as 0/NULL. Return 0 on + * success, -1 if a parent is missing or if a circular relationship + * was requested. */ ^^ nit: there's an extra space here
Pre-existing, and habitual, but I'll clean up the ones you or I notice.
+++ b/src/qemu/qemu_domain.c @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, rem.err = 0; virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, &rem); + if (rem.current) + vm->current_snapshot = NULL;
If rem.err != 0, is this still something that should be done
Yes. rem.current is set only if we removed the current snapshot's metadata; even if we later failed halfway through before removing ALL snapshots, keeping the current_snapshot pointer around would be pointing into free'd memory. So the pointer must be cleared whether we succeed or fail to match that we did remove that metadata (and since we don't have any good way to undo a partial failure). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Make it possible to grab all snapshot XMLs via a single API call, by adding a new internal flag, and expanding the struct used to pass extra data for formatting. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/conf/domain_conf.c | 23 ++++++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5086bc342a..34c0b8cea1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3120,6 +3120,7 @@ typedef enum { VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6, VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7, VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8, + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS = 1 << 9, } virDomainDefFormatFlags; /* Use these flags to skip specific domain ABI consistency checks done @@ -3198,6 +3199,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); * domains, according to the flags in use. */ typedef struct _virDomainDefFormatData { virCapsPtr caps; + virDomainSnapshotObjListPtr snapshots; + virDomainSnapshotObjPtr current_snapshot; } virDomainDefFormatData; typedef struct _virDomainDefFormatData *virDomainDefFormatDataPtr; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 941d582dc1..f383f00b8b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2016 Red Hat, Inc. + * Copyright (C) 2006-2019 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * @@ -27848,7 +27848,8 @@ virDomainDefFormatInternal(virBufferPtr buf, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { @@ -28643,6 +28644,21 @@ virDomainDefFormatInternal(virBufferPtr buf, virDomainSEVDefFormat(buf, def->sev); + if (flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) { + unsigned int snapflags = flags & VIR_DOMAIN_DEF_FORMAT_SECURE ? + VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE : 0; + + if (!(data && data->snapshots)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("snapshots requested but not provided")); + goto error; + } + if (virDomainSnapshotObjListFormat(buf, uuidstr, data->snapshots, + data->current_snapshot, caps, + xmlopt, snapflags) < 0) + goto error; + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</domain>\n"); @@ -28698,7 +28714,8 @@ virDomainDefFormatFull(virDomainDefPtr def, { virBuffer buf = VIR_BUFFER_INITIALIZER; - virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL); + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS | + VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, NULL); if (virDomainDefFormatInternal(&buf, def, data, flags, NULL) < 0) return NULL; -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Make it possible to grab all snapshot XMLs via a single API call, by adding a new internal flag, and expanding the struct used to pass extra data for formatting.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/conf/domain_conf.c | 23 ++++++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Pull out the portion of virDomainSnapshotRefinePrep() that deals with definition sanity into a separate helper routine that can be reused with bulk redefine, leaving behind only the code specific to loop checking and in-place updates that are only needed in single-definition handling. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 186 ++++++++++++++++++++------------------- 1 file changed, 96 insertions(+), 90 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 386ec82d15..a5b05eadf4 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -426,6 +426,87 @@ virDomainSnapshotDefParseString(const char *xmlStr, } +/* Perform sanity checking on a redefined snapshot definition. If + * @other is non-NULL, this may include swapping def->dom from other + * into def. */ +static int +virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def, + const unsigned char *domain_uuid, + virDomainSnapshotObjPtr other, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + bool align_match = true; + bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT || + virDomainSnapshotDefIsExternal(def); + + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk-only flag for snapshot %s requires " + "disk-snapshot state"), + def->name); + return -1; + } + if (def->dom && memcmp(def->dom->uuid, domain_uuid, VIR_UUID_BUFLEN)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(domain_uuid, uuidstr); + virReportError(VIR_ERR_INVALID_ARG, + _("definition for snapshot %s must use uuid %s"), + def->name, uuidstr); + return -1; + } + + if (other) { + 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"), + def->name); + return -1; + } + + 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"), + def->name); + return -1; + } + + if (other->def->dom) { + if (def->dom) { + if (!virDomainDefCheckABIStability(other->def->dom, + def->dom, xmlopt)) + return -1; + } else { + /* Transfer the domain def */ + def->dom = other->def->dom; + other->def->dom = NULL; + } + } + } + + if (def->dom) { + if (external) { + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + align_match = false; + } + if (virDomainSnapshotAlignDisks(def, align_location, + align_match) < 0) + return -1; + } + + + return 0; +} + + /** * virDomainSnapshotDefAssignExternalNames: * @def: snapshot def object @@ -1325,12 +1406,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, unsigned int flags) { virDomainSnapshotDefPtr def = *defptr; - int ret = -1; - int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; - bool align_match = true; virDomainSnapshotObjPtr other; - bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT || - virDomainSnapshotDefIsExternal(def); + bool check_stolen; /* Prevent circular chains */ if (def->parent) { @@ -1338,21 +1415,21 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, virReportError(VIR_ERR_INVALID_ARG, _("cannot set snapshot %s as its own parent"), def->name); - goto cleanup; + return -1; } other = virDomainSnapshotFindByName(vm->snapshots, def->parent); if (!other) { virReportError(VIR_ERR_INVALID_ARG, _("parent %s for snapshot %s not found"), def->parent, def->name); - goto cleanup; + return -1; } while (other->def->parent) { if (STREQ(other->def->parent, def->name)) { virReportError(VIR_ERR_INVALID_ARG, _("parent %s would create cycle to %s"), other->def->name, def->name); - goto cleanup; + return -1; } other = virDomainSnapshotFindByName(vm->snapshots, other->def->parent); @@ -1364,77 +1441,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } } - /* Check that any replacement is compatible */ - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) { - virReportError(VIR_ERR_INVALID_ARG, - _("disk-only flag for snapshot %s requires " - "disk-snapshot state"), - def->name); - goto cleanup; - } - - if (def->dom && - memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_INVALID_ARG, - _("definition for snapshot %s must use uuid %s"), - def->name, uuidstr); - goto cleanup; - } - other = virDomainSnapshotFindByName(vm->snapshots, def->name); + check_stolen = other && other->def->dom; + if (virDomainSnapshotRedefineValidate(def, domain->uuid, other, xmlopt, + flags) < 0) { + /* revert stealing of the snapshot domain definition */ + if (check_stolen && def->dom && !other->def->dom) { + other->def->dom = def->dom; + def->dom = NULL; + } + return -1; + } if (other) { - 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"), - def->name); - goto cleanup; - } - - 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"), - def->name); - goto cleanup; - } - - if (other->def->dom) { - if (def->dom) { - if (!virDomainDefCheckABIStability(other->def->dom, - def->dom, xmlopt)) - goto cleanup; - } else { - /* Transfer the domain def */ - def->dom = other->def->dom; - other->def->dom = NULL; - } - } - - if (def->dom) { - if (external) { - align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; - } - - if (virDomainSnapshotAlignDisks(def, align_location, - align_match) < 0) { - /* revert stealing of the snapshot domain definition */ - if (def->dom && !other->def->dom) { - other->def->dom = def->dom; - def->dom = NULL; - } - goto cleanup; - } - } - if (other == vm->current_snapshot) { *update_current = true; vm->current_snapshot = NULL; @@ -1447,19 +1465,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, other->def = def; *defptr = NULL; *snap = other; - } else { - if (def->dom) { - if (external) { - align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; - } - if (virDomainSnapshotAlignDisks(def, align_location, - align_match) < 0) - goto cleanup; - } } - ret = 0; - cleanup: - return ret; + return 0; } -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Pull out the portion of virDomainSnapshotRefinePrep() that deals with definition sanity into a separate helper routine that can be reused with bulk redefine, leaving behind only the code specific to loop checking and in-place updates that are only needed in single-definition handling.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.c | 186 ++++++++++++++++++++------------------- 1 file changed, 96 insertions(+), 90 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Add a new function to make it possible to parse a list of snapshots at once. This is a counterpart to an earlier patch making it possible to produce all snapshots in a single XML string, and intentionally parses the same top-level element <snapshots> with an optional attribute current='name'. Note that since we know we started with no relations at all, and since checking parent relationships per-snapshot is not viable as we don't control which order the snapshots appear in, that we are fine with doing a final pass to update all parent/child relationships among the definitions. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 7 +++ src/conf/snapshot_conf.c | 111 +++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 119 insertions(+) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 69a7750b0b..f8af991907 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -132,6 +132,13 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseNode(xmlDocPtr xml, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +int virDomainSnapshotObjListParse(const char *xmlStr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index a5b05eadf4..52742d82d6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -507,6 +507,117 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def, } +/* Parse a <snapshots> XML entry into snapshots, which must start empty. + * Any <domain> sub-elements of a <domainsnapshot> must match domain_uuid. + */ +int +virDomainSnapshotObjListParse(const char *xmlStr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + int ret = -1; + xmlDocPtr xml; + xmlNodePtr root; + xmlXPathContextPtr ctxt = NULL; + int n; + size_t i; + int keepBlanksDefault = xmlKeepBlanksDefault(0); + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; + VIR_AUTOFREE(char *) current = NULL; + + if (!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) || + (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("incorrect flags for bulk parse")); + return -1; + } + if (snapshots->metaroot.nchildren || *current_snap) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bulk define of snapshots only possible with " + "no existing snapshot")); + return -1; + } + + if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_snapshot)")))) + goto cleanup; + + root = xmlDocGetRootElement(xml); + if (!virXMLNodeNameEqual(root, "snapshots")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <snapshots>"), root->name); + goto cleanup; + } + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + ctxt->node = root; + current = virXMLPropString(root, "current"); + + if ((n = virXPathNodeSet("./domainsnapshot", ctxt, &nodes)) < 0) + goto cleanup; + if (!n) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("expected at least one <domainsnapshot> child")); + goto cleanup; + } + + for (i = 0; i < n; i++) { + virDomainSnapshotDefPtr def; + virDomainSnapshotObjPtr snap; + + def = virDomainSnapshotDefParseNode(xml, nodes[i], caps, xmlopt, flags); + if (!def) + goto cleanup; + if (!(snap = virDomainSnapshotAssignDef(snapshots, def))) { + virDomainSnapshotDefFree(def); + goto cleanup; + } + if (virDomainSnapshotRedefineValidate(def, domain_uuid, NULL, NULL, + flags) < 0) + goto cleanup; + } + + if (virDomainSnapshotUpdateRelations(snapshots) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<snapshots> contains inconsistent parent-child " + "relationships")); + goto cleanup; + } + + if (current) { + if (!(*current_snap = virDomainSnapshotFindByName(snapshots, + current))) { + virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, + _("no snapshot matching current='%s'"), current); + goto cleanup; + } + (*current_snap)->def->current = true; + } + + ret = 0; + cleanup: + if (ret < 0) { + /* There were no snapshots before this call; so on error, just + * blindly delete anything created before the failure. */ + virHashRemoveAll(snapshots->objs); + snapshots->metaroot.nchildren = 0; + snapshots->metaroot.first_child = NULL; + } + VIR_FREE(current); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + xmlKeepBlanksDefault(keepBlanksDefault); + return ret; +} + + /** * virDomainSnapshotDefAssignExternalNames: * @def: snapshot def object diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 35e0c6d9dc..395e1f8764 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -899,6 +899,7 @@ virDomainSnapshotObjListFree; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNew; virDomainSnapshotObjListNum; +virDomainSnapshotObjListParse; virDomainSnapshotObjListRemove; virDomainSnapshotRedefinePrep; virDomainSnapshotStateTypeFromString; -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Add a new function to make it possible to parse a list of snapshots at once. This is a counterpart to an earlier patch making it possible to produce all snapshots in a single XML string, and intentionally parses the same top-level element <snapshots> with an optional attribute current='name'.
Note that since we know we started with no relations at all, and since checking parent relationships per-snapshot is not viable as we don't control which order the snapshots appear in, that we are fine with doing a final pass to update all parent/child relationships among the definitions.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 7 +++ src/conf/snapshot_conf.c | 111 +++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 119 insertions(+)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 69a7750b0b..f8af991907 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -132,6 +132,13 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseNode(xmlDocPtr xml, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +int virDomainSnapshotObjListParse(const char *xmlStr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainSnapshotDefPtr def, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index a5b05eadf4..52742d82d6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -507,6 +507,117 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def, }
+/* Parse a <snapshots> XML entry into snapshots, which must start empty. + * Any <domain> sub-elements of a <domainsnapshot> must match domain_uuid. + */ +int +virDomainSnapshotObjListParse(const char *xmlStr, + const unsigned char *domain_uuid, + virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr *current_snap, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + int ret = -1; + xmlDocPtr xml; + xmlNodePtr root; + xmlXPathContextPtr ctxt = NULL; + int n; + size_t i; + int keepBlanksDefault = xmlKeepBlanksDefault(0); + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; + VIR_AUTOFREE(char *) current = NULL; + + if (!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) || + (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("incorrect flags for bulk parse")); + return -1; + } + if (snapshots->metaroot.nchildren || *current_snap) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bulk define of snapshots only possible with " + "no existing snapshot")); + return -1; + } + + if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_snapshot)")))) + goto cleanup;
Could just be return -1
+ + root = xmlDocGetRootElement(xml); + if (!virXMLNodeNameEqual(root, "snapshots")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <snapshots>"), root->name); + goto cleanup; + } + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + ctxt->node = root; + current = virXMLPropString(root, "current"); + + if ((n = virXPathNodeSet("./domainsnapshot", ctxt, &nodes)) < 0) + goto cleanup; + if (!n) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("expected at least one <domainsnapshot> child")); + goto cleanup; + } + + for (i = 0; i < n; i++) { + virDomainSnapshotDefPtr def; + virDomainSnapshotObjPtr snap; + + def = virDomainSnapshotDefParseNode(xml, nodes[i], caps, xmlopt, flags); + if (!def) + goto cleanup; + if (!(snap = virDomainSnapshotAssignDef(snapshots, def))) { + virDomainSnapshotDefFree(def); + goto cleanup; + } + if (virDomainSnapshotRedefineValidate(def, domain_uuid, NULL, NULL, + flags) < 0) + goto cleanup; + } + + if (virDomainSnapshotUpdateRelations(snapshots) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<snapshots> contains inconsistent parent-child " + "relationships")); + goto cleanup; + } + + if (current) { + if (!(*current_snap = virDomainSnapshotFindByName(snapshots, + current))) { + virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, + _("no snapshot matching current='%s'"), current); + goto cleanup; + } + (*current_snap)->def->current = true; + } + + ret = 0; + cleanup: + if (ret < 0) { + /* There were no snapshots before this call; so on error, just + * blindly delete anything created before the failure. */ + virHashRemoveAll(snapshots->objs); + snapshots->metaroot.nchildren = 0; + snapshots->metaroot.first_child = NULL; + } + VIR_FREE(current);
Unnecessary / VIR_AUTOFREE Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + xmlKeepBlanksDefault(keepBlanksDefault); + return ret; +} + + /** * virDomainSnapshotDefAssignExternalNames: * @def: snapshot def object diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 35e0c6d9dc..395e1f8764 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -899,6 +899,7 @@ virDomainSnapshotObjListFree; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNew; virDomainSnapshotObjListNum; +virDomainSnapshotObjListParse; virDomainSnapshotObjListRemove; virDomainSnapshotRedefinePrep; virDomainSnapshotStateTypeFromString;
Right now, copying the state of a transient domain with snapshots from one host to another requires multiple API calls on both machines - on the host: get the domain XML, get a list of the snapshots, and then for each snapshot get the snapshot's XML; then on the destination: create the domain, then multiple domain snapshot create calls with the REDEFINE flag. This patch aims to make the process use fewer APIs by making it possible to grab the XML for all snapshots at the same time as grabbing the domain XML. Note that we had to do the modification to virDomainGetXMLDesc(), rather than virDomainSnapshotGetXMLDesc(), since the latter requires a single non-NULL snapshot object, whereas we want the list of all snapshots for the domain (even if the list has 0 elements). Once wired up in drivers in later patches, the new information is provided as: <domain ...> ... </devices> <snapshots current='name'> <domainsnapshot> ... </domainsnapshot> <domainsnapshot> ... </domainsnapshot> </snapshots> </domain> For now, I did not modify the schema to permit this information during virDomainDefineXML; it is still necessary to use virDomainSnapshotCreateXML with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE multiple times to recreate the added state output here. Unfortunately, libvirt versions between 1.2.12 and 5.0.0 will silently ignore the new flag, rather than diagnosing that they don't support it; but at least silent lack of snapshots from an older server is not a security hole. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 13 ++++++++----- src/libvirt-domain.c | 8 +++++++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1d5bdb545d..a8ebb68388 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1570,6 +1570,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_SNAPSHOTS = (1 << 4), /* include all snapshots in the dump */ } virDomainXMLFlags; typedef enum { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f383f00b8b..2cf12e4d95 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28674,11 +28674,12 @@ virDomainDefFormatInternal(virBufferPtr buf, return -1; } -/* Converts VIR_DOMAIN_XML_COMMON_FLAGS into VIR_DOMAIN_DEF_FORMAT_* - * flags, and silently ignores any other flags. Note that the caller - * should validate the set of flags it is willing to accept; see also - * the comment on VIR_DOMAIN_XML_COMMON_FLAGS about security - * considerations with adding new flags. */ +/* Converts VIR_DOMAIN_XML_COMMON_FLAGS and VIR_DOMAIN_XML_SNAPSHOTS + * into VIR_DOMAIN_DEF_FORMAT_* flags, and silently ignores any other + * flags. Note that the caller should validate the set of flags it is + * willing to accept; see also the comment on + * VIR_DOMAIN_XML_COMMON_FLAGS about security considerations with + * adding new flags. */ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) { unsigned int formatFlags = 0; @@ -28689,6 +28690,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; if (flags & VIR_DOMAIN_XML_MIGRATABLE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; + if (flags & VIR_DOMAIN_XML_SNAPSHOTS) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS; return formatFlags; } diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 072b92b717..c886e0bdbe 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -1,7 +1,7 @@ /* * libvirt-domain.c: entry points for virDomainPtr APIs * - * Copyright (C) 2006-2015 Red Hat, Inc. + * Copyright (C) 2006-2019 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -2570,6 +2570,12 @@ virDomainGetControlInfo(virDomainPtr domain, * XML might not validate against the schema, so it is mainly for * internal use. * + * If @flags contains VIR_DOMAIN_XML_SNAPSHOTS, the XML will include + * an additional <snapshots> child element describing all snapshots + * belonging to the domain, including an attribute current='name' if + * one of those snapshots is current. Note that some older servers + * silently ignore this flag instead of diagnosing it as unsupported. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ -- 2.20.1
Continue the work of the previous patch in making it possible to copy the state of a transient domain with snapshots from one host to another, by allowing the destination to perform bulk redefines. Note that the destination still has to do separate calls for creating/defining the domain first, and then redefining the snapshots (that is, there is intentional asymmetry between dumping the list in virDomainGetXMLDesc() but redefining it via virDomainSnapshotCreateXML()), but this is better than the previous state of having to make multiple REDEFINE calls. The bulk flag requires no pre-existing snapshot metadata (as that makes life much easier if there is a failure encountered partway through the list processing - simply remove all other snapshot metadatas), and makes no guarantees on which snapshot (when there are multiple) will actually be returned. Actual driver implementations will be in later patches. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 3 +++ src/libvirt-domain-snapshot.c | 23 +++++++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index d9b689abbd..7e6eff6a9a 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -71,6 +71,9 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot while the guest is running */ + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST = (1 << 9), /* parse multiple + snapshots in one + API call */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index a647a500d6..ab94d29574 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -1,7 +1,7 @@ /* * libvirt-domain-snapshot.c: entry points for virDomainSnapshotPtr APIs * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2019 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -102,7 +102,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * @flags: bitwise-OR of virDomainSnapshotCreateFlags * * Creates a new snapshot of a domain based on the snapshot xml - * contained in xmlDesc. + * contained in xmlDesc, with a top-level <domainsnapshot> element. * * If @flags is 0, the domain can be active, in which case the * snapshot will be a full system snapshot (capturing both disk state, @@ -132,8 +132,17 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * guest-visible layout. When redefining a snapshot name that does * not exist, the hypervisor may validate that reverting to the * snapshot appears to be possible (for example, disk images have - * snapshot contents by the requested name). Not all hypervisors - * support these flags. + * snapshot contents by the requested name). Alternatively, if @flags + * includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST (which requires + * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and is incompatible with + * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT), and the domain has no existing + * snapshot metadata, then @xmlDesc is parsed as a top-level + * <snapshots> element with an optional current='name' attribute, and + * containing one or more <domainsnapshot> children (as produced by + * virDomainGetXMLDesc() with the flag VIR_DOMAIN_XML_SNAPSHOTS), to + * do a bulk redefine of all snapshots at once (it is unspecified + * which of the redefined snapshots will be used as the return value + * on success). Not all hypervisors support these flags. * * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, then the * domain's disk images are modified according to @xmlDesc, but then @@ -219,6 +228,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, error); + VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + error); VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, @@ -226,6 +238,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, VIR_DOMAIN_SNAPSHOT_CREATE_HALT, error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, + error); if (conn->driver->domainSnapshotCreateXML) { virDomainSnapshotPtr ret; -- 2.20.1
On Mon, Mar 04, 2019 at 09:34:40PM -0600, Eric Blake wrote:
Continue the work of the previous patch in making it possible to copy the state of a transient domain with snapshots from one host to another, by allowing the destination to perform bulk redefines. Note that the destination still has to do separate calls for creating/defining the domain first, and then redefining the snapshots (that is, there is intentional asymmetry between dumping the list in virDomainGetXMLDesc() but redefining it via virDomainSnapshotCreateXML()), but this is better than the previous state of having to make multiple REDEFINE calls.
What is the intention behind the assymetry? It feels odd to request the list by a flag to virDomainGetXMLDesc (because we're not going to reuse the whole <domain>, just the <snapshots> sub-element here). Having a counterpart to the API doing the redefine would only mean two calls (GetXMLDesc, GetSnapshots) instead of getting every snapshot separately. Also, virDomainSnapshotCreateXML is designed for a single snapshot, using a flag to turn it into a different API ('virDomainSnapshotsCreateXML'? 'virDomainSnapshotsRedefine'?) leads to strangeness like returning a single snapshot while making no guarantees on which one it is or a repetition of this pattern: if (flags & REDEFINE_LIST) { /* ... */ goto cleanup; /* <- no fallthrough here */ } Jano
The bulk flag requires no pre-existing snapshot metadata (as that makes life much easier if there is a failure encountered partway through the list processing - simply remove all other snapshot metadatas), and makes no guarantees on which snapshot (when there are multiple) will actually be returned.
Actual driver implementations will be in later patches.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 3 +++ src/libvirt-domain-snapshot.c | 23 +++++++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-)
On 3/7/19 10:13 AM, Ján Tomko wrote:
On Mon, Mar 04, 2019 at 09:34:40PM -0600, Eric Blake wrote:
Continue the work of the previous patch in making it possible to copy the state of a transient domain with snapshots from one host to another, by allowing the destination to perform bulk redefines. Note that the destination still has to do separate calls for creating/defining the domain first, and then redefining the snapshots (that is, there is intentional asymmetry between dumping the list in virDomainGetXMLDesc() but redefining it via virDomainSnapshotCreateXML()), but this is better than the previous state of having to make multiple REDEFINE calls.
What is the intention behind the assymetry?
virDomainSnapshotGetXMLDesc won't work - you can't pass in NULL because that function requires a snapshot (in order to get at the virDomain and virConnection) to even make the call. On the flip side, I did NOT want virDomainDefine/virDomainCreate to take the <snapshots> argument, even with the presence of a flag, because there are scenarios where you want the domain defined before you add in the snapshots; virDomainSnapshotCreateXML with new flag fit that purpose well. I could, however, add a new API instead of a new flag overloading to the existing API. Naming is hard, maybe: virDomainGetSnapshotsXMLDesc since it would be a new virDomain function, but returns the new <snapshots> XML element.
It feels odd to request the list by a flag to virDomainGetXMLDesc (because we're not going to reuse the whole <domain>, just the <snapshots> sub-element here). Having a counterpart to the API doing the redefine would only mean two calls (GetXMLDesc, GetSnapshots) instead of getting every snapshot separately.
One call (virDomainGetXMLDesc with flag) is even better than two (virDomainGetXMLDesc, virDomainGetSnapshotsXMLDesc), but a new API has the benefit of not suffering from the recently-fixed bug about unknown new flags being rejected by buggy old servers.
Also, virDomainSnapshotCreateXML is designed for a single snapshot, using a flag to turn it into a different API ('virDomainSnapshotsCreateXML'? 'virDomainSnapshotsRedefine'?) leads to strangeness like returning a single snapshot while making no guarantees on which one it is or a repetition of this pattern: if (flags & REDEFINE_LIST) { /* ... */ goto cleanup; /* <- no fallthrough here */ }
If I add a new API for getting the XML, then it is not a stretch to require a new API for redefining all snapshots at once. And now that I've typed this up, the suggestion for a separate API is starting to be more appealing. Looks like I'll be posting a v4 shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add flags to the 'dumpxml' and 'snapshot-create' commands to pass the newly-added bulk snapshot flags through. For command-line convenience, I intentionally made --redefine-list imply --redefine, even though the counterpart C flags are distinct (and you get an error if you pass _REDEFINE_LIST without _REDEFINE). Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 7 +++++++ tools/virsh-snapshot.c | 14 ++++++++++++++ tools/virsh.pod | 18 ++++++++++++++---- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5699018dcc..78854b1e0a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10055,6 +10055,10 @@ static const vshCmdOptDef opts_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("provide XML suitable for migrations") }, + {.name = "snapshots", + .type = VSH_OT_BOOL, + .help = N_("include all domain snapshots in XML dump"), + }, {.name = NULL} }; @@ -10069,6 +10073,7 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) bool secure = vshCommandOptBool(cmd, "security-info"); bool update = vshCommandOptBool(cmd, "update-cpu"); bool migratable = vshCommandOptBool(cmd, "migratable"); + bool snapshots = vshCommandOptBool(cmd, "snapshots"); if (inactive) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -10078,6 +10083,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_UPDATE_CPU; if (migratable) flags |= VIR_DOMAIN_XML_MIGRATABLE; + if (snapshots) + flags |= VIR_DOMAIN_XML_SNAPSHOTS; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e38ebb1f28..cbb7d744f9 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -80,6 +80,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + vshPrintExtra(ctl, "%s", + _("Domain snapshot list imported successfully")); + ret = true; + goto cleanup; + } + name = virDomainSnapshotGetName(snapshot); if (!name) { vshError(ctl, "%s", _("Could not get snapshot name")); @@ -122,6 +129,10 @@ static const vshCmdOptDef opts_snapshot_create[] = { .help = N_("redefine metadata for existing snapshot") }, VIRSH_COMMON_OPT_CURRENT(N_("with redefine, set current snapshot")), + {.name = "redefine-list", + .type = VSH_OT_BOOL, + .help = N_("bulk define a set of snapshots, implies --redefine"), + }, {.name = "no-metadata", .type = VSH_OT_BOOL, .help = N_("take snapshot but create no metadata") @@ -177,6 +188,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "redefine-list")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 8e18b30f29..ea51584d3e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1647,7 +1647,7 @@ is required in order to produce valid ELF file which can be later processed by the crash utility. =item B<dumpxml> I<domain> [I<--inactive>] [I<--security-info>] -[I<--update-cpu>] [I<--migratable>] +[I<--update-cpu>] [I<--migratable>] [I<--snapshots>] Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be @@ -1660,6 +1660,9 @@ migrations, i.e., compatible with older libvirt releases and possibly amended with internal run-time options. This option may automatically enable other options (I<--update-cpu>, I<--security-info>, ...) as necessary. +Using I<--snapshots> will expand the output to also include information on +all domain snapshots, for servers that understand the flag. + =item B<edit> I<domain> Edit the XML configuration file for a domain, which will affect the @@ -4544,8 +4547,9 @@ used to represent properties of snapshots. =over 4 -=item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] -| [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] +=item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> +{[I<--current>] | [I<--redefine-list>]}] | [I<--no-metadata>] +[I<--halt>] [I<--disk-only>] [I<--reuse-external>] [I<--quiesce>] [I<--atomic>] [I<--live>]} Create a snapshot for domain I<domain> with the properties specified in @@ -4575,7 +4579,12 @@ the same name and UUID, or to make slight alterations in the snapshot metadata (such as host-specific aspects of the domain XML embedded in the snapshot). When this flag is supplied, the I<xmlfile> argument is mandatory, and the domain's current snapshot will not be altered -unless the I<--current> flag is also given. +unless the I<--current> flag is also given. If I<--redefine-list> is +specified, I<--redefine> is implied, I<--current> is rejected, and +the XML changes from being a single <domainsnapshot> to instead being +a <snapshots> element describing a list of snapshots. List form only +works if the domain has no currently-defined snapshot metadata, and + can be obtained as a subset of I<dumpxml --snapshots> output. If I<--no-metadata> is specified, then the snapshot data is created, but any metadata is immediately discarded (that is, libvirt does not @@ -4779,6 +4788,7 @@ files for disk images or memory state. Output the snapshot XML for the domain's snapshot named I<snapshot>. Using I<--security-info> will also include security sensitive information. Use B<snapshot-current> to easily access the XML of the current snapshot. +To grab the XML for all snapshots at once, use B<dumpxml --snapshots>. =item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>} -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Add flags to the 'dumpxml' and 'snapshot-create' commands to pass the newly-added bulk snapshot flags through.
For command-line convenience, I intentionally made --redefine-list imply --redefine, even though the counterpart C flags are distinct (and you get an error if you pass _REDEFINE_LIST without _REDEFINE).
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 7 +++++++ tools/virsh-snapshot.c | 14 ++++++++++++++ tools/virsh.pod | 18 ++++++++++++++---- 3 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5699018dcc..78854b1e0a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10055,6 +10055,10 @@ static const vshCmdOptDef opts_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("provide XML suitable for migrations") }, + {.name = "snapshots", + .type = VSH_OT_BOOL, + .help = N_("include all domain snapshots in XML dump"), + }, {.name = NULL} };
@@ -10069,6 +10073,7 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) bool secure = vshCommandOptBool(cmd, "security-info"); bool update = vshCommandOptBool(cmd, "update-cpu"); bool migratable = vshCommandOptBool(cmd, "migratable"); + bool snapshots = vshCommandOptBool(cmd, "snapshots");
if (inactive) flags |= VIR_DOMAIN_XML_INACTIVE; @@ -10078,6 +10083,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_UPDATE_CPU; if (migratable) flags |= VIR_DOMAIN_XML_MIGRATABLE; + if (snapshots) + flags |= VIR_DOMAIN_XML_SNAPSHOTS;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e38ebb1f28..cbb7d744f9 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -80,6 +80,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, goto cleanup; }
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + vshPrintExtra(ctl, "%s", + _("Domain snapshot list imported successfully")); + ret = true; + goto cleanup; + } + name = virDomainSnapshotGetName(snapshot); if (!name) { vshError(ctl, "%s", _("Could not get snapshot name")); @@ -122,6 +129,10 @@ static const vshCmdOptDef opts_snapshot_create[] = { .help = N_("redefine metadata for existing snapshot") }, VIRSH_COMMON_OPT_CURRENT(N_("with redefine, set current snapshot")), + {.name = "redefine-list", + .type = VSH_OT_BOOL, + .help = N_("bulk define a set of snapshots, implies --redefine"), + }, {.name = "no-metadata", .type = VSH_OT_BOOL, .help = N_("take snapshot but create no metadata") @@ -177,6 +188,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "redefine-list")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 8e18b30f29..ea51584d3e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1647,7 +1647,7 @@ is required in order to produce valid ELF file which can be later processed by the crash utility.
=item B<dumpxml> I<domain> [I<--inactive>] [I<--security-info>] -[I<--update-cpu>] [I<--migratable>] +[I<--update-cpu>] [I<--migratable>] [I<--snapshots>]
Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be @@ -1660,6 +1660,9 @@ migrations, i.e., compatible with older libvirt releases and possibly amended with internal run-time options. This option may automatically enable other options (I<--update-cpu>, I<--security-info>, ...) as necessary.
+Using I<--snapshots> will expand the output to also include information on +all domain snapshots, for servers that understand the flag.
s/, for/ for/ s/understand/support/ (remove comma)...
+ =item B<edit> I<domain>
Edit the XML configuration file for a domain, which will affect the @@ -4544,8 +4547,9 @@ used to represent properties of snapshots.
=over 4
-=item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] -| [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] +=item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> +{[I<--current>] | [I<--redefine-list>]}] | [I<--no-metadata>] +[I<--halt>] [I<--disk-only>] [I<--reuse-external>] [I<--quiesce>] [I<--atomic>] [I<--live>]}
Create a snapshot for domain I<domain> with the properties specified in @@ -4575,7 +4579,12 @@ the same name and UUID, or to make slight alterations in the snapshot metadata (such as host-specific aspects of the domain XML embedded in the snapshot). When this flag is supplied, the I<xmlfile> argument is mandatory, and the domain's current snapshot will not be altered -unless the I<--current> flag is also given. +unless the I<--current> flag is also given. If I<--redefine-list> is
New paragraph makes it easier to read...
+specified, I<--redefine> is implied, I<--current> is rejected, and +the XML changes from being a single <domainsnapshot> to instead being +a <snapshots> element describing a list of snapshots. List form only
? s/list of snapshots/list of all snapshots/ ?
+works if the domain has no currently-defined snapshot metadata, and
s/, and/ and/
+ can be obtained as a subset of I<dumpxml --snapshots> output.
s/ can/can (extra space) Reviewed-by: John Ferlan <jferlan@redhat.com> John
If I<--no-metadata> is specified, then the snapshot data is created, but any metadata is immediately discarded (that is, libvirt does not @@ -4779,6 +4788,7 @@ files for disk images or memory state. Output the snapshot XML for the domain's snapshot named I<snapshot>. Using I<--security-info> will also include security sensitive information. Use B<snapshot-current> to easily access the XML of the current snapshot. +To grab the XML for all snapshots at once, use B<dumpxml --snapshots>.
=item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>}
Implement the new flags for bulk snapshot dump and redefine. The bulk of the work is already done by the common code. Since each connection to test:///default restarts at the same canned state, this can easily be tested with: $ virsh -c test:///default " snapshot-create-as test s1 snapshot-create-as test s2 dumpxml test --snapshots" | sed -n '/<snapshots/,/<.snapshots/p' > list.xml $ virsh -c test:///default " snapshot-list test snapshot-create test --redefine-list list.xml snapshot-current --name test snapshot-list --parent test " Name Creation Time State ------------------------------- Domain snapshot list imported successfully s2 Name Creation Time State Parent ------------------------------------------------------ s1 2019-02-20 22:26:52 -0600 running s2 2019-02-20 22:26:52 -0600 running s1 The test driver also makes it easy to test input validation, by modifying list.xml incorrectly (such as trying to attempt circular dependencies). Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c5de17f366..4e85f6627a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2627,8 +2627,11 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainDefPtr def; virDomainObjPtr privdom; char *ret = NULL; + virDomainDefFormatData data = { + .caps = privconn->caps, + }; - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_SNAPSHOTS, NULL); if (!(privdom = testDomObjFromDomain(domain))) return NULL; @@ -2636,8 +2639,10 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) def = (flags & VIR_DOMAIN_XML_INACTIVE) && privdom->newDef ? privdom->newDef : privdom->def; - ret = virDomainDefFormat(def, privconn->caps, - virDomainDefFormatConvertXMLFlags(flags)); + data.snapshots = privdom->snapshots; + data.current_snapshot = privdom->current_snapshot; + ret = virDomainDefFormatFull(def, &data, + virDomainDefFormatConvertXMLFlags(flags)); virDomainObjEndAPI(&privdom); return ret; @@ -6314,6 +6319,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, * QUIESCE: Nothing to do * ATOMIC: Nothing to do * LIVE: Nothing to do + * REDEFINE_LIST: Implemented */ virCheckFlags( VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | @@ -6321,7 +6327,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_HALT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL); if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))) update_current = false; @@ -6337,6 +6344,18 @@ testDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + if (virDomainSnapshotObjListParse(xmlDesc, vm->def->uuid, vm->snapshots, + &vm->current_snapshot, privconn->caps, + privconn->xmlopt, parse_flags) < 0) + goto cleanup; + + /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL); + snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name); + goto cleanup; + } + if (!(def = virDomainSnapshotDefParseString(xmlDesc, privconn->caps, privconn->xmlopt, @@ -6384,7 +6403,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: if (vm) { - if (snapshot) { + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST)) { virDomainSnapshotObjPtr other; if (update_current) vm->current_snapshot = snap; -- 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). Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 120 ++++++++++++++++++++++------------------- 1 file changed, 66 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 76fab0421b..39cc45537d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15673,6 +15673,69 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, } +/* Validate that a snapshot object does not violate any qemu-specific + * constraints. */ +static int +qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, + virDomainSnapshotState 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_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_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_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"), + virDomainSnapshotStateTypeToString(state)); + return -1; + } + return 0; +} + + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15693,7 +15756,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; - virDomainSnapshotState state; virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | @@ -15748,25 +15810,9 @@ 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 && @@ -15778,40 +15824,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } - /* allow snapshots only in certain states */ - state = redefine ? def->state : vm->state.state; - switch (state) { - /* valid states */ - 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_SNAP_STATE_DISK_SNAPSHOT: - if (!redefine) { - virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), - virDomainSnapshotStateTypeToString(state)); - goto cleanup; - } - break; - - case VIR_SNAP_STATE_PMSUSPENDED: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("qemu doesn't support taking snapshots of " - "PMSUSPENDED guests")); - goto cleanup; - - /* invalid states */ - 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"), - virDomainSnapshotStateTypeToString(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 3/4/19 10:34 PM, Eric Blake wrote:
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).
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 120 ++++++++++++++++++++++------------------- 1 file changed, 66 insertions(+), 54 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
qemuDomainSnapshotWriteMetadata does not modify the directory name, and making it const-correct aids in writing an upcoming patch. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7c6b50184c..37c9813ec5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -683,7 +683,7 @@ int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virDomainSnapshotObjPtr snapshot, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - char *snapshotDir); + const char *snapshotDir); int qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3ac79fa50b..6f8e03ba36 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8399,7 +8399,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virDomainSnapshotObjPtr snapshot, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, - char *snapshotDir) + const char *snapshotDir) { char *newxml = NULL; int ret = -1; -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
qemuDomainSnapshotWriteMetadata does not modify the directory name, and making it const-correct aids in writing an upcoming patch.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Implement the new flags for bulk snapshot dump and redefine. This borrows from ideas in the test driver, but is further complicated by the fact that qemu must write snapshot XML to disk, and thus must do additional validation after the initial parse to ensure the user didn't attempt to rename a snapshot with "../" or similar. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_domain.c | 26 +++++++++++++---- src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6f8e03ba36..68298ad4e8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7717,6 +7717,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver, static int qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainDefPtr def, virCPUDefPtr origCPU, unsigned int flags, @@ -7726,8 +7727,10 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, virDomainDefPtr copy = NULL; virQEMUCapsPtr qemuCaps = NULL; virDomainDefFormatData data = { 0 }; + bool snapshots = flags & VIR_DOMAIN_XML_SNAPSHOTS; - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU | + VIR_DOMAIN_XML_SNAPSHOTS, -1); if (!(data.caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -7894,6 +7897,15 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, } format: + if (snapshots) { + if (!vm) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("snapshots XML requested but not provided")); + goto cleanup; + } + data.snapshots = vm->snapshots; + data.current_snapshot = vm->current_snapshot; + } ret = virDomainDefFormatInternal(buf, def, &data, virDomainDefFormatConvertXMLFlags(flags), driver->xmlopt); @@ -7912,19 +7924,21 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, unsigned int flags, virBufferPtr buf) { - return qemuDomainDefFormatBufInternal(driver, def, NULL, flags, buf); + return qemuDomainDefFormatBufInternal(driver, NULL, def, NULL, flags, buf); } static char * qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver, + virDomainObjPtr vm, virDomainDefPtr def, virCPUDefPtr origCPU, unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (qemuDomainDefFormatBufInternal(driver, def, origCPU, flags, &buf) < 0) + if (qemuDomainDefFormatBufInternal(driver, vm, def, origCPU, flags, + &buf) < 0) return NULL; return virBufferContentAndReset(&buf); @@ -7936,7 +7950,7 @@ qemuDomainDefFormatXML(virQEMUDriverPtr driver, virDomainDefPtr def, unsigned int flags) { - return qemuDomainDefFormatXMLInternal(driver, def, NULL, flags); + return qemuDomainDefFormatXMLInternal(driver, NULL, def, NULL, flags); } @@ -7955,7 +7969,7 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver, origCPU = priv->origCPU; } - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); + return qemuDomainDefFormatXMLInternal(driver, vm, def, origCPU, flags); } char * @@ -7972,7 +7986,7 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver, if (compatible) flags |= VIR_DOMAIN_XML_MIGRATABLE; - return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); + return qemuDomainDefFormatXMLInternal(driver, NULL, def, origCPU, flags); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39cc45537d..6a8f8e2bbe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7341,8 +7341,8 @@ static char virDomainObjPtr vm; char *ret = NULL; - virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, - NULL); + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU | + VIR_DOMAIN_XML_SNAPSHOTS, NULL); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15736,6 +15736,33 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, } +/* Struct and hash-iterator callback used when bulk redefining snapshots */ +struct qemuDomainSnapshotBulk { + virDomainObjPtr vm; + virQEMUDriverPtr driver; + const char *snapshotDir; + unsigned int flags; +}; + +static int +qemuDomainSnapshotBulkRedefine(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainSnapshotObjPtr snap = payload; + struct qemuDomainSnapshotBulk *data = opaque; + + if (qemuDomainSnapshotValidate(snap->def, snap->def->state, + data->flags) < 0) + return -1; + if (qemuDomainSnapshotWriteMetadata(data->vm, snap, data->driver->caps, + data->driver->xmlopt, + data->snapshotDir) < 0) + return -1; + return 0; +} + + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15765,7 +15792,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL); VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, @@ -15797,6 +15825,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + struct qemuDomainSnapshotBulk bulk = { + .vm = vm, + .driver = driver, + .snapshotDir = cfg->snapshotDir, + .flags = flags, + }; + + if (virDomainSnapshotObjListParse(xmlDesc, vm->def->uuid, + vm->snapshots, &vm->current_snapshot, + caps, driver->xmlopt, + parse_flags) < 0) + goto cleanup; + /* Validate and save the snapshots to disk. Since we don't get + * here unless there were no snapshots beforehand, just delete + * everything if anything failed, ignoring further errors. */ + if (virDomainSnapshotForEach(vm->snapshots, + qemuDomainSnapshotBulkRedefine, + &bulk) < 0) { + virErrorPtr orig_err = virSaveLastError(); + + qemuDomainSnapshotDiscardAllMetadata(driver, vm); + virSetError(orig_err); + virFreeError(orig_err); + goto cleanup; + } + /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL); + snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name); + goto cleanup; + } + if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot halt after transient domain snapshot")); -- 2.20.1
On 3/4/19 10:34 PM, Eric Blake wrote:
Implement the new flags for bulk snapshot dump and redefine. This borrows from ideas in the test driver, but is further complicated by the fact that qemu must write snapshot XML to disk, and thus must do additional validation after the initial parse to ensure the user didn't attempt to rename a snapshot with "../" or similar.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_domain.c | 26 +++++++++++++---- src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 9 deletions(-)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39cc45537d..6a8f8e2bbe 100644
[...]
static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15765,7 +15792,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL);
VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, @@ -15797,6 +15825,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; }
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) { + struct qemuDomainSnapshotBulk bulk = { + .vm = vm, + .driver = driver, + .snapshotDir = cfg->snapshotDir, + .flags = flags, + }; + + if (virDomainSnapshotObjListParse(xmlDesc, vm->def->uuid, + vm->snapshots, &vm->current_snapshot, + caps, driver->xmlopt, + parse_flags) < 0) + goto cleanup; + /* Validate and save the snapshots to disk. Since we don't get + * here unless there were no snapshots beforehand, just delete + * everything if anything failed, ignoring further errors. */ + if (virDomainSnapshotForEach(vm->snapshots, + qemuDomainSnapshotBulkRedefine, + &bulk) < 0) { + virErrorPtr orig_err = virSaveLastError();
I've seen newer code using virErrorPreserveLast
+ + qemuDomainSnapshotDiscardAllMetadata(driver, vm); + virSetError(orig_err); + virFreeError(orig_err);
Similar newer code using virErrorRestore instead of both
+ goto cleanup; + } + /* Return is arbitrary, so use the first root */ + snap = virDomainSnapshotFindByName(vm->snapshots, NULL); + snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name); + goto cleanup; + } + if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot halt after transient domain snapshot"));
Reviewed-by: John Ferlan <jferlan@redhat.com> John FYI: I ran the series through Coverity too with no new errors
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko