On 09/26/2013 10:45 AM, Michal Privoznik wrote:

On 25.09.2013 21:15, Cole Robinson wrote:

...

Similar to the runstate commit, allow a boolean <test:transient/> element for setting domain persistence at driver startup. --- v3: Validate that transient is not specified with runstate=shutoff

src/test/test_driver.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 48b5ec1..c51c7ca 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -159,6 +159,7 @@ typedef struct _testDomainNamespaceDef testDomainNamespaceDef; typedef testDomainNamespaceDef *testDomainNamespaceDefPtr; struct _testDomainNamespaceDef { int runstate; + bool transient; };

static void @@ -189,6 +190,13 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, if (VIR_ALLOC(nsdata) < 0) return -1;

+ tmp = virXPathBoolean("boolean(./test:transient)", ctxt); + if (tmp == -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid transient")); + goto error; + } + nsdata->transient = tmp; + tmp = virXPathUInt("string(./test:runstate)", ctxt, &tmpuint); if (tmp == 0) { if (tmpuint >= VIR_DOMAIN_LAST) { @@ -205,6 +213,12 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, goto error; }

+ if (nsdata->transient && nsdata->runstate == VIR_DOMAIN_SHUTOFF) { + virReportError(VIR_ERR_XML_ERROR,

s/$/ "%s",/

...

+ _("transient domain cannot have runstate 'shutoff'")); + goto error; + } + *data = nsdata; return 0;

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; }

Thanks, pushed with that bit fixed. - Cole

On 25.09.2013 21:15, Cole Robinson wrote:

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,

s/$/ "%s",/

+ _("domain with managedsave data can only have runstate 'shutoff'")); + goto error; + }

@@ -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;

While this assignment is safe for now (there's not goto cleanup afterwards) I'd rather see it more closer to the ret = 0; line. What if in the future we insert something in between?

+ 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;

This ^^^ is what I have in my mind.

+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; +}

The driver can be unlocked right after virDomainObjListFindByName(). But who cares about throughput in test driver, right? ACK if you squash this in: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 77c3d23..f40a841 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -227,7 +227,7 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, goto error; } if (nsdata->hasManagedSave && nsdata->runstate != VIR_DOMAIN_SHUTOFF) { - virReportError(VIR_ERR_XML_ERROR, + virReportError(VIR_ERR_XML_ERROR, "%s", _("domain with managedsave data can only have runstate 'shutoff'")); goto error; } Michal

On 09/26/2013 10:44 AM, Michal Privoznik wrote:

On 25.09.2013 21:15, Cole Robinson wrote:

...

This is just stolen from qemu_driver.c with tweaks to fit the test driver. --- src/test/test_driver.c | 392 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 392 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7e60716..9a39087 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -45,6 +45,7 @@ #include "interface_conf.h" #include "domain_conf.h" #include "domain_event.h" +#include "snapshot_conf.h" #include "fdstream.h" #include "storage_conf.h" #include "node_device_conf.h" @@ -423,6 +424,27 @@ static const unsigned long long defaultPoolAlloc = 0; static int testStoragePoolObjSetDefaults(virStoragePoolObjPtr pool); static int testNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);

+static virDomainObjPtr +testDomObjFromDomain(virDomainPtr domain) +{ + virDomainObjPtr vm; + testConnPtr driver = domain->conn->privateData; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + testDriverLock(driver); + vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); + if (!vm) { + virUUIDFormat(domain->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, domain->name); + vm = NULL;

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).

I dropped it from this patch, but the qemu driver isn't affected anymore since that code was recently altered.

...

+ } + + testDriverUnlock(driver); + return vm; +}

Once we have this function a follow up patch would be nice - turn all these

testDriverLock() virDomainObjListFindByName() testDriverUnlock()

into testDomObjFromDomain().

Yeah there's definitely a lot of opportunity for such cleanups in the test driver.

...

