Currently it lacks synchronization to modify domain's snapshot object list, that race condition causes unsafe access to some freed snapshot objects. Therefore, this patch wraps all access of snapshot object list in vm job lock. Only the qemuDomainSnapshotCreateXML is not synchronized, it is related to QEMU_ASYNC_JOB_SNAPSHOT async job for --disk-only snapshot. I am not sure if it's ok to remove it, so need your ideas for qemuDomainSnapshotCreateXML. Signed-off-by: Jincheng Miao <jmiao(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 137 +++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 112 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 73959da..7329aa9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13609,6 +13609,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, { virDomainObjPtr vm = NULL; int n = -1; + virQEMUDriverPtr driver = domain->conn->privateData; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); @@ -13619,8 +13620,12 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, if (virDomainSnapshotListNamesEnsureACL(domain->conn, vm->def) < 0) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen, flags); + ignore_value(qemuDomainObjEndJob(driver, vm)); cleanup: if (vm) @@ -13633,6 +13638,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, { virDomainObjPtr vm = NULL; int n = -1; + virQEMUDriverPtr driver = domain->conn->privateData; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); @@ -13643,8 +13649,13 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, if (virDomainSnapshotNumEnsureACL(domain->conn, vm->def) < 0) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + n = virDomainSnapshotObjListNum(vm->snapshots, NULL, flags); + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13657,6 +13668,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, { virDomainObjPtr vm = NULL; int n = -1; + virQEMUDriverPtr driver = domain->conn->privateData; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); @@ -13667,8 +13679,13 @@ qemuDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, if (virDomainListAllSnapshotsEnsureACL(domain->conn, vm->def) < 0) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + n = virDomainListSnapshots(vm->snapshots, NULL, domain, snaps, flags); + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13684,6 +13701,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, virDomainObjPtr vm = NULL; virDomainSnapshotObjPtr snap = NULL; int n = -1; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); @@ -13694,12 +13712,18 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, if (virDomainSnapshotListChildrenNamesEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + n = virDomainSnapshotObjListGetNames(vm->snapshots, snap, names, nameslen, flags); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13713,6 +13737,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, virDomainObjPtr vm = NULL; virDomainSnapshotObjPtr snap = NULL; int n = -1; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); @@ -13723,11 +13748,17 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, if (virDomainSnapshotNumChildrenEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + n = virDomainSnapshotObjListNum(vm->snapshots, snap, flags); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13742,6 +13773,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, virDomainObjPtr vm = NULL; virDomainSnapshotObjPtr snap = NULL; int n = -1; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); @@ -13752,12 +13784,18 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, if (virDomainSnapshotListAllChildrenEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + n = virDomainListSnapshots(vm->snapshots, snap, snapshot->domain, snaps, flags); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13771,6 +13809,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, virDomainObjPtr vm; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; + virQEMUDriverPtr driver = domain->conn->privateData; virCheckFlags(0, NULL); @@ -13780,11 +13819,17 @@ static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, if (virDomainSnapshotLookupByNameEnsureACL(domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromName(vm, name))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromName(vm, name))) + goto endjob; + snapshot = virGetDomainSnapshot(domain, snap->def->name); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13796,6 +13841,7 @@ static int qemuDomainHasCurrentSnapshot(virDomainPtr domain, { virDomainObjPtr vm; int ret = -1; + virQEMUDriverPtr driver = domain->conn->privateData; virCheckFlags(0, -1); @@ -13805,8 +13851,13 @@ static int qemuDomainHasCurrentSnapshot(virDomainPtr domain, if (virDomainHasCurrentSnapshotEnsureACL(domain->conn, vm->def) < 0) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + ret = (vm->current_snapshot != NULL); + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13820,6 +13871,7 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, virDomainObjPtr vm; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr parent = NULL; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(0, NULL); @@ -13829,18 +13881,24 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, if (virDomainSnapshotGetParentEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + if (!snap->def->parent) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("snapshot '%s' does not have a parent"), snap->def->name); - goto cleanup; + goto endjob; } parent = virGetDomainSnapshot(snapshot->domain, snap->def->parent); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13852,6 +13910,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCurrent(virDomainPtr domain, { virDomainObjPtr vm; virDomainSnapshotPtr snapshot = NULL; + virQEMUDriverPtr driver = domain->conn->privateData; virCheckFlags(0, NULL); @@ -13861,14 +13920,20 @@ static virDomainSnapshotPtr qemuDomainSnapshotCurrent(virDomainPtr domain, if (virDomainSnapshotCurrentEnsureACL(domain->conn, vm->def) < 0) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + if (!vm->current_snapshot) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, "%s", _("the domain does not have a current snapshot")); - goto cleanup; + goto endjob; } snapshot = virGetDomainSnapshot(domain, vm->current_snapshot->def->name); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13882,6 +13947,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, char *xml = NULL; virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); @@ -13891,13 +13957,19 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, if (virDomainSnapshotGetXMLDescEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + virUUIDFormat(snapshot->domain->uuid, uuidstr); xml = virDomainSnapshotDefFormat(uuidstr, snap->def, flags, 0); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13911,6 +13983,7 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, virDomainObjPtr vm = NULL; int ret = -1; virDomainSnapshotObjPtr snap = NULL; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(0, -1); @@ -13920,12 +13993,18 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, if (virDomainSnapshotIsCurrentEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + ret = (vm->current_snapshot && STREQ(snapshot->name, vm->current_snapshot->def->name)); + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -13940,6 +14019,7 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, virDomainObjPtr vm = NULL; int ret = -1; virDomainSnapshotObjPtr snap = NULL; + virQEMUDriverPtr driver = snapshot->domain->conn->privateData; virCheckFlags(0, -1); @@ -13949,14 +14029,20 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, if (virDomainSnapshotHasMetadataEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + /* XXX Someday, we should recognize internal snapshots in qcow2 * images that are not tied to a libvirt snapshot; if we ever do * that, then we would have a reason to return 0 here. */ ret = 1; + endjob: + ignore_value(qemuDomainObjEndJob(driver, vm)); + cleanup: if (vm) virObjectUnlock(vm); @@ -14027,9 +14113,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + if (!vm->persistent && snap->def->state != VIR_DOMAIN_RUNNING && snap->def->state != VIR_DOMAIN_PAUSED && @@ -14038,13 +14127,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("transient domain needs to request run or pause " "to revert to inactive snapshot")); - goto cleanup; + goto endjob; } if (virDomainSnapshotIsExternal(snap)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("revert to external snapshot not supported yet")); - goto cleanup; + goto endjob; } if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { @@ -14052,7 +14141,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, _("snapshot '%s' lacks domain '%s' rollback info"), snap->def->name, vm->def->name); - goto cleanup; + goto endjob; } if (virDomainObjIsActive(vm) && !(snap->def->state == VIR_DOMAIN_RUNNING @@ -14061,7 +14150,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", _("must respawn qemu to start inactive snapshot")); - goto cleanup; + goto endjob; } } @@ -14070,7 +14159,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, cfg->snapshotDir) < 0) - goto cleanup; + goto endjob; vm->current_snapshot = NULL; /* XXX Should we restore vm->current_snapshot after this point * in the failure cases where we know there was no change? */ @@ -14085,12 +14174,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snap->def->dom) { config = virDomainDefCopy(snap->def->dom, caps, driver->xmlopt, true); if (!config) - goto cleanup; + goto endjob; } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - switch ((virDomainState) snap->def->state) { case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_PAUSED: @@ -14400,9 +14486,13 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (virDomainSnapshotDeleteEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + if (!metadata_only) { if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && virDomainSnapshotIsExternal(snap)) @@ -14415,13 +14505,10 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("deletion of %d external disk snapshots not " "supported yet"), external); - goto cleanup; + goto endjob; } } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { rem.driver = driver; -- 1.7.1