On 25.09.2013 21:15, Cole Robinson wrote: s/$/ "%s",/ ACK with this squashed in: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c51c7ca..87a87cc 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -214,7 +214,7 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, } if (nsdata->transient && nsdata->runstate == VIR_DOMAIN_SHUTOFF) { - virReportError(VIR_ERR_XML_ERROR, + virReportError(VIR_ERR_XML_ERROR, "%s", _("transient domain cannot have runstate 'shutoff'")); goto error; } Michal P.S. I have a dream that one day we'll document this XML passed to test driver.

Also add a <test:hasmanagedsave> element to set this data when starting the connection. --- v3: Mandate managedsave have runstate=shutoff src/test/test_driver.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++- tests/virshtest.c | 2 +- 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c51c7ca..7e60716 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -160,6 +160,7 @@ typedef testDomainNamespaceDef *testDomainNamespaceDefPtr; struct _testDomainNamespaceDef { int runstate; bool transient; + bool hasManagedSave; }; static void @@ -197,6 +198,13 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, } nsdata->transient = tmp; + tmp = virXPathBoolean("boolean(./test:hasmanagedsave)", ctxt); + if (tmp == -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid hasmanagedsave")); + goto error; + } + nsdata->hasManagedSave = tmp; + tmp = virXPathUInt("string(./test:runstate)", ctxt, &tmpuint); if (tmp == 0) { if (tmpuint >= VIR_DOMAIN_LAST) { @@ -218,6 +226,11 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, _("transient domain cannot have runstate 'shutoff'")); goto error; } + if (nsdata->hasManagedSave && nsdata->runstate != VIR_DOMAIN_SHUTOFF) { + virReportError(VIR_ERR_XML_ERROR, + _("domain with managedsave data can only have runstate 'shutoff'")); + goto error; + } *data = nsdata; return 0; @@ -588,6 +601,7 @@ testDomainStartState(testConnPtr privconn, goto cleanup; } + dom->hasManagedSave = false; ret = 0; cleanup: if (ret < 0) @@ -937,6 +951,7 @@ testParseDomains(testConnPtr privconn, nsdata = def->namespaceData; obj->persistent = !nsdata->transient; + obj->hasManagedSave = nsdata->hasManagedSave; if (nsdata->runstate != VIR_DOMAIN_SHUTOFF) { if (testDomainStartState(privconn, obj, @@ -2825,7 +2840,7 @@ static int testDomainUndefineFlags(virDomainPtr domain, virDomainEventPtr event = NULL; int ret = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); testDriverLock(privconn); privdom = virDomainObjListFindByName(privconn->domains, @@ -2836,6 +2851,15 @@ static int testDomainUndefineFlags(virDomainPtr domain, goto cleanup; } + if (privdom->hasManagedSave && + !(flags & VIR_DOMAIN_UNDEFINE_MANAGED_SAVE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Refusing to undefine while domain managed " + "save image exists")); + goto cleanup; + } + privdom->hasManagedSave = false; + event = virDomainEventNewFromObj(privdom, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); @@ -5969,6 +5993,111 @@ testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, return cpuGetModels(arch, models); } +static int +testDomainManagedSave(virDomainPtr dom, unsigned int flags) +{ + testConnPtr privconn = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainEventPtr event = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | + VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); + + testDriverLock(privconn); + vm = virDomainObjListFindByName(privconn->domains, dom->name); + testDriverUnlock(privconn); + + if (vm == NULL) { + virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto cleanup; + } + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (!vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do managed save for transient domain")); + goto cleanup; + } + + testDomainShutdownState(dom, vm, VIR_DOMAIN_SHUTOFF_SAVED); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SAVED); + vm->hasManagedSave = true; + + ret = 0; +cleanup: + if (vm) + virObjectUnlock(vm); + if (event) { + testDriverLock(privconn); + testDomainEventQueue(privconn, event); + testDriverUnlock(privconn); + } + + return ret; +} + + +static int +testDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) +{ + testConnPtr privconn = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + virCheckFlags(0, -1); + + testDriverLock(privconn); + + vm = virDomainObjListFindByName(privconn->domains, dom->name); + if (vm == NULL) { + virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto cleanup; + } + + ret = vm->hasManagedSave; +cleanup: + if (vm) + virObjectUnlock(vm); + testDriverUnlock(privconn); + return ret; +} + +static int +testDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) +{ + testConnPtr privconn = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + virCheckFlags(0, -1); + + testDriverLock(privconn); + + vm = virDomainObjListFindByName(privconn->domains, dom->name); + if (vm == NULL) { + virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto cleanup; + } + + vm->hasManagedSave = false; + ret = 0; +cleanup: + if (vm) + virObjectUnlock(vm); + testDriverUnlock(privconn); + return ret; +} + + static virDriver testDriver = { .no = VIR_DRV_TEST, .name = "Test", @@ -6042,6 +6171,9 @@ static virDriver testDriver = { .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */ .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */ .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */ + .domainManagedSave = testDomainManagedSave, /* 1.1.3 */ + .domainHasManagedSaveImage = testDomainHasManagedSaveImage, /* 1.1.3 */ + .domainManagedSaveRemove = testDomainManagedSaveRemove, /* 1.1.3 */ }; static virNetworkDriver testNetworkDriver = { diff --git a/tests/virshtest.c b/tests/virshtest.c index ca35bb0..fe255d3 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -34,7 +34,7 @@ Max memory: 261072 KiB\n\ Used memory: 131072 KiB\n\ Persistent: yes\n\ Autostart: disable\n\ -Managed save: unknown\n\ +Managed save: no\n\ \n"; static const char *domuuid_fc4 = DOM_UUID "\n\n"; static const char *domid_fc4 = "2\n\n"; -- 1.8.3.1

On 09/26/2013 10:44 AM, Michal Privoznik wrote: Thanks, pushed with that bit squashed in, and the above mentioned assignment moved after the UNDEFINE event. - Cole

On 25.09.2013 21:15, Cole Robinson wrote: This line is NOP. But we have the very same line in qemu driver too. So please remove this and post patch that cleanups *DomObjFromDomain() functions (quick look into libxl and lxc drivers shows these functions can be simplified a bit). Once we have this function a follow up patch would be nice - turn all these testDriverLock() virDomainObjListFindByName() testDriverUnlock() into testDomObjFromDomain(). Yup, these are true copy of qemu functions. Unfortunately, we are in the freeze, so you'll need to update these to 1.1.4 and push after the release. ACK Michal

Again stolen from qemu_driver.c, but dropping all the unneeded bits. This aims to copy all the current qemu validation checks since that's the most commonly used real driver, but some of the checks are completely artificial in the test driver. This only supports creation of internal snapshots for initial simplicity. --- v3: Use STRNEQ_NULLABLE for domain_conf.c change src/conf/domain_conf.c | 2 +- src/test/test_driver.c | 504 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 504 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70fdafc..dbf239e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13515,7 +13515,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src, virArchToString(src->os.arch)); return false; } - if (STRNEQ(src->os.machine, dst->os.machine)) { + if (STRNEQ_NULLABLE(src->os.machine, dst->os.machine)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain OS type %s does not match source %s"), dst->os.machine, src->os.machine); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 9a39087..1b79b70 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2860,9 +2860,11 @@ static int testDomainUndefineFlags(virDomainPtr domain, testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; virDomainEventPtr event = NULL; + int nsnapshots; int ret = -1; - virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); testDriverLock(privconn); privdom = virDomainObjListFindByName(privconn->domains, @@ -2882,6 +2884,24 @@ static int testDomainUndefineFlags(virDomainPtr domain, } privdom->hasManagedSave = false; + /* Requiring an inactive VM is part of the documented API for + * UNDEFINE_SNAPSHOTS_METADATA + */ + if (!virDomainObjIsActive(privdom) && + (nsnapshots = virDomainSnapshotObjListNum(privdom->snapshots, + NULL, 0))) { + if (!(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete inactive domain with %d " + "snapshots"), + nsnapshots); + goto cleanup; + } + + /* There isn't actually anything to do, we are just emulating qemu + * behavior here. */ + } + event = virDomainEventNewFromObj(privdom, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); @@ -6475,6 +6495,485 @@ cleanup: return ret; } +static int +testDomainSnapshotAlignDisks(virDomainObjPtr vm, + virDomainSnapshotDefPtr def, + unsigned int flags) +{ + int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + int align_match = true; + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + align_match = false; + if (virDomainObjIsActive(vm)) + def->state = VIR_DOMAIN_DISK_SNAPSHOT; + else + def->state = VIR_DOMAIN_SHUTOFF; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + def->state = virDomainObjGetState(vm, NULL); + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + align_match = false; + } else { + def->state = virDomainObjGetState(vm, NULL); + def->memory = (def->state == VIR_DOMAIN_SHUTOFF ? + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); + } + + return virDomainSnapshotAlignDisks(def, align_location, align_match); +} + +static virDomainSnapshotPtr +testDomainSnapshotCreateXML(virDomainPtr domain, + const char *xmlDesc, + unsigned int flags) +{ + testConnPtr privconn = domain->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainSnapshotDefPtr def = NULL; + virDomainSnapshotObjPtr snap = NULL; + virDomainSnapshotPtr snapshot = NULL; + virDomainEventPtr event = NULL; + char *xml = NULL; + unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + + /* + * REDEFINE + CURRENT: Not implemented yet + * DISK_ONLY: Not implemented yet + * REUSE_EXT: Not implemented yet + * + * NO_METADATA: Explicitly not implemented + * + * HALT: Implemented + * QUIESCE: Nothing to do + * ATOMIC: Nothing to do + * LIVE: Nothing to do + */ + virCheckFlags( + VIR_DOMAIN_SNAPSHOT_CREATE_HALT | + VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | + VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + + if (!(vm = testDomObjFromDomain(domain))) + goto cleanup; + + testDriverLock(privconn); + + if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot halt after transient domain snapshot")); + goto cleanup; + } + + if (!(def = virDomainSnapshotDefParseString(xmlDesc, + privconn->caps, + privconn->xmlopt, + 1 << VIR_DOMAIN_VIRT_TEST, + parse_flags))) + goto cleanup; + + if (!(xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_INACTIVE)) || + !(def->dom = virDomainDefParseString(xml, + privconn->caps, + privconn->xmlopt, + 1 << VIR_DOMAIN_VIRT_TEST, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (testDomainSnapshotAlignDisks(vm, def, flags) < 0) + goto cleanup; + + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + goto cleanup; + def = NULL; + + if (vm->current_snapshot) { + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + VIR_STRDUP(snap->def->parent, vm->current_snapshot->def->name) < 0) + goto cleanup; + } + + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) && + virDomainObjIsActive(vm)) { + testDomainShutdownState(domain, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + } + + + snapshot = virGetDomainSnapshot(domain, snap->def->name); +cleanup: + VIR_FREE(xml); + if (vm) { + if (snapshot) { + virDomainSnapshotObjPtr other; + vm->current_snapshot = snap; + other = virDomainSnapshotFindByName(vm->snapshots, + snap->def->parent); + snap->parent = other; + other->nchildren++; + snap->sibling = other->first_child; + other->first_child = snap; + } + virObjectUnlock(vm); + } + if (event) { + testDomainEventQueue(privconn, event); + } + testDriverUnlock(privconn); + virDomainSnapshotDefFree(def); + return snapshot; +} + + +typedef struct _testSnapRemoveData testSnapRemoveData; +typedef testSnapRemoveData *testSnapRemoveDataPtr; +struct _testSnapRemoveData { + virDomainObjPtr vm; + bool current; +}; + +static void +testDomainSnapshotDiscard(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr snap = payload; + testSnapRemoveDataPtr curr = data; + + if (snap->def->current) + curr->current = true; + virDomainSnapshotObjListRemove(curr->vm->snapshots, snap); +} + +typedef struct _testSnapReparentData testSnapReparentData; +typedef testSnapReparentData *testSnapReparentDataPtr; +struct _testSnapReparentData { + virDomainSnapshotObjPtr parent; + virDomainObjPtr vm; + int err; + virDomainSnapshotObjPtr last; +}; + +static void +testDomainSnapshotReparentChildren(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr snap = payload; + testSnapReparentDataPtr rep = data; + + if (rep->err < 0) { + return; + } + + VIR_FREE(snap->def->parent); + snap->parent = rep->parent; + + if (rep->parent->def && + VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) { + rep->err = -1; + return; + } + + if (!snap->sibling) + rep->last = snap; +} + +static int +testDomainSnapshotDelete(virDomainSnapshotPtr snapshot, + unsigned int flags) +{ + virDomainObjPtr vm = NULL; + virDomainSnapshotObjPtr snap = NULL; + virDomainSnapshotObjPtr parentsnap = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, -1); + + if (!(vm = testDomObjFromSnapshot(snapshot))) + return -1; + + if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) + goto cleanup; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + testSnapRemoveData rem; + rem.vm = vm; + rem.current = false; + virDomainSnapshotForEachDescendant(snap, + testDomainSnapshotDiscard, + &rem); + if (rem.current) { + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + snap->def->current = true; + } + vm->current_snapshot = snap; + } + } else if (snap->nchildren) { + testSnapReparentData rep; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.last = NULL; + virDomainSnapshotForEachChild(snap, + testDomainSnapshotReparentChildren, + &rep); + if (rep.err < 0) + goto cleanup; + + /* Can't modify siblings during ForEachChild, so do it now. */ + snap->parent->nchildren += snap->nchildren; + rep.last->sibling = snap->parent->first_child; + snap->parent->first_child = snap->first_child; + } + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + snap->nchildren = 0; + snap->first_child = NULL; + } else { + virDomainSnapshotDropParent(snap); + if (snap == vm->current_snapshot) { + if (snap->def->parent) { + parentsnap = virDomainSnapshotFindByName(vm->snapshots, + snap->def->parent); + if (!parentsnap) { + VIR_WARN("missing parent snapshot matching name '%s'", + snap->def->parent); + } else { + parentsnap->def->current = true; + } + } + vm->current_snapshot = parentsnap; + } + virDomainSnapshotObjListRemove(vm->snapshots, snap); + } + + ret = 0; +cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int +testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, + unsigned int flags) +{ + testConnPtr privconn = snapshot->domain->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainSnapshotObjPtr snap = NULL; + virDomainEventPtr event = NULL; + virDomainEventPtr event2 = NULL; + virDomainDefPtr config = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | + VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1); + + /* We have the following transitions, which create the following events: + * 1. inactive -> inactive: none + * 2. inactive -> running: EVENT_STARTED + * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED + * 4. running -> inactive: EVENT_STOPPED + * 5. running -> running: none + * 6. running -> paused: EVENT_PAUSED + * 7. paused -> inactive: EVENT_STOPPED + * 8. paused -> running: EVENT_RESUMED + * 9. paused -> paused: none + * Also, several transitions occur even if we fail partway through, + * and use of FORCE can cause multiple transitions. + */ + + if (!(vm = testDomObjFromSnapshot(snapshot))) + return -1; + + if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) + goto cleanup; + + testDriverLock(privconn); + + if (!vm->persistent && + snap->def->state != VIR_DOMAIN_RUNNING && + snap->def->state != VIR_DOMAIN_PAUSED && + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domain needs to request run or pause " + "to revert to inactive snapshot")); + goto cleanup; + } + + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { + if (!snap->def->dom) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, + _("snapshot '%s' lacks domain '%s' rollback info"), + snap->def->name, vm->def->name); + goto cleanup; + } + if (virDomainObjIsActive(vm) && + !(snap->def->state == VIR_DOMAIN_RUNNING + || snap->def->state == VIR_DOMAIN_PAUSED) && + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", + _("must respawn guest to start inactive snapshot")); + goto cleanup; + } + } + + + if (vm->current_snapshot) { + vm->current_snapshot->def->current = false; + vm->current_snapshot = NULL; + } + + snap->def->current = true; + config = virDomainDefCopy(snap->def->dom, + privconn->caps, privconn->xmlopt, true); + if (!config) + goto cleanup; + + if (snap->def->state == VIR_DOMAIN_RUNNING || + snap->def->state == VIR_DOMAIN_PAUSED) { + /* Transitions 2, 3, 5, 6, 8, 9 */ + bool was_running = false; + bool was_stopped = false; + + if (virDomainObjIsActive(vm)) { + /* Transitions 5, 6, 8, 9 */ + /* Check for ABI compatibility. */ + if (!virDomainDefCheckABIStability(vm->def, config)) { + virErrorPtr err = virGetLastError(); + + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { + /* Re-spawn error using correct category. */ + if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", + err->str2); + goto cleanup; + } + + virResetError(err); + testDomainShutdownState(snapshot->domain, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + if (event) + testDomainEventQueue(privconn, event); + goto load; + } + + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + /* Transitions 5, 6 */ + was_running = true; + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + /* Create an event now in case the restore fails, so + * that user will be alerted that they are now paused. + * If restore later succeeds, we might replace this. */ + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); + } + virDomainObjAssignDef(vm, config, false, NULL); + + } else { + /* Transitions 2, 3 */ + load: + was_stopped = true; + virDomainObjAssignDef(vm, config, false, NULL); + if (testDomainStartState(privconn, vm, + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT) < 0) + goto cleanup; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + } + + /* Touch up domain state. */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && + (snap->def->state == VIR_DOMAIN_PAUSED || + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { + /* Transitions 3, 6, 9 */ + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + if (was_stopped) { + /* Transition 3, use event as-is and add event2 */ + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); + } /* else transition 6 and 9 use event as-is */ + } else { + /* Transitions 2, 5, 8 */ + virDomainEventFree(event); + event = NULL; + + if (was_stopped) { + /* Transition 2 */ + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + } else if (was_running) { + /* Transition 8 */ + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_RESUMED, + VIR_DOMAIN_EVENT_RESUMED); + } + } + } else { + /* Transitions 1, 4, 7 */ + virDomainObjAssignDef(vm, config, false, NULL); + + if (virDomainObjIsActive(vm)) { + /* Transitions 4, 7 */ + testDomainShutdownState(snapshot->domain, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + } + + if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + /* Flush first event, now do transition 2 or 3 */ + bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; + + if (event) + testDomainEventQueue(privconn, event); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + if (paused) { + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); + } + } + } + + vm->current_snapshot = snap; + ret = 0; +cleanup: + if (event) { + testDomainEventQueue(privconn, event); + if (event2) + testDomainEventQueue(privconn, event2); + } + virObjectUnlock(vm); + testDriverUnlock(privconn); + + return ret; +} + + static virDriver testDriver = { .no = VIR_DRV_TEST, @@ -6566,6 +7065,9 @@ static virDriver testDriver = { .domainSnapshotCurrent = testDomainSnapshotCurrent, /* 1.1.3 */ .domainSnapshotIsCurrent = testDomainSnapshotIsCurrent, /* 1.1.3 */ .domainSnapshotHasMetadata = testDomainSnapshotHasMetadata, /* 1.1.3 */ + .domainSnapshotCreateXML = testDomainSnapshotCreateXML, /* 1.1.3 */ + .domainRevertToSnapshot = testDomainRevertToSnapshot, /* 1.1.3 */ + .domainSnapshotDelete = testDomainSnapshotDelete, /* 1.1.3 */ }; static virNetworkDriver testNetworkDriver = { -- 1.8.3.1

On 10/04/2013 07:05 AM, John Ferlan wrote: Hmm, the logic in this code is pretty intense :) In the followup patch I've added: @@ -7078,6 +7076,8 @@ cleanup: testDomainEventQueue(privconn, event); if (event2) testDomainEventQueue(privconn, event2); + } else { + virDomainEventFree(event2); } virObjectUnlock(vm); testDriverUnlock(privconn); Hopefully that covers it. Thanks, Cole

On 25.09.2013 21:15, Cole Robinson wrote: ACK. While this is safe for 1.1.3 I'd rather wait and push it after the freeze. You know, the freeze is supposed to be for bug fixing and not code refactoring. Moreover, if you'd move this somewhere at the beginning of you set, the test driver can use the more readable code too. Michal (This is the last patch I've time to review today, I'll continue tomorrow)

--- src/conf/snapshot_conf.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 7 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 131 +---------------------------------------- 4 files changed, 160 insertions(+), 129 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 207a8fe..1fcc4cb 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1099,3 +1099,153 @@ virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap) { return virDomainSnapshotDefIsExternal(snap->def); } + +int +virDomainSnapshotRedefinePrep(virDomainPtr domain, + virDomainObjPtr vm, + virDomainSnapshotDefPtr *defptr, + virDomainSnapshotObjPtr *snap, + bool *update_current, + unsigned int flags) +{ + virDomainSnapshotDefPtr def = *defptr; + int ret = -1; + int align_location; + int align_match; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainSnapshotObjPtr other; + + virUUIDFormat(domain->uuid, uuidstr); + + /* Prevent circular chains */ + if (def->parent) { + if (STREQ(def->name, def->parent)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot set snapshot %s as its own parent"), + def->name); + goto cleanup; + } + 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; + } + 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; + } + other = virDomainSnapshotFindByName(vm->snapshots, + other->def->parent); + if (!other) { + VIR_WARN("snapshots are inconsistent for %s", + vm->def->name); + break; + } + } + } + + /* Check that any replacement is compatible */ + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && + def->state != VIR_DOMAIN_DISK_SNAPSHOT) { + 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)) { + virReportError(VIR_ERR_INVALID_ARG, + _("definition for snapshot %s must use uuid %s"), + def->name, uuidstr); + goto cleanup; + } + + 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)) { + 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_DOMAIN_DISK_SNAPSHOT) != + (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot change between disk snapshot and " + "system checkpoint in snapshot %s"), + def->name); + goto cleanup; + } + + if (other->def->dom) { + if (def->dom) { + if (!virDomainDefCheckABIStability(other->def->dom, + def->dom)) + goto cleanup; + } else { + /* Transfer the domain def */ + def->dom = other->def->dom; + other->def->dom = NULL; + } + } + + if (def->dom) { + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || + virDomainSnapshotDefIsExternal(def)) { + 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; + } + + /* Drop and rebuild the parent relationship, but keep all + * child relations by reusing snap. */ + virDomainSnapshotDropParent(other); + virDomainSnapshotDefFree(other->def); + other->def = def; + *defptr = NULL; + *snap = other; + } else { + if (def->dom) { + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_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; +} diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 0b8d18a..e8f8179 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -176,6 +176,13 @@ int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, bool virDomainSnapshotDefIsExternal(virDomainSnapshotDefPtr def); bool virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap); +int virDomainSnapshotRedefinePrep(virDomainPtr domain, + virDomainObjPtr vm, + virDomainSnapshotDefPtr *def, + virDomainSnapshotObjPtr *snap, + bool *update_current, + unsigned int flags); + VIR_ENUM_DECL(virDomainSnapshotLocation) VIR_ENUM_DECL(virDomainSnapshotState) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 48e4b04..e999c25 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -652,6 +652,7 @@ virDomainSnapshotLocationTypeToString; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNum; virDomainSnapshotObjListRemove; +virDomainSnapshotRedefinePrep; virDomainSnapshotStateTypeFromString; virDomainSnapshotStateTypeToString; virDomainSnapshotUpdateRelations; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0dc975b..2ebcb35 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12270,7 +12270,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, char *xml = NULL; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; bool update_current = true; bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; @@ -12304,8 +12303,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (redefine) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; - virUUIDFormat(domain->uuid, uuidstr); - if (!(vm = qemuDomObjFromDomain(domain))) goto cleanup; @@ -12374,133 +12371,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } if (redefine) { - /* Prevent circular chains */ - if (def->parent) { - if (STREQ(def->name, def->parent)) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot set snapshot %s as its own parent"), - def->name); - goto cleanup; - } - 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; - } - 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; - } - other = virDomainSnapshotFindByName(vm->snapshots, - other->def->parent); - if (!other) { - VIR_WARN("snapshots are inconsistent for %s", - vm->def->name); - break; - } - } - } - - /* Check that any replacement is compatible */ - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && - def->state != VIR_DOMAIN_DISK_SNAPSHOT) { - 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)) { - virReportError(VIR_ERR_INVALID_ARG, - _("definition for snapshot %s must use uuid %s"), - def->name, uuidstr); + if (!virDomainSnapshotRedefinePrep(domain, vm, &def, &snap, + &update_current, flags) < 0) goto cleanup; - } - - 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)) { - 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_DOMAIN_DISK_SNAPSHOT) != - (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot change between disk snapshot and " - "system checkpoint in snapshot %s"), - def->name); - goto cleanup; - } - - if (other->def->dom) { - if (def->dom) { - if (!virDomainDefCheckABIStability(other->def->dom, - def->dom)) - goto cleanup; - } else { - /* Transfer the domain def */ - def->dom = other->def->dom; - other->def->dom = NULL; - } - } - - if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || - virDomainSnapshotDefIsExternal(def)) { - 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; - } - - /* Drop and rebuild the parent relationship, but keep all - * child relations by reusing snap. */ - virDomainSnapshotDropParent(other); - virDomainSnapshotDefFree(other->def); - other->def = def; - def = NULL; - snap = other; - } else { - if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || - def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { - align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; - } - if (virDomainSnapshotAlignDisks(def, align_location, - align_match) < 0) - goto cleanup; - } - } } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ -- 1.8.3.1

On 09/27/2013 02:13 AM, Michal Privoznik wrote: I restored the default from qemu_driver.c and pushed now. Thanks, Cole

On 25.09.2013 21:15, Cole Robinson wrote: Oh, so you'd done what I suggested in 5/8. Nice. This fixes the bug I'm mentioning in 4/8. ACK Michal

On 25.09.2013 21:15, Cole Robinson wrote: This line ^^^ is not needed. VIR_ALLOC (not the one in this context, but a few lines above it) fills allocated memory with zeros. So num_snap_nodes is zero at this point. For sure. ACK Michal