vz: cleanup loading domain code

  9c14a9ab introduced vzNewDomain function to enlist libvirt domain
object before actually creating vz sdk domain. Fix should fix
race on same vz sdk domain added event where libvirt domain object is
enlisted too. But later eb5e9c1e added locked checks for
adding livirtd domain object to list on vz sdk domain added event.
Thus now approach of 9c14a9ab is unnecessary complicated.

  See we have otherwise unuseful prlsdkGetDomainIds function only
to create minimal domain definition to create libvirt domain object.
Also vzNewDomain is difficult to use as it creates partially
constructed domain object.

  Let's move back to original approach where prlsdkLoadDomain do
all the necessary job. Another benefit is that we can now
take driver lock for bare minimum and in single place. Reducing
locking time have small disadvatage of double parsing on race
conditions which is typical if domain is added thru vz driver.
Well we have this double parse inevitably with current vz sdk api
on any domain updates so i would not take it here seriously.

  Performance events subscribtion is done before locked check and
therefore could be done twice on races but this is not the problem.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 1bb99f069fc4052ed62465eadfcfd28408835eee..9a71c3b6a9951046684965802eac0953ba3f9957 100644 (file)
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -730,7 +730,6 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
     if (flags & VIR_DOMAIN_DEFINE_VALIDATE)
         parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
 
-    virObjectLock(driver);
     if ((def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
                                        parse_flags)) == NULL)
         goto cleanup;
@@ -738,9 +737,6 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
     olddom = virDomainObjListFindByUUID(driver->domains, def->uuid);
     if (olddom == NULL) {
         virResetLastError();
-        newdom = vzNewDomain(driver, def->name, def->uuid);
-        if (!newdom)
-            goto cleanup;
         if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
             if (prlsdkCreateVm(driver, def))
                 goto cleanup;
@@ -754,7 +750,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
             goto cleanup;
         }
 
-        if (prlsdkLoadDomain(driver, newdom))
+        if (!(newdom = prlsdkAddDomainByUUID(driver, def->uuid)))
             goto cleanup;
     } else {
         int state, reason;
@@ -802,7 +798,6 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
              virObjectUnlock(newdom);
     }
     virDomainDefFree(def);
-    virObjectUnlock(driver);
     return retdom;
 }
 
@@ -2621,7 +2616,6 @@ vzDomainMigrateFinish3Params(virConnectPtr dconn,
     vzConnPtr privconn = dconn->privateData;
     vzDriverPtr driver = privconn->driver;
     const char *name = NULL;
-    PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
 
     virCheckFlags(VZ_MIGRATION_FLAGS, NULL);
 
@@ -2635,11 +2629,8 @@ vzDomainMigrateFinish3Params(virConnectPtr dconn,
                                 VIR_MIGRATE_PARAM_DEST_NAME, &name) < 0)
         return NULL;
 
-    sdkdom = prlsdkSdkDomainLookupByName(driver, name);
-    if (sdkdom == PRL_INVALID_HANDLE)
-        goto cleanup;
 
-    if (!(dom = prlsdkNewDomainByHandle(driver, sdkdom)))
+    if (!(dom = prlsdkAddDomainByName(driver, name)))
         goto cleanup;
 
     domain = virGetDomain(dconn, dom->def->name, dom->def->uuid);
@@ -2653,7 +2644,6 @@ vzDomainMigrateFinish3Params(virConnectPtr dconn,
         VIR_WARN("Can't provide domain '%s' after successfull migration.", name);
     if (dom)
         virObjectUnlock(dom);
-    PrlHandle_Free(sdkdom);
     return domain;
 }
 

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 9495a36968a4cf489af07565d2514b8523f7a670..0662be12806395ba3be091dbaca147b94055a0e2 100644 (file)
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -417,37 +417,6 @@ prlsdkUUIDParse(const char *uuidstr, unsigned char *uuid)
     return ret;
 }
 
