conf: Clean up object referencing for Add and Remove

When adding a new object to the domain object list, there should
have been 2 virObjectRef calls made one for each list into which
the object was placed to match the 2 virObjectUnref calls that
would occur during Remove as part of virHashRemoveEntry when
virObjectFreeHashData is called when the element is removed from
the hash table as set up in virDomainObjListNew.

Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency
by calling virObjectRef upon successful return from virDomainObjListAdd
in order to use virDomainObjEndAPI when done with the returned @vm.
While others (bhyve, openvz, test, and vmware) handled this via only
calling virObjectUnlock upon successful return from virDomainObjListAdd.

This patch will "unify" the approach to use virDomainObjEndAPI
for any @vm successfully returned from virDomainObjListAdd.

Because list removal is so tightly coupled with list addition,
this patch fixes the list removal algorithm to return the object
as entered - "locked and reffed".  This way, the callers can then
decide how to uniformly handle add/remove success and failure.
This removes the onus on the caller to "specially handle" the
@vm during removal processing.

The Add/Remove logic allows for some logic simplification such
as in libxl where we can Remove the @vm directly rather than
needing to set a @remove_dom boolean and removing after the
libxlDomainObjEndJob completes as the @vm is locked/reffed.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

src/bhyve/bhyve_driver.c

@@ -550,7 +550,6 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
     if (virDomainSaveConfig(BHYVE_CONFIG_DIR, caps,
                             vm->newDef ? vm->newDef : vm->def) < 0) {
         virDomainObjListRemove(privconn->domains, vm);
-        vm = NULL;
         goto cleanup;
     }
 
@@ -566,8 +565,7 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
     virObjectUnref(caps);
     virDomainDefFree(def);
     virDomainDefFree(oldDef);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         virObjectEventStateQueue(privconn->domainEventState, event);
 
@@ -609,12 +607,10 @@ bhyveDomainUndefine(virDomainPtr domain)
                                               VIR_DOMAIN_EVENT_UNDEFINED,
                                               VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
 
-    if (virDomainObjIsActive(vm)) {
+    if (virDomainObjIsActive(vm))
         vm->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(privconn->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 
@@ -958,10 +954,8 @@ bhyveDomainCreateXML(virConnectPtr conn,
                              VIR_DOMAIN_RUNNING_BOOTED,
                              start_flags) < 0) {
         /* If domain is not persistent, remove its data */
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(privconn->domains, vm);
-            vm = NULL;
-        }
         goto cleanup;
     }
 
