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(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

The existing virDomainSnapshotState is a superset of virDomainState, adding one more state (disk-snapshot) on top of valid domain states. But as written, the enum cannot be used for gcc validation that all enum values are covered in a strongly-typed switch condition, because the enum does not explicitly include the values it is adding to. Copy the style used in qemu_blockjob.h of creating new enum names for every inherited value, and update most clients to use the new enum names anywhere snapshot state is referenced. The exception is two switch statements in qemu code, which instead gain a fixme comment about odd type usage (which will be cleaned up in the next patch). The rest of the patch is mechanical. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/snapshot_conf.h | 21 ++++++++++++++++++--- src/conf/snapshot_conf.c | 28 ++++++++++++++-------------- src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------ src/test/test_driver.c | 20 ++++++++++---------- src/vbox/vbox_common.c | 4 ++-- 5 files changed, 66 insertions(+), 41 deletions(-) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 7a175dfc96..9084f5fb8b 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -36,11 +36,26 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_LOCATION_LAST } virDomainSnapshotLocation; +/** + * This enum has to map all known domain states from the public enum + * virDomainState, before adding one additional state possible only + * for snapshots. + */ typedef enum { - /* Inherit the VIR_DOMAIN_* states from virDomainState. */ - VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, - VIR_DOMAIN_SNAPSHOT_STATE_LAST + /* Mapped to public enum */ + VIR_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE, + VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING, + VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED, + VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED, + VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN, + VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF, + VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED, + VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED, + /* Additional enum values local to qemu */ + VIR_SNAP_STATE_DISK_SNAPSHOT, + VIR_SNAP_STATE_LAST } virDomainSnapshotState; +verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST); /* Stores disk-snapshot information */ typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 41236d9932..299fc2101b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -58,7 +58,7 @@ VIR_ENUM_IMPL(virDomainSnapshotLocation, VIR_DOMAIN_SNAPSHOT_LOCATION_LAST, ); /* virDomainSnapshotState is really virDomainState plus one extra state */ -VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST, +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_SNAP_STATE_LAST, "nostate", "running", "blocked", @@ -257,8 +257,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, state); goto cleanup; } - offline = (def->state == VIR_DOMAIN_SHUTOFF || - def->state == VIR_DOMAIN_DISK_SNAPSHOT); + offline = (def->state == VIR_SNAP_STATE_SHUTOFF || + def->state == VIR_SNAP_STATE_DISK_SNAPSHOT); /* Older snapshots were created with just <domain>/<uuid>, and * lack domain/@type. In that case, leave dom NULL, and @@ -879,14 +879,14 @@ static int virDomainSnapshotObjListCopyNames(void *payload, if (data->flags & VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS) { if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE) && - obj->def->state == VIR_DOMAIN_SHUTOFF) + obj->def->state == VIR_SNAP_STATE_SHUTOFF) return 0; if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) && - obj->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + obj->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) return 0; if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE) && - obj->def->state != VIR_DOMAIN_SHUTOFF && - obj->def->state != VIR_DOMAIN_DISK_SNAPSHOT) + obj->def->state != VIR_SNAP_STATE_SHUTOFF && + obj->def->state != VIR_SNAP_STATE_DISK_SNAPSHOT) return 0; } @@ -1225,7 +1225,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; virDomainSnapshotObjPtr other; - bool external = def->state == VIR_DOMAIN_DISK_SNAPSHOT || + bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def); /* Prevent circular chains */ @@ -1282,10 +1282,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { - if ((other->def->state == VIR_DOMAIN_RUNNING || - other->def->state == VIR_DOMAIN_PAUSED) != - (def->state == VIR_DOMAIN_RUNNING || - def->state == VIR_DOMAIN_PAUSED)) { + if ((other->def->state == VIR_SNAP_STATE_RUNNING || + other->def->state == VIR_SNAP_STATE_PAUSED) != + (def->state == VIR_SNAP_STATE_RUNNING || + def->state == VIR_SNAP_STATE_PAUSED)) { virReportError(VIR_ERR_INVALID_ARG, _("cannot change between online and offline " "snapshot state in snapshot %s"), @@ -1293,8 +1293,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; } - if ((other->def->state == VIR_DOMAIN_DISK_SNAPSHOT) != - (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + if ((other->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) != + (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) { virReportError(VIR_ERR_INVALID_ARG, _("cannot change between disk only and " "full system in snapshot %s"), diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7e2b5b9106..2abe3417c9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15022,7 +15022,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: found_internal = true; - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && active) { + if (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT && active) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("active qemu domains require external disk " "snapshots; disk %s requested internal"), @@ -15099,7 +15099,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, } /* disk snapshot requires at least one disk */ - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && !external) { + if (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT && !external) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("disk-only snapshots require at least " "one disk to be selected for snapshot")); @@ -15781,8 +15781,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, /* allow snapshots only in certain states */ state = vm->state.state; if (redefine) - state = def->state == VIR_DOMAIN_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF : + state = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF : def->state; + /* FIXME: state should be virDomainSnapshotState, with the switch + * statement handling of VIR_SNAP_STATE_DISK_SNAPSHOT (the only + * enum value added beyond what virDomainState supports). But for + * now it doesn't matter, because we slammed the extra snapshot + * state into a safe domain state. */ switch (state) { /* valid states */ case VIR_DOMAIN_RUNNING: @@ -15838,9 +15843,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; if (virDomainObjIsActive(vm)) - def->state = VIR_DOMAIN_DISK_SNAPSHOT; + def->state = VIR_SNAP_STATE_DISK_SNAPSHOT; else - def->state = VIR_DOMAIN_SHUTOFF; + def->state = VIR_SNAP_STATE_SHUTOFF; def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); @@ -15857,7 +15862,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto endjob; } - def->memory = (def->state == VIR_DOMAIN_SHUTOFF ? + def->memory = (def->state == VIR_SNAP_STATE_SHUTOFF ? VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); } @@ -16420,8 +16425,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; if (!vm->persistent && - snap->def->state != VIR_DOMAIN_RUNNING && - snap->def->state != VIR_DOMAIN_PAUSED && + snap->def->state != VIR_SNAP_STATE_RUNNING && + snap->def->state != VIR_SNAP_STATE_PAUSED && (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -16444,8 +16449,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } if (virDomainObjIsActive(vm) && - !(snap->def->state == VIR_DOMAIN_RUNNING - || snap->def->state == VIR_DOMAIN_PAUSED) && + !(snap->def->state == VIR_SNAP_STATE_RUNNING || + snap->def->state == VIR_SNAP_STATE_PAUSED) && (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", @@ -16481,6 +16486,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, cookie = (qemuDomainSaveCookiePtr) snap->def->cookie; + /* FIXME: This cast should be to virDomainSnapshotState, with + * better handling of VIR_SNAP_STATE_DISK_SNAPSHOT (the only enum + * value added beyond what virDomainState supports). But for now + * it doesn't matter, because of the above rejection of revert to + * external snapshots. */ switch ((virDomainState) snap->def->state) { case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_PAUSED: @@ -16622,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Touch up domain state. */ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && - (snap->def->state == VIR_DOMAIN_PAUSED || + (snap->def->state == VIR_SNAP_STATE_PAUSED || (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { /* Transitions 3, 6, 9 */ virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, @@ -16729,7 +16739,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid target domain state '%s'. Refusing " "snapshot reversion"), - virDomainStateTypeToString(snap->def->state)); + virDomainSnapshotStateTypeToString(snap->def->state)); goto endjob; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce0df1f8e3..4a6555e0ea 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6271,9 +6271,9 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm, align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; if (virDomainObjIsActive(vm)) - def->state = VIR_DOMAIN_DISK_SNAPSHOT; + def->state = VIR_SNAP_STATE_DISK_SNAPSHOT; else - def->state = VIR_DOMAIN_SHUTOFF; + def->state = VIR_SNAP_STATE_SHUTOFF; def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); @@ -6281,7 +6281,7 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm, align_match = false; } else { def->state = virDomainObjGetState(vm, NULL); - def->memory = def->state == VIR_DOMAIN_SHUTOFF ? + def->memory = def->state == VIR_SNAP_STATE_SHUTOFF ? VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } @@ -6572,8 +6572,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; if (!vm->persistent && - snap->def->state != VIR_DOMAIN_RUNNING && - snap->def->state != VIR_DOMAIN_PAUSED && + snap->def->state != VIR_SNAP_STATE_RUNNING && + snap->def->state != VIR_SNAP_STATE_PAUSED && (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -6590,8 +6590,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } if (virDomainObjIsActive(vm) && - !(snap->def->state == VIR_DOMAIN_RUNNING - || snap->def->state == VIR_DOMAIN_PAUSED) && + !(snap->def->state == VIR_SNAP_STATE_RUNNING || + snap->def->state == VIR_SNAP_STATE_PAUSED) && (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", @@ -6612,8 +6612,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (!config) goto cleanup; - if (snap->def->state == VIR_DOMAIN_RUNNING || - snap->def->state == VIR_DOMAIN_PAUSED) { + if (snap->def->state == VIR_SNAP_STATE_RUNNING || + snap->def->state == VIR_SNAP_STATE_PAUSED) { /* Transitions 2, 3, 5, 6, 8, 9 */ bool was_running = false; bool was_stopped = false; @@ -6672,7 +6672,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Touch up domain state. */ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && - (snap->def->state == VIR_DOMAIN_PAUSED || + (snap->def->state == VIR_SNAP_STATE_PAUSED || (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { /* Transitions 3, 6, 9 */ virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 0a27deeaf8..9c2bd556bd 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -6316,9 +6316,9 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, goto cleanup; } if (online) - def->state = VIR_DOMAIN_RUNNING; + def->state = VIR_SNAP_STATE_RUNNING; else - def->state = VIR_DOMAIN_SHUTOFF; + def->state = VIR_SNAP_STATE_SHUTOFF; virUUIDFormat(dom->uuid, uuidstr); memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); -- 2.20.1

On Mon, Mar 04, 2019 at 09:34:29PM -0600, Eric Blake wrote: As Eric Blake pointed out in an earlier iteration: s/qemu/snapshots/ VIR_DOMAIN_SNAPSHOT_STATE would be the least surprising prefix Jano

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(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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/7/19 8:02 AM, John Ferlan wrote: 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). Pre-existing, and habitual, but I'll clean up the ones you or I notice. 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

On 3/4/19 10:34 PM, Eric Blake wrote: Could just be return -1 Unnecessary / VIR_AUTOFREE Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

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(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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 3/4/19 10:34 PM, Eric Blake wrote: s/, for/ for/ s/understand/support/ (remove comma)... New paragraph makes it easier to read... ? s/list of snapshots/list of all snapshots/ ? s/, and/ and/ s/ can/can (extra space) Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

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(a)redhat.com&gt; --- 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

qemuDomainSnapshotWriteMetadata does not modify the directory name, and making it const-correct aids in writing an upcoming patch. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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