-static int
-prlsdkGetDomainIds(PRL_HANDLE sdkdom,
-                   char **name,
-                   unsigned char *uuid)
-{
-    char uuidstr[VIR_UUID_STRING_BRACED_BUFLEN];
-    PRL_RESULT pret;
-
-    if (name && !(*name = prlsdkGetStringParamVar(PrlVmCfg_GetName, sdkdom)))
-        goto error;
-
-    if (uuid) {
-        pret = prlsdkGetStringParamBuf(PrlVmCfg_GetUuid,
-                                       sdkdom, uuidstr, sizeof(uuidstr));
-        prlsdkCheckRetGoto(pret, error);
-
-        if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("Domain UUID is malformed or empty"));
-            goto error;
-        }
-    }
-
-    return 0;
-
- error:
-    if (name)
-        VIR_FREE(*name);
-    return -1;
-}
-
 static int
 prlsdkGetDomainState(PRL_HANDLE sdkdom, VIRTUAL_MACHINE_STATE_PTR vmState)
 {
@@ -1454,36 +1423,6 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr def)
     return -1;
 }
 
-virDomainObjPtr
-prlsdkNewDomainByHandle(vzDriverPtr driver, PRL_HANDLE sdkdom)
-{
-    virDomainObjPtr dom = NULL;
-    unsigned char uuid[VIR_UUID_BUFLEN];
-    char *name = NULL;
-
-    virObjectLock(driver);
-    if (prlsdkGetDomainIds(sdkdom, &name, uuid) < 0)
-        goto cleanup;
-
-    /* we should make sure that there is no such a VM exists */
-    if ((dom = virDomainObjListFindByUUID(driver->domains, uuid)))
-        goto cleanup;
-
-    if (!(dom = vzNewDomain(driver, name, uuid)))
-        goto cleanup;
-
-    if (prlsdkLoadDomain(driver, dom) < 0) {
-        virDomainObjListRemove(driver->domains, dom);
-        dom = NULL;
-        goto cleanup;
-    }
-
- cleanup:
-    virObjectUnlock(driver);
-    VIR_FREE(name);
-    return dom;
-}
-
 static PRL_HANDLE
 prlsdkGetDevByDevIndex(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE type, PRL_UINT32 devIndex)
 {
@@ -1707,8 +1646,16 @@ prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def)
     return ret;
 }
 
-int
-prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom)
+/* if dom is NULL adds new domain into domain list
+ * if dom not NULL updates given locked dom object.
+ *
+ * Returned object is locked.
+ */
+
+static virDomainObjPtr
+prlsdkLoadDomain(vzDriverPtr driver,
+                 PRL_HANDLE sdkdom,
+                 virDomainObjPtr dom)
 {
     virDomainDefPtr def = NULL;
     vzDomObjPtr pdom = NULL;
@@ -1718,24 +1665,26 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom)
     PRL_UINT32 ram;
     PRL_UINT32 envId;
     PRL_VM_AUTOSTART_OPTION autostart;
-    PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
     PRL_HANDLE job;
-
-    virCheckNonNullArgGoto(dom, error);
-
-    pdom = dom->privateData;
-    sdkdom = prlsdkSdkDomainLookupByUUID(driver, dom->def->uuid);
-    if (sdkdom == PRL_INVALID_HANDLE)
-        return -1;
+    char uuidstr[VIR_UUID_STRING_BRACED_BUFLEN];
 
     if (!(def = virDomainDefNew()))
         goto error;
 
-    def->virtType = dom->def->virtType;
-    def->id = dom->def->id;
+    if (!(def->name = prlsdkGetStringParamVar(PrlVmCfg_GetName, sdkdom)))
+        goto error;
+
+    pret = prlsdkGetStringParamBuf(PrlVmCfg_GetUuid,
+                                   sdkdom, uuidstr, sizeof(uuidstr));
+    prlsdkCheckRetGoto(pret, error);
 
-    if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
+    if (prlsdkUUIDParse(uuidstr, def->uuid) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Domain UUID is malformed or empty"));
         goto error;
+    }
+
+    def->virtType = VIR_DOMAIN_VIRT_VZ;
 
     def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
     def->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY;