@@ -974,8 +968,7 @@ bhyveDomainCreateXML(virConnectPtr conn,
  cleanup:
     virObjectUnref(caps);
     virDomainDefFree(def);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         virObjectEventStateQueue(privconn->domainEventState, event);
 
@@ -1007,10 +1000,8 @@ bhyveDomainDestroy(virDomainPtr dom)
                                               VIR_DOMAIN_EVENT_STOPPED,
                                               VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
 
-    if (!vm->persistent) {
+    if (!vm->persistent)
         virDomainObjListRemove(privconn->domains, vm);
-        virObjectLock(vm);
-    }
 
  cleanup:
     virDomainObjEndAPI(&vm);

src/conf/virdomainobjlist.c

@@ -226,9 +226,13 @@ virDomainObjListFindByName(virDomainObjListPtr doms,
  * Upon entry @vm should have at least 1 ref and be locked.
  *
  * Add the @vm into the @doms->objs and @doms->objsName hash
- * tables.
+ * tables. Once successfully added into a table, increase the
+ * reference count since upon removal in virHashRemoveEntry
+ * the virObjectUnref will be called since the hash tables were
+ * configured to call virObjectFreeHashData when the object is
+ * removed from the hash table.
  *
- * Returns 0 on success with 2 references and locked
+ * Returns 0 on success with 3 references and locked
  *        -1 on failure with 1 reference and locked
  */
 static int
@@ -240,15 +244,12 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms,
     virUUIDFormat(vm->def->uuid, uuidstr);
     if (virHashAddEntry(doms->objs, uuidstr, vm) < 0)
         return -1;
+    virObjectRef(vm);
 
     if (virHashAddEntry(doms->objsName, vm->def->name, vm) < 0) {
-        virObjectRef(vm);
         virHashRemoveEntry(doms->objs, uuidstr);
         return -1;
     }
-
-    /* Since domain is in two hash tables, increment the
-     * reference counter */
     virObjectRef(vm);
 
     return 0;
@@ -266,6 +267,9 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms,
  * the @def being added is assumed to represent a
  * live config, not a future inactive config
  *
+ * The returned @vm from this function will be locked and ref
+ * counted. The caller is expected to use virDomainObjEndAPI
+ * when it completes usage.
  */
 static virDomainObjPtr
 virDomainObjListAddLocked(virDomainObjListPtr doms,
@@ -311,9 +315,6 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
                               def,
                               !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
                               oldDef);
-        /* XXX: Temporary until this API is fixed to return a locked and
-         *      refcnt'd object */
-        virObjectUnref(vm);
     } else {
         /* UUID does not match, but if a name matches, refuse it */
         if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) {
@@ -371,7 +372,6 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms,
 
     virHashRemoveEntry(doms->objs, uuidstr);
     virHashRemoveEntry(doms->objsName, dom->def->name);
-    virObjectUnlock(dom);
 }
 
 
@@ -386,8 +386,7 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms,
  * no one else is either waiting for 'dom' or still using it.
  *
  * When this function returns, @dom will be removed from the hash
- * tables, unlocked, and returned with the refcnt that was present
- * upon entry.
+ * tables and returned with lock and refcnt that was present upon entry.
  */
 void
 virDomainObjListRemove(virDomainObjListPtr doms,
@@ -453,9 +452,11 @@ virDomainObjListRename(virDomainObjListPtr doms,
     if (virHashAddEntry(doms->objsName, new_name, dom) < 0)
         goto cleanup;
 
-    /* Okay, this is crazy. virHashAddEntry() does not increment
-     * the refcounter of @dom, but virHashRemoveEntry() does
-     * decrement it. We need to work around it. */
+    /* Increment the refcnt for @new_name. We're about to remove
+     * the @old_name which will cause the refcnt to be decremented
+     * via the virObjectUnref call made during the virObjectFreeHashData
+     * as a result of removing something from the object list hash
+     * table as set up during virDomainObjListNew. */
     virObjectRef(dom);
 
     rc = callback(dom, new_name, flags, opaque);
@@ -624,7 +625,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
         if (dom) {
             if (!liveStatus)
                 dom->persistent = 1;
-            virObjectUnlock(dom);
+            virDomainObjEndAPI(&dom);
         } else {
             VIR_ERROR(_("Failed to load config for domain '%s'"), entry->d_name);
         }

src/libxl/libxl_driver.c

@@ -450,13 +450,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
 
  error:
     libxlDomainCleanup(driver, vm);
-    if (!vm->persistent) {
+    if (!vm->persistent)
         virDomainObjListRemoveLocked(driver->domains, vm);
-
-        /* virDomainObjListRemoveLocked leaves the object unlocked,
-         * lock it again to factorize more code. */
-        virObjectLock(vm);
-    }
     goto cleanup;
 }
 
@@ -605,7 +600,6 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
                                    0,
                                    &oldDef)))
         goto cleanup;
-
     def = NULL;
 
     vm->persistent = 1;
@@ -626,8 +620,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
     libxl_dominfo_dispose(&d_info);
     virDomainDefFree(def);
     virDomainDefFree(oldDef);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -1043,23 +1036,18 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-    virObjectRef(vm);
     def = NULL;
 
     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            virObjectLock(vm);
-        }
         goto cleanup;
     }
 
     if (libxlDomainStartNew(driver, vm,
                          (flags & VIR_DOMAIN_START_PAUSED) != 0) < 0) {
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            virObjectLock(vm);
-        }
         goto endjob;
     }
 
