Fix virDomainObj ref handling in QEMU driver

Since the monitor I/O is processed out of band from the main
thread(s) invoking monitor  commands, the virDomainObj may be
deleted by the I/O thread. The qemuDomainObjBeginJob takes an
extra reference to protect against final deletion, but this
reference is released by the corresponding EndJob call. THus
after the EndJob call it may not be valid to reference the
virDomainObj any more. To allow callers to detect this, the
EndJob call is changed to return the remaining reference count.

* src/conf/domain_conf.c: Make virDomainObjUnref return the
  remaining reference count
* src/qemu/qemu_driver.c: Avoid referencing virDomainObjPtr
  after qemuDomainObjEndJob if it has been deleted.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 21d509d9c893240ba44a019aa3fcb63cee7212bf..dca2e499641de10ed7b42194f30266277842a8f4 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -231,7 +231,7 @@ static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUT
 {
     virDomainObjPtr obj = payload;
     virDomainObjLock(obj);
-    if (!virDomainObjUnref(obj))
+    if (virDomainObjUnref(obj) > 0)
         virDomainObjUnlock(obj);
 }
 
@@ -637,9 +637,9 @@ int virDomainObjUnref(virDomainObjPtr dom)
     if (dom->refs == 0) {
         virDomainObjUnlock(dom);
         virDomainObjFree(dom);
-        return 1;
+        return 0;
     }
