[libvirt] [PATCH RFC] qemu: add some synchronizations for snapshot

Thursday, 28 August 2014

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


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH RFC] qemu: add some synchronizations for snapshot