@@ -1396,10 +1384,8 @@ libxlDomainDestroyFlags(virDomainPtr dom,
                                      VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
 
     libxlDomainCleanup(driver, vm);
-    if (!vm->persistent) {
+    if (!vm->persistent)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 
@@ -1759,7 +1745,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
     libxlDriverPrivatePtr driver = dom->conn->privateData;
     virDomainObjPtr vm;
     int ret = -1;
-    bool remove_dom = false;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
     virReportUnsupportedError();
@@ -1791,7 +1776,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
         goto endjob;
 
     if (!vm->persistent)
-        remove_dom = true;
+        virDomainObjListRemove(driver->domains, vm);
 
     ret = 0;
 
@@ -1799,10 +1784,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
     libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (remove_dom && vm) {
-        virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
     virDomainObjEndAPI(&vm);
     return ret;
 }
@@ -1850,24 +1831,19 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-    virObjectRef(vm);
     def = NULL;
 
     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            virObjectLock(vm);
-        }
         goto cleanup;
     }
 
     ret = libxlDomainStartRestore(driver, vm,
                                   (flags & VIR_DOMAIN_SAVE_PAUSED) != 0,
                                   fd, hdr.version);
-    if (ret < 0 && !vm->persistent) {
+    if (ret < 0 && !vm->persistent)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     libxlDomainObjEndJob(driver, vm);
 
@@ -1893,7 +1869,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
     libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
     virDomainObjPtr vm;
     virObjectEventPtr event = NULL;
-    bool remove_dom = false;
     bool paused = false;
     int ret = -1;
 
@@ -1951,7 +1926,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
         event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
                                          VIR_DOMAIN_EVENT_STOPPED_CRASHED);
         if (!vm->persistent)
-            remove_dom = true;
+            virDomainObjListRemove(driver->domains, vm);
     }
 
     ret = 0;
@@ -1972,10 +1947,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
     libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (remove_dom && vm) {
-        virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
     virDomainObjEndAPI(&vm);
     if (event)
         libxlDomainEventQueue(driver, event);
@@ -1990,7 +1961,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags)
     virDomainObjPtr vm = NULL;
     char *name = NULL;
     int ret = -1;
-    bool remove_dom = false;
 
     virCheckFlags(0, -1);
 
@@ -2023,7 +1993,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags)
         goto endjob;
 
     if (!vm->persistent)
-        remove_dom = true;
+        virDomainObjListRemove(driver->domains, vm);
 
     ret = 0;
 
@@ -2031,10 +2001,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags)
     libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (remove_dom && vm) {
-        virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
     virDomainObjEndAPI(&vm);
     VIR_FREE(name);
     return ret;
@@ -2765,16 +2731,14 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag
                                    0,
                                    &oldDef)))
         goto cleanup;
-
-    virObjectRef(vm);
     def = NULL;
+
     vm->persistent = 1;
 
     if (virDomainSaveConfig(cfg->configDir,
                             cfg->caps,
                             vm->newDef ? vm->newDef : vm->def) < 0) {
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
         goto cleanup;
     }
 