@@ -1801,20 +1750,38 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom)
             goto error;
     }
 
-    if (!pdom->sdkdom) {
+    if (!dom) {
+        virDomainObjPtr olddom = NULL;
+
         job = PrlVm_SubscribeToPerfStats(sdkdom, NULL);
         if (PRL_FAILED(waitJob(job)))
             goto error;
 
-        PrlHandle_AddRef(sdkdom);
+        virObjectLock(driver);
+        if (!(olddom = virDomainObjListFindByUUID(driver->domains, def->uuid)))
+            dom = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0, NULL);
+        virObjectUnlock(driver);
+
+        if (olddom) {
+            virDomainDefFree(def);
+            return olddom;
+        } else if (!dom) {
+            goto error;
+        }
+
+        pdom = dom->privateData;
         pdom->sdkdom = sdkdom;
+        PrlHandle_AddRef(sdkdom);
+        dom->persistent = 1;
+    } else {
+        /* assign new virDomainDef without any checks
+         * we can't use virDomainObjAssignDef, because it checks
+         * for state and domain name */
+        virDomainDefFree(dom->def);
+        dom->def = def;
     }
 
-    /* assign new virDomainDef without any checks
-     * we can't use virDomainObjAssignDef, because it checks
-     * for state and domain name */
-    virDomainDefFree(dom->def);
-    dom->def = def;
+    pdom = dom->privateData;
     pdom->id = envId;
 
     prlsdkConvertDomainState(domainState, envId, dom);
@@ -1824,12 +1791,11 @@ prlsdkLoadDomain(vzDriverPtr driver, virDomainObjPtr dom)
     else
         dom->autostart = 0;
 
-    PrlHandle_Free(sdkdom);
-    return 0;
+    return dom;
+
  error:
-    PrlHandle_Free(sdkdom);
     virDomainDefFree(def);
-    return -1;
+    return NULL;
 }
 
 int
@@ -1855,7 +1821,7 @@ prlsdkLoadDomains(vzDriverPtr driver)
         pret = PrlResult_GetParamByIndex(result, i, &sdkdom);
         prlsdkCheckRetGoto(pret, error);
 
-        if ((dom = prlsdkNewDomainByHandle(driver, sdkdom)))
+        if ((dom = prlsdkLoadDomain(driver, sdkdom, NULL)))
             virObjectUnlock(dom);
 
         PrlHandle_Free(sdkdom);
@@ -1871,6 +1837,38 @@ prlsdkLoadDomains(vzDriverPtr driver)
     return -1;
 }
 
+virDomainObjPtr
+prlsdkAddDomainByUUID(vzDriverPtr driver, const unsigned char *uuid)
+{
+    PRL_HANDLE sdkdom;
+    virDomainObjPtr dom;
+
+    sdkdom = prlsdkSdkDomainLookupByUUID(driver, uuid);
+    if (sdkdom == PRL_INVALID_HANDLE)
+        return NULL;
+
+    dom = prlsdkLoadDomain(driver, sdkdom, NULL);
+
+    PrlHandle_Free(sdkdom);
+    return dom;
+}
+
+virDomainObjPtr
+prlsdkAddDomainByName(vzDriverPtr driver, const char *name)
+{
+    PRL_HANDLE sdkdom;
+    virDomainObjPtr dom;
+
+    sdkdom = prlsdkSdkDomainLookupByName(driver, name);
+    if (sdkdom == PRL_INVALID_HANDLE)
+        return NULL;
+
+    dom = prlsdkLoadDomain(driver, sdkdom, NULL);
+
+    PrlHandle_Free(sdkdom);
+    return dom;
+}
+
 int
 prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom)
 {
@@ -1881,7 +1879,7 @@ prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom)
     if (waitJob(job))
         return -1;
 