+ static char * testDomainGenerateIfname(virDomainDefPtr domdef) { int maxif = 1024; @@ -6098,6 +6120,362 @@ cleanup: }

+/* + * Snapshot APIs + */ +

Yup, these are true copy of qemu functions.

...

static virDriver testDriver = { .no = VIR_DRV_TEST, .name = "Test", @@ -6174,6 +6552,20 @@ static virDriver testDriver = { .domainManagedSave = testDomainManagedSave, /* 1.1.3 */ .domainHasManagedSaveImage = testDomainHasManagedSaveImage, /* 1.1.3 */ .domainManagedSaveRemove = testDomainManagedSaveRemove, /* 1.1.3 */ + + .domainSnapshotNum = testDomainSnapshotNum, /* 1.1.3 */ + .domainSnapshotListNames = testDomainSnapshotListNames, /* 1.1.3 */ + .domainListAllSnapshots = testDomainListAllSnapshots, /* 1.1.3 */ + .domainSnapshotGetXMLDesc = testDomainSnapshotGetXMLDesc, /* 1.1.3 */ + .domainSnapshotNumChildren = testDomainSnapshotNumChildren, /* 1.1.3 */ + .domainSnapshotListChildrenNames = testDomainSnapshotListChildrenNames, /* 1.1.3 */ + .domainSnapshotListAllChildren = testDomainSnapshotListAllChildren, /* 1.1.3 */ + .domainSnapshotLookupByName = testDomainSnapshotLookupByName, /* 1.1.3 */ + .domainHasCurrentSnapshot = testDomainHasCurrentSnapshot, /* 1.1.3 */ + .domainSnapshotGetParent = testDomainSnapshotGetParent, /* 1.1.3 */ + .domainSnapshotCurrent = testDomainSnapshotCurrent, /* 1.1.3 */ + .domainSnapshotIsCurrent = testDomainSnapshotIsCurrent, /* 1.1.3 */ + .domainSnapshotHasMetadata = testDomainSnapshotHasMetadata, /* 1.1.3 */ };

static virNetworkDriver testNetworkDriver = {

Unfortunately, we are in the freeze, so you'll need to update these to 1.1.4 and push after the release.

ACK

Pushed with those changes. Thanks, Cole

On 25.09.2013 21:15, Cole Robinson wrote:

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;

This is exactly the problem I'm describing in 2/8. If something below fails, we've undefined the managed save image.

+ /* 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);

Needless parentheses.

+ } + + 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);

No need to lock the driver this soon.

+ + if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot halt after transient domain snapshot")); + goto cleanup; + }

IE vm is locked here and this has no impact on the driver.

+ + if (!(def = virDomainSnapshotDefParseString(xmlDesc, + privconn->caps, + privconn->xmlopt, + 1 << VIR_DOMAIN_VIRT_TEST, + parse_flags))) + goto cleanup;

Neither has this ...

+ + 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;

.. or this .. BTW we have virDomainDefCopy().

+ + if (testDomainSnapshotAlignDisks(vm, def, flags) < 0) + goto cleanup; +

If you intended the driver lock as mutex for vm->snapshots I don't think it's necessary since vm is locked throughout the whole API. So there is no chance for other API to hop in and change the vm->snapshots.

+ if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + goto cleanup; + def = NULL;

I don't think this is gonna work. If flags has _REDEFINE flag set, the virDomainSnapshotAssignDef() will fail and the code below won't get any chance to reverse the upcoming error.

+ + 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) {

In fact this is the only place that require test driver to be locked.

+ 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)

Please keep the 'All' suffix just like the qemu counterpart has. It made me wondering why is the function header different to qemuDomainSnapshotDiscard, while I needed to look at qemuDomainSnaphostDiscardAll with the very same header.

+{ + 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,

s/Discard/DiscardAll/

+ &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);

This looks suspicious again.

+ + 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);

Shouldn't this be testDomainShutdownState() instead of virDomainObjSetState()?

+ /* 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 = {

ACK if you address the nits I've pointed out. Michal

On 09/26/2013 10:44 AM, Michal Privoznik wrote:

On 25.09.2013 21:15, Cole Robinson wrote:

...

Makes things more readable IMO --- src/qemu/qemu_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 346a8f9..0dc975b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12273,6 +12273,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; bool update_current = true; + bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; virDomainSnapshotObjPtr other = NULL; int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; @@ -12297,11 +12298,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, return NULL; }

- if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && - !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || + if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) update_current = false; - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) + if (redefine) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE;

virUUIDFormat(domain->uuid, uuidstr); @@ -12366,14 +12366,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && (!virDomainObjIsActive(vm) || def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + redefine)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live snapshot creation is supported only " "with external checkpoints")); goto cleanup; }

- if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + if (redefine) { /* Prevent circular chains */ if (def->parent) { if (STREQ(def->name, def->parent)) { @@ -12544,7 +12544,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (update_current) snap->def->current = true; if (vm->current_snapshot) { - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + if (!redefine && VIR_STRDUP(snap->def->parent, vm->current_snapshot->def->name) < 0) goto cleanup; if (update_current) { @@ -12557,7 +12557,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, }

/* actually do the snapshot */ - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + if (redefine) { /* XXX Should we validate that the redefined snapshot even * makes sense, such as checking that qemu-img recognizes the * snapshot name in at least one of the domain's disks? */

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.

Pushed now. Thanks, Cole

On 25.09.2013 21:15, Cole Robinson wrote:

--- 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(-)

Almost.

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;

These two ^^^ had a default set ...

+ 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) {

... and here you're calling them with a random value (unless the previous if evaluates true).

+ /* revert stealing of the snapshot domain definition */ + if (def->dom && !other->def->dom) { + other->def->dom = def->dom; + def->dom = NULL; + } + goto cleanup; + } + }

ACK once you fix it. Michal

On 09/27/2013 02:13 AM, Michal Privoznik wrote:

On 25.09.2013 21:15, Cole Robinson wrote:

...

--- src/test/test_driver.c | 67 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b79b70..8a690fd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6537,26 +6537,35 @@ testDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotPtr snapshot = NULL; virDomainEventPtr event = NULL; char *xml = NULL; + bool update_current = true; + bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;

Oh, so you'd done what I suggested in 5/8. Nice.

...

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 * + * REDEFINE + CURRENT: Implemented * HALT: Implemented * QUIESCE: Nothing to do * ATOMIC: Nothing to do * LIVE: Nothing to do */ virCheckFlags( + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | VIR_DOMAIN_SNAPSHOT_CREATE_HALT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);

+ if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))) + update_current = false; + if (redefine) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; + if (!(vm = testDomObjFromDomain(domain))) goto cleanup;

@@ -6575,34 +6584,43 @@ testDomainSnapshotCreateXML(virDomainPtr domain, 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 (redefine) { + if (!virDomainSnapshotRedefinePrep(domain, vm, &def, &snap, + &update_current, flags) < 0) + goto cleanup; + } else { + 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 (vm->current_snapshot) { - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && - VIR_STRDUP(snap->def->parent, vm->current_snapshot->def->name) < 0) + if (testDomainSnapshotAlignDisks(vm, def, flags) < 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); + if (!snap) { + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + goto cleanup; + def = NULL; }

This fixes the bug I'm mentioning in 4/8.

...

+ if (!redefine) { + if (vm->current_snapshot && + (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: @@ -6610,7 +6628,8 @@ cleanup: if (vm) { if (snapshot) { virDomainSnapshotObjPtr other; - vm->current_snapshot = snap; + if (update_current) + vm->current_snapshot = snap; other = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); snap->parent = other;

ACK

Thanks, pushed now. - Cole

On 09/27/2013 02:13 AM, Michal Privoznik wrote:

On 25.09.2013 21:15, Cole Robinson wrote:

...

The user can pass it as a <test:domainsnapshot> subelement of a <domain>. --- src/test/test_driver.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 8a690fd..6b11e9c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -162,12 +162,24 @@ struct _testDomainNamespaceDef { int runstate; bool transient; bool hasManagedSave; + + unsigned int num_snap_nodes; + xmlNodePtr *snap_nodes; };

static void testDomainDefNamespaceFree(void *data) { testDomainNamespaceDefPtr nsdata = data; + size_t i; + + if (!nsdata) + return; + + for (i = 0; i < nsdata->num_snap_nodes; i++) + xmlFreeNode(nsdata->snap_nodes[i]); + + VIR_FREE(nsdata->snap_nodes); VIR_FREE(nsdata); }

@@ -178,7 +190,9 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, void **data) { testDomainNamespaceDefPtr nsdata = NULL; - int tmp; + xmlNodePtr *nodes = NULL; + int tmp, n; + size_t i; unsigned int tmpuint;

if (xmlXPathRegisterNs(ctxt, BAD_CAST "test", @@ -192,6 +206,26 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, if (VIR_ALLOC(nsdata) < 0) return -1;

+ n = virXPathNodeSet("./test:domainsnapshot", ctxt, &nodes); + if (n < 0) + goto error; + + if (n && VIR_ALLOC_N(nsdata->snap_nodes, n) < 0) + goto error; + + nsdata->num_snap_nodes = 0;

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.

...

+ for (i = 0; i < n; i++) { + xmlNodePtr newnode = xmlCopyNode(nodes[i], 1); + if (!newnode) { + virReportOOMError(); + goto error; + } + + nsdata->snap_nodes[nsdata->num_snap_nodes] = newnode; + nsdata->num_snap_nodes++; + } + VIR_FREE(nodes); + tmp = virXPathBoolean("boolean(./test:transient)", ctxt); if (tmp == -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid transient")); @@ -237,6 +271,7 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, return 0;

error: + VIR_FREE(nodes); testDomainDefNamespaceFree(nsdata); return -1; } @@ -934,6 +969,63 @@ error: }

static int +testParseDomainSnapshots(testConnPtr privconn, + virDomainObjPtr domobj, + const char *file, + xmlXPathContextPtr ctxt) +{ + size_t i; + int ret = -1; + testDomainNamespaceDefPtr nsdata = domobj->def->namespaceData; + xmlNodePtr *nodes = nsdata->snap_nodes; + + for (i = 0; i < nsdata->num_snap_nodes; i++) { + virDomainSnapshotObjPtr snap; + virDomainSnapshotDefPtr def; + xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, + "domainsnapshot"); + if (!node) + goto error; + + def = virDomainSnapshotDefParseNode(ctxt->doc, node, + privconn->caps, + privconn->xmlopt, + 1 << VIR_DOMAIN_VIRT_TEST, + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE); + if (!def) + goto error; + + if (!(snap = virDomainSnapshotAssignDef(domobj->snapshots, def))) { + virDomainSnapshotDefFree(def); + goto error; + } + + if (def->current) { + if (domobj->current_snapshot) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("more than one snapshot claims to be active")); + goto error; + } + + domobj->current_snapshot = snap; + } + } + + if (virDomainSnapshotUpdateRelations(domobj->snapshots) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Snapshots have inconsistent relations for " + "domain %s"), domobj->def->name); + goto error; + } + + ret = 0; +error: + return ret; +} + +static int testParseDomains(testConnPtr privconn, const char *file, xmlXPathContextPtr ctxt) @@ -971,6 +1063,11 @@ testParseDomains(testConnPtr privconn, goto error; }

+ if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) { + virObjectUnlock(obj); + goto error; + } + nsdata = def->namespaceData; obj->persistent = !nsdata->transient; obj->hasManagedSave = nsdata->hasManagedSave;

ACK

Michal

Pushed with the suggested change. Thanks, Cole