@@ -2851,12 +2815,10 @@ libxlDomainUndefineFlags(virDomainPtr dom,
     event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED,
                                      VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
 
-    if (virDomainObjIsActive(vm)) {
+    if (virDomainObjIsActive(vm))
         vm->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 

src/libxl/libxl_migration.c

@@ -265,7 +265,6 @@ libxlDoMigrateDstReceive(void *opaque)
     int recvfd = args->recvfd;
     size_t i;
     int ret;
-    bool remove_dom = 0;
 
     virObjectRef(vm);
     virObjectLock(vm);
@@ -280,7 +279,7 @@ libxlDoMigrateDstReceive(void *opaque)
                                   args->migcookie->xenMigStreamVer);
 
     if (ret < 0 && !vm->persistent)
-        remove_dom = true;
+        virDomainObjListRemove(driver->domains, vm);
 
     /* Remove all listen socks from event handler, and close them. */
     for (i = 0; i < nsocks; i++) {
@@ -296,10 +295,6 @@ libxlDoMigrateDstReceive(void *opaque)
     libxlDomainObjEndJob(driver, vm);
 
  cleanup:
-    if (remove_dom) {
-        virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
     virDomainObjEndAPI(&vm);
 }
 
@@ -581,9 +576,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto error;
-
-    virObjectRef(vm);
     *def = NULL;
+
     priv = vm->privateData;
 
     if (taint_hook) {
@@ -633,10 +627,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
     VIR_FORCE_CLOSE(dataFD[0]);
     virObjectUnref(args);
     /* Remove virDomainObj from domain list */
-    if (vm) {
+    if (vm)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
  done:
     virDomainObjEndAPI(&vm);
@@ -680,9 +672,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto error;
-
-    virObjectRef(vm);
     *def = NULL;
+
     priv = vm->privateData;
 
     if (taint_hook) {
@@ -807,10 +798,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
         priv->migrationPort = 0;
     }
     /* Remove virDomainObj from domain list */
-    if (vm) {
+    if (vm)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
  done:
     VIR_FREE(xmlout);
@@ -1336,11 +1325,8 @@ libxlDomainMigrationDstFinish(virConnectPtr dconn,
                              VIR_DOMAIN_SHUTOFF_FAILED);
         event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
                                          VIR_DOMAIN_EVENT_STOPPED_FAILED);
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            /* Caller passed a locked vm and expects the same on return */
-            virObjectLock(vm);
-        }
     }
 
     if (event)
@@ -1392,11 +1378,8 @@ libxlDomainMigrationSrcConfirm(libxlDriverPrivatePtr driver,
     if (flags & VIR_MIGRATE_UNDEFINE_SOURCE)
         virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm);
 
-    if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) {
+    if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE))
         virDomainObjListRemove(driver->domains, vm);
-        /* Caller passed a locked vm and expects the same on return */
-        virObjectLock(vm);
-    }
 
     ret = 0;
 

src/lxc/lxc_driver.c

@@ -478,14 +478,12 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
                                    0, &oldDef)))
         goto cleanup;
 
-    virObjectRef(vm);
     def = NULL;
     vm->persistent = 1;
 
     if (virDomainSaveConfig(cfg->configDir, driver->caps,
                             vm->newDef ? vm->newDef : vm->def) < 0) {
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
         goto cleanup;
     }
 
@@ -546,12 +544,10 @@ static int lxcDomainUndefineFlags(virDomainPtr dom,
                                      VIR_DOMAIN_EVENT_UNDEFINED,
                                      VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
 
-    if (virDomainObjIsActive(vm)) {
+    if (virDomainObjIsActive(vm))
         vm->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 
@@ -1233,14 +1229,11 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-    virObjectRef(vm);
     def = NULL;
 
     if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            virObjectLock(vm);
-        }
         goto cleanup;
     }
 
@@ -1250,10 +1243,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
                            VIR_DOMAIN_RUNNING_BOOTED) < 0) {
         virDomainAuditStart(vm, "booted", false);
         virLXCDomainObjEndJob(driver, vm);
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            virObjectLock(vm);
-        }
         goto cleanup;
     }
 
@@ -1523,10 +1514,8 @@ lxcDomainDestroyFlags(virDomainPtr dom,
 
  endjob:
     virLXCDomainObjEndJob(driver, vm);
-    if (!vm->persistent) {
+    if (!vm->persistent)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
  cleanup:
     virDomainObjEndAPI(&vm);

src/lxc/lxc_process.c

@@ -671,10 +671,8 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon,
         } else {
             VIR_DEBUG("Stop event has already been sent");
         }
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            vm = NULL;
-        }
     } else {
         int ret = virLXCProcessReboot(driver, vm);
         virDomainAuditStop(vm, "reboot");
@@ -685,15 +683,15 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon,
             event = virDomainEventLifecycleNewFromObj(vm,
                                              VIR_DOMAIN_EVENT_STOPPED,
                                              priv->stopReason);
-            if (!vm->persistent) {
+            if (!vm->persistent)
                 virDomainObjListRemove(driver->domains, vm);
-                vm = NULL;
-            }
         }
     }
 
