From cfc6815568396794cf6e365484c63a7de0e09b32 Mon Sep 17 00:00:00 2001 From: Nikolay Shirokovskiy Date: Tue, 14 Jun 2016 11:45:57 +0300 Subject: [PATCH] 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 --- src/vz/vz_driver.c | 14 +--- src/vz/vz_sdk.c | 186 ++++++++++++++++++++++----------------------- src/vz/vz_sdk.h | 9 +-- src/vz/vz_utils.c | 24 ------ src/vz/vz_utils.h | 4 - 5 files changed, 95 insertions(+), 142 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 1bb99f069f..9a71c3b6a9 100644 --- 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 9495a36968..0662be1280 100644 --- 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,25 +1665,27 @@ 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 (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0) + if (!(def->name = prlsdkGetStringParamVar(PrlVmCfg_GetName, sdkdom))) goto error; + pret = prlsdkGetStringParamBuf(PrlVmCfg_GetUuid, + sdkdom, uuidstr, sizeof(uuidstr)); + prlsdkCheckRetGoto(pret, error); + + 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; def->onCrash = VIR_DOMAIN_LIFECYCLE_CRASH_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 a7dec565ff..262887351e 100644 --- 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 114738e6da..d3427caaf3 100644 --- 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 a9a8176952..92503a7ed4 100644 --- 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