-    return 0;
+    return dom->refs;
 }
 
 static virDomainObjPtr virDomainObjNew(virConnectPtr conn,

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7dfd1e692cf1f172e32a357dd163924d279c9b25..7e60d0ebcd55dd1eb618cfe0ffd7eb1b711cd3c4 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -371,15 +371,18 @@ static int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver,
  *
  * To be called after completing the work associated with the
  * earlier  qemuDomainBeginJob() call
+ *
+ * Returns remaining refcount on 'obj', maybe 0 to indicated it
+ * was deleted
  */
-static void qemuDomainObjEndJob(virDomainObjPtr obj)
+static int ATTRIBUTE_RETURN_CHECK qemuDomainObjEndJob(virDomainObjPtr obj)
 {
     qemuDomainObjPrivatePtr priv = obj->privateData;
 
     priv->jobActive = 0;
     virCondSignal(&priv->jobCond);
 
-    virDomainObjUnref(obj);
+    return virDomainObjUnref(obj);
 }
 
 
@@ -3039,9 +3042,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
         goto cleanup; /* XXXX free the 'vm' we created ? */
 
     if (qemudStartVMDaemon(conn, driver, vm, NULL, -1) < 0) {
-        qemuDomainObjEndJob(vm);
-        virDomainRemoveInactive(&driver->domains,
-                                vm);
+        if (qemuDomainObjEndJob(vm) > 0)
+            virDomainRemoveInactive(&driver->domains,
+                                    vm);
         vm = NULL;
         goto endjob;
     }
@@ -3054,8 +3057,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
     if (dom) dom->id = vm->def->id;
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     virDomainDefFree(def);
@@ -3110,7 +3114,8 @@ static int qemudDomainSuspend(virDomainPtr dom) {
     ret = 0;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -3169,7 +3174,8 @@ static int qemudDomainResume(virDomainPtr dom) {
     ret = 0;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -3213,7 +3219,8 @@ static int qemudDomainShutdown(virDomainPtr dom) {
     qemuDomainObjExitMonitor(vm);
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -3252,16 +3259,17 @@ static int qemudDomainDestroy(virDomainPtr dom) {
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
     if (!vm->persistent) {
-        qemuDomainObjEndJob(vm);
-        virDomainRemoveInactive(&driver->domains,
-                                vm);
+        if (qemuDomainObjEndJob(vm) > 0)
+            virDomainRemoveInactive(&driver->domains,
+                                    vm);
         vm = NULL;
     }
     ret = 0;
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -3402,7 +3410,8 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
     ret = 0;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -3452,7 +3461,8 @@ static int qemudDomainGetInfo(virDomainPtr dom,
             err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
             qemuDomainObjExitMonitor(vm);
             if (err < 0) {
-                qemuDomainObjEndJob(vm);
+                if (qemuDomainObjEndJob(vm) == 0)
+                    vm = NULL;
                 goto cleanup;
             }
 
@@ -3462,7 +3472,10 @@ static int qemudDomainGetInfo(virDomainPtr dom,
             else
                 info->memory = balloon;
 
-            qemuDomainObjEndJob(vm);
+            if (qemuDomainObjEndJob(vm) == 0) {
+                vm = NULL;
+                goto cleanup;
+            }
         } else {
             info->memory = vm->def->memory;
         }
@@ -3671,15 +3684,16 @@ static int qemudDomainSave(virDomainPtr dom,
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_SAVED);
     if (!vm->persistent) {
-        qemuDomainObjEndJob(vm);
-        virDomainRemoveInactive(&driver->domains,
-                                vm);
+        if (qemuDomainObjEndJob(vm) > 0)
+            virDomainRemoveInactive(&driver->domains,
+                                    vm);
         vm = NULL;
     }
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (fd != -1)
@@ -3766,7 +3780,8 @@ static int qemudDomainCoreDump(virDomainPtr dom,
     }
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -3827,7 +3842,8 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) {
     ret = 0;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -4235,9 +4251,9 @@ static int qemudDomainRestore(virConnectPtr conn,
     fd = -1;
     if (ret < 0) {
         if (!vm->persistent) {
-            qemuDomainObjEndJob(vm);
-            virDomainRemoveInactive(&driver->domains,
-                                    vm);
+            if (qemuDomainObjEndJob(vm) > 0)
+                virDomainRemoveInactive(&driver->domains,
+                                        vm);
             vm = NULL;
         }
         goto endjob;
@@ -4265,8 +4281,9 @@ static int qemudDomainRestore(virConnectPtr conn,
     ret = 0;
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     virDomainDefFree(def);
@@ -4314,7 +4331,10 @@ static char *qemudDomainDumpXML(virDomainPtr dom,
             qemuDomainObjEnterMonitor(vm);
             err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
             qemuDomainObjExitMonitor(vm);
-            qemuDomainObjEndJob(vm);
+            if (qemuDomainObjEndJob(vm) == 0) {
+                vm = NULL;
+                goto cleanup;
+            }
             if (err < 0)
                 goto cleanup;
             if (err > 0)
@@ -4547,7 +4567,8 @@ static int qemudDomainStart(virDomainPtr dom) {
                                          VIR_DOMAIN_EVENT_STARTED_BOOTED);
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -5338,7 +5359,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
         ret = -1;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (cgroup)
@@ -5672,7 +5694,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
         ret = -1;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     virDomainDeviceDefFree(dev);
@@ -5996,7 +6019,8 @@ qemudDomainBlockStats (virDomainPtr dom,
     qemuDomainObjExitMonitor(vm);
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     VIR_FREE(qemu_dev_name);
@@ -6214,7 +6238,8 @@ qemudDomainMemoryPeek (virDomainPtr dom,
     ret = 0;
 
 endjob:
-    qemuDomainObjEndJob(vm);
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     VIR_FREE(tmp);
@@ -6708,8 +6733,8 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn,
     if (qemust == NULL) {
         qemudShutdownVMDaemon(NULL, driver, vm);
         if (!vm->persistent) {
-            qemuDomainObjEndJob(vm);
-            virDomainRemoveInactive(&driver->domains, vm);
+            if (qemuDomainObjEndJob(vm) > 0)
+                virDomainRemoveInactive(&driver->domains, vm);
             vm = NULL;
         }
         virReportSystemError(dconn, errno,
@@ -6727,8 +6752,9 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn,
     ret = 0;
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     virDomainDefFree(def);
@@ -6895,8 +6921,8 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
          * should have already done that.
          */
         if (!vm->persistent) {
-            qemuDomainObjEndJob(vm);
-            virDomainRemoveInactive(&driver->domains, vm);
+            if (qemuDomainObjEndJob(vm) > 0)
+                virDomainRemoveInactive(&driver->domains, vm);
             vm = NULL;
         }
         goto endjob;
@@ -6908,8 +6934,9 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
     ret = 0;
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     virDomainDefFree(def);
@@ -7399,8 +7426,8 @@ qemudDomainMigratePerform (virDomainPtr dom,
                                      VIR_DOMAIN_EVENT_STOPPED_MIGRATED);
     if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) {
         virDomainDeleteConfig(dom->conn, driver->configDir, driver->autostartDir, vm);
-        qemuDomainObjEndJob(vm);
-        virDomainRemoveInactive(&driver->domains, vm);
+        if (qemuDomainObjEndJob(vm) > 0)
+            virDomainRemoveInactive(&driver->domains, vm);
         vm = NULL;
     }
     ret = 0;
@@ -7424,8 +7451,9 @@ endjob:
                                          VIR_DOMAIN_EVENT_RESUMED,
                                          VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
     }
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)
@@ -7523,15 +7551,16 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn,
                                          VIR_DOMAIN_EVENT_STOPPED,
                                          VIR_DOMAIN_EVENT_STOPPED_FAILED);
         if (!vm->persistent) {
-            qemuDomainObjEndJob(vm);
-            virDomainRemoveInactive(&driver->domains, vm);
+            if (qemuDomainObjEndJob(vm) > 0)
+                virDomainRemoveInactive(&driver->domains, vm);
             vm = NULL;
         }
     }
 
 endjob:
-    if (vm)
-        qemuDomainObjEndJob(vm);
+    if (vm &&
+        qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
 
 cleanup:
     if (vm)