-    if (vm)
-        virObjectUnlock(vm);
+    /* NB: virLXCProcessConnectMonitor will perform the virObjectRef(vm)
+     * before adding monitorCallbacks. Since we are now done with the @vm
+     * we can Unref/Unlock */
+    virDomainObjEndAPI(&vm);
     if (event)
         virObjectEventStateQueue(driver->domainEventState, event);
 }
@@ -803,7 +801,8 @@ static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver,
         goto cleanup;
 
     /* Hold an extra reference because we can't allow 'vm' to be
-     * deleted while the monitor is active */
+     * deleted while the monitor is active. This will be unreffed
+     * during EOFNotify processing. */
     virObjectRef(vm);
 
     monitor = virLXCMonitorNew(vm, cfg->stateDir, &monitorCallbacks);

src/openvz/openvz_conf.c

@@ -611,7 +611,7 @@ int openvzLoadDomains(struct openvz_driver *driver)
         /* XXX OpenVZ doesn't appear to have concept of a transient domain */
         dom->persistent = 1;
 
-        virObjectUnlock(dom);
+        virDomainObjEndAPI(&dom);
         dom = NULL;
         def = NULL;
     }

src/openvz/openvz_driver.c

@@ -982,8 +982,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla
 
  cleanup:
     virDomainDefFree(vmdef);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     openvzDriverUnlock(driver);
     return dom;
 }
@@ -1071,8 +1070,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml,
 
  cleanup:
     virDomainDefFree(vmdef);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     openvzDriverUnlock(driver);
     return dom;
 }
@@ -1151,12 +1149,10 @@ openvzDomainUndefineFlags(virDomainPtr dom,
     if (virRun(prog, NULL) < 0)
         goto cleanup;
 
-    if (virDomainObjIsActive(vm)) {
+    if (virDomainObjIsActive(vm))
         vm->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 
@@ -2232,16 +2228,13 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
 
  error:
     virDomainDefFree(def);
-    if (vm) {
+    if (vm)
         virDomainObjListRemove(driver->domains, vm);
-        vm = NULL;
-    }
 
  done:
     VIR_FREE(my_hostname);
     virURIFree(uri);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -2396,7 +2389,6 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain,
     VIR_DEBUG("Domain '%s' successfully migrated", vm->def->name);
 
     virDomainObjListRemove(driver->domains, vm);
-    virObjectLock(vm);
 
     ret = 0;
 

src/qemu/qemu_domain.c

@@ -7167,24 +7167,9 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
         VIR_FREE(snapDir);
     }
 
-    virObjectRef(vm);
-
     virDomainObjListRemove(driver->domains, vm);
-    /*
-     * virDomainObjListRemove() leaves the domain unlocked so it can
-     * be unref'd for other drivers that depend on that, but we still
-     * need to reset a job and we have a reference from the API that
-     * called this function.  So we need to lock it back.  This is
-     * just a workaround for the qemu driver.
-     *
-     * XXX: Ideally, the global handling of domain objects and object
-     *      lists would be refactored so we don't need hacks like
-     *      this, but since that requires refactor of all drivers,
-     *      it's a work for another day.
-     */
-    virObjectLock(vm);
+
     virObjectUnref(cfg);
-    virObjectUnref(vm);
 }
 
 

src/qemu/qemu_driver.c

@@ -1765,7 +1765,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-    virObjectRef(vm);
     def = NULL;
 
     if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START,
@@ -6737,7 +6736,6 @@ qemuDomainRestoreFlags(virConnectPtr conn,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-    virObjectRef(vm);
     def = NULL;
 
     if (flags & VIR_DOMAIN_SAVE_RUNNING)
@@ -7411,9 +7409,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
                                    driver->xmlopt,
                                    0, &oldDef)))
         goto cleanup;
-
-    virObjectRef(vm);
     def = NULL;
+
     if (qemuDomainHasBlockjob(vm, true)) {
         virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
                        _("domain has active block job"));
@@ -16420,7 +16417,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
                                    NULL)))
         goto cleanup;
 
-    virObjectRef(vm);
     def = NULL;
 
     if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {

src/qemu/qemu_migration.c

@@ -2280,9 +2280,8 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
                                    VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
                                    NULL)))
         goto cleanup;