-    return prlsdkLoadDomain(driver, dom);
+    return prlsdkLoadDomain(driver, pdom->sdkdom, dom) ? 0 : -1;
 }
 
 static int prlsdkSendEvent(vzDriverPtr driver,
@@ -2000,15 +1998,9 @@ prlsdkHandleVmAddedEvent(vzDriverPtr driver,
     virDomainObjPtr dom = NULL;
     PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
 
-    dom = virDomainObjListFindByUUID(driver->domains, uuid);
-    if (!dom) {
-        sdkdom = prlsdkSdkDomainLookupByUUID(driver, uuid);
-        if (sdkdom == PRL_INVALID_HANDLE)
-            goto cleanup;
-
-        if (!(dom = prlsdkNewDomainByHandle(driver, sdkdom)))
-            goto cleanup;
-    }
+    if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)) &&
+        !(dom = prlsdkAddDomainByUUID(driver, uuid)))
+        goto cleanup;
 
     prlsdkSendEvent(driver, dom, VIR_DOMAIN_EVENT_DEFINED,
                     VIR_DOMAIN_EVENT_DEFINED_ADDED);

diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
index a7dec565ffc5612ea42606380d6e73a425d43c5b..262887351e3c375200ffdcfb40957a9737db5468 100644 (file)
--- a/src/vz/vz_sdk.h
+++ b/src/vz/vz_sdk.h
@@ -30,10 +30,11 @@ int prlsdkConnect(vzDriverPtr driver);
 void prlsdkDisconnect(vzDriverPtr driver);
 int
 prlsdkLoadDomains(vzDriverPtr driver);
+virDomainObjPtr
+prlsdkAddDomainByUUID(vzDriverPtr driver, const unsigned char *uuid);
+virDomainObjPtr
+prlsdkAddDomainByName(vzDriverPtr driver, const char *name);
 int prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom);
-int
-prlsdkLoadDomain(vzDriverPtr driver,
-                 virDomainObjPtr dom);
 int prlsdkSubscribeToPCSEvents(vzDriverPtr driver);
 void prlsdkUnsubscribeFromPCSEvents(vzDriverPtr driver);
 PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
@@ -95,5 +96,3 @@ prlsdkMigrate(virDomainObjPtr dom,
 
 PRL_HANDLE
 prlsdkSdkDomainLookupByName(vzDriverPtr driver, const char *name);
-virDomainObjPtr
-prlsdkNewDomainByHandle(vzDriverPtr driver, PRL_HANDLE sdkdom);

diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c
index 114738e6da1ea562972c53f18515b61694392440..d3427caaf3c710b6c71023fa6292a9d4461dcac7 100644 (file)
--- a/src/vz/vz_utils.c
+++ b/src/vz/vz_utils.c
@@ -159,30 +159,6 @@ vzGetOutput(const char *binary, ...)
     return outbuf;
 }
 
-virDomainObjPtr
-vzNewDomain(vzDriverPtr driver, const char *name, const unsigned char *uuid)
-{
-    virDomainDefPtr def = NULL;
-    virDomainObjPtr dom = NULL;
-
-    if (!(def = virDomainDefNewFull(name, uuid, -1)))
-        goto error;
-
-    def->virtType = VIR_DOMAIN_VIRT_VZ;
-
-    if (!(dom = virDomainObjListAdd(driver->domains, def,
-                                    driver->xmlopt,
-                                    0, NULL)))
-        goto error;
-
-    dom->persistent = 1;
-    return dom;
-
- error:
-    virDomainDefFree(def);
-    return NULL;
-}
-
 static void
 vzInitCaps(unsigned long vzVersion, vzCapabilitiesPtr vzCaps)
 {

diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h
index a9a8176952e3f8beef6753e11f764e46c66dd354..92503a7ed439501e70d4b1573f8a0f7462e35f30 100644 (file)
--- a/src/vz/vz_utils.h
+++ b/src/vz/vz_utils.h
@@ -117,10 +117,6 @@ vzGetDriverConnection(void);
 void
 vzDestroyDriverConnection(void);
 
-virDomainObjPtr
-vzNewDomain(vzDriverPtr driver,
-            const char *name,
-            const unsigned char *uuid);
 int
 vzInitVersion(vzDriverPtr driver);
 int