-
-    virObjectRef(vm);
     *def = NULL;
+
     priv = vm->privateData;
     if (VIR_STRDUP(priv->origname, origname) < 0)
         goto cleanup;

src/test/test_driver.c

@@ -893,7 +893,7 @@ testParseDomains(testDriverPtr privconn,
     int num, ret = -1;
     size_t i;
     xmlNodePtr *nodes = NULL;
-    virDomainObjPtr obj;
+    virDomainObjPtr obj = NULL;
 
     num = virXPathNodeSet("/node/domain", ctxt, &nodes);
     if (num < 0)
@@ -921,10 +921,8 @@ testParseDomains(testDriverPtr privconn,
             goto error;
         }
 
-        if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) {
-            virObjectUnlock(obj);
+        if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0)
             goto error;
-        }
 
         nsdata = def->namespaceData;
         obj->persistent = !nsdata->transient;
@@ -932,20 +930,19 @@ testParseDomains(testDriverPtr privconn,
 
         if (nsdata->runstate != VIR_DOMAIN_SHUTOFF) {
             if (testDomainStartState(privconn, obj,
-                                     VIR_DOMAIN_RUNNING_BOOTED) < 0) {
-                virObjectUnlock(obj);
+                                     VIR_DOMAIN_RUNNING_BOOTED) < 0)
                 goto error;
-            }
         } else {
             testDomainShutdownState(NULL, obj, 0);
         }
         virDomainObjSetState(obj, nsdata->runstate, 0);
 
-        virObjectUnlock(obj);
+        virDomainObjEndAPI(&obj);
     }
 
     ret = 0;
  error:
+    virDomainObjEndAPI(&obj);
     VIR_FREE(nodes);
     return ret;
 }
@@ -1678,10 +1675,8 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
     def = NULL;
 
     if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) {
-        if (!dom->persistent) {
+        if (!dom->persistent)
             virDomainObjListRemove(privconn->domains, dom);
-            dom = NULL;
-        }
         goto cleanup;
     }
 
@@ -1692,8 +1687,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
     ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
 
  cleanup:
-    if (dom)
-        virObjectUnlock(dom);
+    virDomainObjEndAPI(&dom);
     testObjectEventQueue(privconn, event);
     virDomainDefFree(def);
     testDriverUnlock(privconn);
@@ -1787,10 +1781,8 @@ static int testDomainDestroyFlags(virDomainPtr domain,
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
 
-    if (!privdom->persistent) {
+    if (!privdom->persistent)
         virDomainObjListRemove(privconn->domains, privdom);
-        virObjectLock(privdom);
-    }
 
     ret = 0;
  cleanup:
@@ -1888,10 +1880,8 @@ static int testDomainShutdownFlags(virDomainPtr domain,
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
 
-    if (!privdom->persistent) {
+    if (!privdom->persistent)
         virDomainObjListRemove(privconn->domains, privdom);
-        virObjectLock(privdom);
-    }
 
     ret = 0;
  cleanup:
@@ -1957,10 +1947,8 @@ static int testDomainReboot(virDomainPtr domain,
                                          VIR_DOMAIN_EVENT_STOPPED,
                                          VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
 
-        if (!privdom->persistent) {
+        if (!privdom->persistent)
             virDomainObjListRemove(privconn->domains, privdom);
-            virObjectLock(privdom);
-        }
     }
 
     ret = 0;
@@ -2095,10 +2083,8 @@ testDomainSaveFlags(virDomainPtr domain, const char *path,
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_SAVED);
 
-    if (!privdom->persistent) {
+    if (!privdom->persistent)
         virDomainObjListRemove(privconn->domains, privdom);
-        virObjectLock(privdom);
-    }
 
     ret = 0;
  cleanup:
@@ -2200,10 +2186,8 @@ testDomainRestoreFlags(virConnectPtr conn,
     def = NULL;
 
     if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) {
-        if (!dom->persistent) {
+        if (!dom->persistent)
             virDomainObjListRemove(privconn->domains, dom);
-            dom = NULL;
-        }
         goto cleanup;
     }
 
@@ -2216,8 +2200,7 @@ testDomainRestoreFlags(virConnectPtr conn,
     virDomainDefFree(def);
     VIR_FREE(xml);
     VIR_FORCE_CLOSE(fd);
-    if (dom)
-        virObjectUnlock(dom);
+    virDomainObjEndAPI(&dom);
     testObjectEventQueue(privconn, event);
     return ret;
 }
@@ -2280,10 +2263,8 @@ static int testDomainCoreDumpWithFormat(virDomainPtr domain,
         event = virDomainEventLifecycleNewFromObj(privdom,
                                          VIR_DOMAIN_EVENT_STOPPED,
                                          VIR_DOMAIN_EVENT_STOPPED_CRASHED);
-        if (!privdom->persistent) {
+        if (!privdom->persistent)
             virDomainObjListRemove(privconn->domains, privdom);
-            virObjectLock(privdom);
-        }
     }
 
     ret = 0;
@@ -2800,8 +2781,7 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn,
  cleanup:
     virDomainDefFree(def);
     virDomainDefFree(oldDef);
-    if (dom)
-        virObjectUnlock(dom);
+    virDomainObjEndAPI(&dom);
     testObjectEventQueue(privconn, event);
     return ret;
 }
@@ -3068,12 +3048,10 @@ static int testDomainUndefineFlags(virDomainPtr domain,
                                      VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
     privdom->hasManagedSave = false;
 
-    if (virDomainObjIsActive(privdom)) {
+    if (virDomainObjIsActive(privdom))
         privdom->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(privconn->domains, privdom);
-        virObjectLock(privdom);
-    }
 
     ret = 0;
 

src/uml/uml_driver.c

@@ -379,10 +379,8 @@ umlInotifyEvent(int watch,
             event = virDomainEventLifecycleNewFromObj(dom,
                                              VIR_DOMAIN_EVENT_STOPPED,
                                              VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
-            if (!dom->persistent) {
+            if (!dom->persistent)
                 virDomainObjListRemove(driver->domains, dom);
-                virObjectLock(dom);
-            }
         } else if (e.mask & (IN_CREATE | IN_MODIFY)) {
             VIR_DEBUG("Got inotify domain startup '%s'", name);
             if (virDomainObjIsActive(dom)) {
@@ -412,10 +410,8 @@ umlInotifyEvent(int watch,
                 event = virDomainEventLifecycleNewFromObj(dom,
                                                  VIR_DOMAIN_EVENT_STOPPED,
                                                  VIR_DOMAIN_EVENT_STOPPED_FAILED);
-                if (!dom->persistent) {
+                if (!dom->persistent)
                     virDomainObjListRemove(driver->domains, dom);
-                    virObjectLock(dom);
-                }
             } else if (umlIdentifyChrPTY(driver, dom) < 0) {
                 VIR_WARN("Could not identify character devices for new domain");
                 umlShutdownVMDaemon(driver, dom,
@@ -424,10 +420,8 @@ umlInotifyEvent(int watch,
                 event = virDomainEventLifecycleNewFromObj(dom,
                                                  VIR_DOMAIN_EVENT_STOPPED,
                                                  VIR_DOMAIN_EVENT_STOPPED_FAILED);
-                if (!dom->persistent) {
+                if (!dom->persistent)
                     virDomainObjListRemove(driver->domains, dom);
-                    virObjectLock(dom);
-                }
             }
         }
         virDomainObjEndAPI(&dom);
@@ -785,10 +779,8 @@ static int umlProcessAutoDestroyDom(void *payload,
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
 
-    if (!dom->persistent) {
+    if (!dom->persistent)
         virDomainObjListRemove(data->driver->domains, dom);
-        virObjectLock(dom);
-    }
 
     virDomainObjEndAPI(&dom);
     if (event)
@@ -1609,10 +1601,8 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml,
     if (umlStartVMDaemon(conn, driver, vm,
                          (flags & VIR_DOMAIN_START_AUTODESTROY)) < 0) {
         virDomainAuditStart(vm, "booted", false);
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            vm = NULL;
-        }
         goto cleanup;
     }
     virDomainAuditStart(vm, "booted", true);
@@ -1624,8 +1614,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml,
 
  cleanup:
     virDomainDefFree(def);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         umlDomainEventQueue(driver, event);
     umlDriverUnlock(driver);
@@ -1694,10 +1683,8 @@ umlDomainDestroyFlags(virDomainPtr dom,
     event = virDomainEventLifecycleNewFromObj(vm,
                                      VIR_DOMAIN_EVENT_STOPPED,
                                      VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
-    if (!vm->persistent) {
+    if (!vm->persistent)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
     ret = 0;
 
  cleanup:
@@ -2009,7 +1996,6 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
     if (virDomainSaveConfig(driver->configDir, driver->caps,
                             vm->newDef ? vm->newDef : vm->def) < 0) {
         virDomainObjListRemove(driver->domains, vm);
-        vm = NULL;
         goto cleanup;
     }
 
@@ -2017,8 +2003,7 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
 
  cleanup:
     virDomainDefFree(def);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     umlDriverUnlock(driver);
     return dom;
 }
@@ -2054,12 +2039,10 @@ static int umlDomainUndefineFlags(virDomainPtr dom,
     if (virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm) < 0)
         goto cleanup;
 
-    if (virDomainObjIsActive(vm)) {
+    if (virDomainObjIsActive(vm))
         vm->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 

src/vmware/vmware_conf.c

@@ -186,10 +186,9 @@ vmwareLoadDomains(struct vmware_driver *driver)
                              VIR_DOMAIN_RUNNING_UNKNOWN);
         vm->persistent = 1;
 
-        virObjectUnlock(vm);
+        virDomainObjEndAPI(&vm);
 
         vmdef = NULL;
-        vm = NULL;
     }
 
     ret = 0;

src/vmware/vmware_driver.c

@@ -502,10 +502,8 @@ vmwareDomainShutdownFlags(virDomainPtr dom,
     if (vmwareStopVM(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN) < 0)
         goto cleanup;
 
-    if (!vm->persistent) {
+    if (!vm->persistent)
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
  cleanup:
@@ -717,10 +715,8 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml,
     vmdef = NULL;
 
     if (vmwareStartVM(driver, vm) < 0) {
-        if (!vm->persistent) {
+        if (!vm->persistent)
             virDomainObjListRemove(driver->domains, vm);
-            vm = NULL;
-        }
         goto cleanup;
     }
 
@@ -730,8 +726,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml,
     virDomainDefFree(vmdef);
     VIR_FREE(vmx);
     VIR_FREE(vmxPath);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     vmwareDriverUnlock(driver);
     return dom;
 }
@@ -796,12 +791,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom,
     if (vmwareUpdateVMStatus(driver, vm) < 0)
         goto cleanup;
 
-    if (virDomainObjIsActive(vm)) {
+    if (virDomainObjIsActive(vm))
         vm->persistent = 0;
-    } else {
+    else
         virDomainObjListRemove(driver->domains, vm);
-        virObjectLock(vm);
-    }
 
     ret = 0;
 

src/vz/vz_driver.c

@@ -3161,7 +3161,6 @@ vzDomainMigratePerformStep(virDomainObjPtr dom,
         goto cleanup;
 
     virDomainObjListRemove(driver->domains, dom);
-    virObjectLock(dom);
 
     ret = 0;
 

src/vz/vz_sdk.c

@@ -1969,7 +1969,6 @@ prlsdkLoadDomain(vzDriverPtr driver,
             goto error;
         }
 
-        virObjectRef(dom);
         pdom = dom->privateData;
         pdom->sdkdom = sdkdom;
         PrlHandle_AddRef(sdkdom);
@@ -2235,7 +2234,6 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver,
                     VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
 
     virDomainObjListRemove(driver->domains, dom);
-    virObjectLock(dom);
     virDomainObjEndAPI(&dom);
     return;
 }
@@ -4334,7 +4332,6 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla
                     VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
 
     virDomainObjListRemove(driver->domains, dom);
-    virObjectLock(dom);
 
     ret = 0;
  cleanup: