[libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list

Race condition: User calls defineXML to create new instance. The main thread from vzDomainDefineXMLFlags() creates new instance by prlsdkCreateVm. Then this thread calls prlsdkAddDomain to add new domain to domains list. The second thread receives notification from hypervisor that new VM was created. It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains list. These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and don't find new domain. So they add two domains with the same uuid to domains list. This fix splits logic of prlsdkAddDomain() into two functions. 1. vzNewDomain() creates new empty domain in domains list with the specific uuid. 2. prlsdkLoadDomain() add data from VM to domain object. New algorithm for creating an instance: In vzDomainDefineXMLFlags() we add new domain to domain list by calling vzNewDomain() and only after that we call CreateVm() to create VM. It means that we "reserve" domain object with the specific uuid. After creation of new VM we add info from this VM to reserved domain object by calling prlsdkLoadDomain(). Before this patch prlsdkLoadDomain() worked in 2 different cases: 1. It creates and initializes new domain. Then updates it from sdk handle. 2. It updates existed domain from sdk handle. In this patch we remove code which creates new domain from LoadDomain() and move it to vzNewDomain(). Now prlsdkLoadDomain() only updates domain from skd handle. In notification handler prlsdkHandleVmAddedEvent() we check the existence of a domain and if it doesn't exist we add new domain by calling vzNewDomain() and load info from sdk handle via prlsdkLoadDomain(). --- src/vz/vz_driver.c | 13 +++- src/vz/vz_sdk.c | 217 ++++++++++++++++++++++------------------------------- src/vz/vz_sdk.h | 7 +- src/vz/vz_utils.c | 44 +++++++++++ src/vz/vz_utils.h | 4 + 5 files changed, 152 insertions(+), 133 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 925ff32..f7a8617 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -688,6 +688,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) virDomainPtr retdom = NULL; virDomainDefPtr def; virDomainObjPtr olddom = NULL; + virDomainObjPtr newdom = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -703,6 +704,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid); if (olddom == NULL) { virResetLastError(); + newdom = vzNewDomain(privconn, def->name, def->uuid); + if (!newdom) + goto cleanup; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (prlsdkCreateVm(conn, def)) goto cleanup; @@ -716,8 +720,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; } - olddom = prlsdkAddDomain(privconn, def->uuid); - if (!olddom) + if (prlsdkLoadDomain(privconn, newdom)) goto cleanup; } else { int state, reason; @@ -758,6 +761,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) cleanup: if (olddom) virObjectUnlock(olddom); + if (newdom) { + if (!retdom) + virDomainObjListRemove(privconn->domains, newdom); + else + virObjectUnlock(newdom); + } virDomainDefFree(def); vzDriverUnlock(privconn); return retdom; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 4423788..92bb7c5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -408,7 +408,7 @@ prlsdkGetDomainState(PRL_HANDLE sdkdom, VIRTUAL_MACHINE_STATE_PTR vmState) return ret; } -static void +void prlsdkDomObjFreePrivate(void *p) { vzDomObjPtr pdom = p; @@ -1241,56 +1241,58 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr def) return -1; } -/* - * This function retrieves information about domain. - * If the domains is already in the domains list - * privconn->domains, then locked 'olddom' must be - * provided. If the domains must be added to the list, - * olddom must be NULL. - * - * The function return a pointer to a locked virDomainObj. - */ static virDomainObjPtr -prlsdkLoadDomain(vzConnPtr privconn, - PRL_HANDLE sdkdom, - virDomainObjPtr olddom) +prlsdkNewDomainByHandle(vzConnPtr privconn, PRL_HANDLE sdkdom) { virDomainObjPtr dom = NULL; + unsigned char uuid[VIR_UUID_BUFLEN]; + char *name = NULL; + + if (prlsdkGetDomainIds(sdkdom, &name, uuid) < 0) + goto cleanup; + + if (!(dom = vzNewDomain(privconn, name, uuid))) + goto cleanup; + + if (prlsdkLoadDomain(privconn, dom) < 0) { + virDomainObjListRemove(privconn->domains, dom); + dom = NULL; + goto cleanup; + } + + cleanup: + VIR_FREE(name); + return dom; +} + +int +prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom) +{ virDomainDefPtr def = NULL; vzDomObjPtr pdom = NULL; VIRTUAL_MACHINE_STATE domainState; + char *home = NULL; PRL_UINT32 buflen = 0; PRL_RESULT pret; PRL_UINT32 ram; PRL_UINT32 envId; PRL_VM_AUTOSTART_OPTION autostart; + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; virCheckNonNullArgGoto(privconn, error); - virCheckNonNullArgGoto(sdkdom, error); + virCheckNonNullArgGoto(dom, error); + + pdom = dom->privateData; + sdkdom = prlsdkSdkDomainLookupByUUID(privconn, dom->def->uuid); + if (sdkdom == PRL_INVALID_HANDLE) + return -1; if (!(def = virDomainDefNew())) goto error; - if (!olddom) { - if (VIR_ALLOC(pdom) < 0) - goto error; - pdom->cache.stats = PRL_INVALID_HANDLE; - pdom->cache.count = -1; - if (virCondInit(&pdom->cache.cond) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize condition")); - goto error; - } - } else { - pdom = olddom->privateData; - } - - if (STREQ(privconn->drivername, "vz")) - def->virtType = VIR_DOMAIN_VIRT_VZ; - else - def->virtType = VIR_DOMAIN_VIRT_PARALLELS; - - def->id = -1; + def->virtType = dom->def->virtType; + def->id = dom->def->id; if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0) goto error; @@ -1323,29 +1325,33 @@ prlsdkLoadDomain(vzConnPtr privconn, pret = PrlVmCfg_GetEnvId(sdkdom, &envId); prlsdkCheckRetGoto(pret, error); - pdom->id = envId; buflen = 0; pret = PrlVmCfg_GetHomePath(sdkdom, NULL, &buflen); prlsdkCheckRetGoto(pret, error); - VIR_FREE(pdom->home); - if (VIR_ALLOC_N(pdom->home, buflen) < 0) + if (VIR_ALLOC_N(home, buflen) < 0) goto error; - pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen); + pret = PrlVmCfg_GetHomePath(sdkdom, home, &buflen); prlsdkCheckRetGoto(pret, error); - /* For VMs pdom->home is actually /directory/config.pvs */ + /* For VMs home is actually /directory/config.pvs */ if (!IS_CT(def)) { /* Get rid of /config.pvs in path string */ - char *s = strrchr(pdom->home, '/'); + char *s = strrchr(home, '/'); if (s) *s = '\0'; } pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart); prlsdkCheckRetGoto(pret, error); + if (autostart != PAO_VM_START_ON_LOAD && + autostart != PAO_VM_START_MANUAL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown autostart mode: %X"), autostart); + goto error; + } if (prlsdkGetDomainState(sdkdom, &domainState) < 0) goto error; @@ -1368,65 +1374,35 @@ prlsdkLoadDomain(vzConnPtr privconn, goto error; } - if (olddom) { - /* assign new virDomainDef without any checks */ - /* we can't use virDomainObjAssignDef, because it checks - * for state and domain name */ - dom = olddom; - virDomainDefFree(dom->def); - dom->def = def; - } else { - if (!(dom = virDomainObjListAdd(privconn->domains, def, - privconn->xmlopt, - 0, NULL))) - goto error; - } - /* dom is locked here */ - - dom->privateData = pdom; - dom->privateDataFreeFunc = prlsdkDomObjFreePrivate; - dom->persistent = 1; - - switch (autostart) { - case PAO_VM_START_ON_LOAD: - dom->autostart = 1; - break; - case PAO_VM_START_MANUAL: - dom->autostart = 0; - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown autostart mode: %X"), autostart); - goto error; - } - if (prlsdkConvertDomainState(domainState, envId, dom) < 0) goto error; + /* 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->id = envId; + VIR_FREE(pdom->home); + pdom->home = home; + if (!pdom->sdkdom) { - pret = PrlHandle_AddRef(sdkdom); - prlsdkCheckRetGoto(pret, error); + PrlHandle_AddRef(sdkdom); pdom->sdkdom = sdkdom; } - return dom; - error: - if (dom && !olddom) { - /* Domain isn't persistent means that we haven't yet set - * prlsdkDomObjFreePrivate and should call it manually - */ - if (!dom->persistent) - prlsdkDomObjFreePrivate(pdom); - - virDomainObjListRemove(privconn->domains, dom); - } - /* Delete newly allocated def only if we haven't assigned it to domain - * Otherwise we will end up with domain having invalid def within it - */ - if (!dom) - virDomainDefFree(def); + if (autostart == PAO_VM_START_ON_LOAD) + dom->autostart = 1; + else + dom->autostart = 0; - return NULL; + PrlHandle_Free(sdkdom); + return 0; + error: + PrlHandle_Free(sdkdom); + VIR_FREE(home); + virDomainDefFree(def); + return -1; } int @@ -1434,7 +1410,7 @@ prlsdkLoadDomains(vzConnPtr privconn) { PRL_HANDLE job = PRL_INVALID_HANDLE; PRL_HANDLE result; - PRL_HANDLE sdkdom; + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; PRL_UINT32 paramsCount; PRL_RESULT pret; size_t i = 0; @@ -1450,63 +1426,36 @@ prlsdkLoadDomains(vzConnPtr privconn) for (i = 0; i < paramsCount; i++) { pret = PrlResult_GetParamByIndex(result, i, &sdkdom); - if (PRL_FAILED(pret)) { - logPrlError(pret); - PrlHandle_Free(sdkdom); + prlsdkCheckRetGoto(pret, error); + + if (!(dom = prlsdkNewDomainByHandle(privconn, sdkdom))) goto error; - } - dom = prlsdkLoadDomain(privconn, sdkdom, NULL); + virObjectUnlock(dom); PrlHandle_Free(sdkdom); - - if (!dom) - goto error; - else - virObjectUnlock(dom); + sdkdom = PRL_INVALID_HANDLE; } PrlHandle_Free(result); return 0; error: + PrlHandle_Free(sdkdom); PrlHandle_Free(result); return -1; } -virDomainObjPtr -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid) -{ - PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; - virDomainObjPtr dom; - - dom = virDomainObjListFindByUUID(privconn->domains, uuid); - if (dom) { - /* domain is already in the list */ - return dom; - } - - sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid); - if (sdkdom == PRL_INVALID_HANDLE) - return NULL; - - dom = prlsdkLoadDomain(privconn, sdkdom, NULL); - PrlHandle_Free(sdkdom); - return dom; -} - int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom) { PRL_HANDLE job; - virDomainObjPtr retdom = NULL; vzDomObjPtr pdom = dom->privateData; job = PrlVm_RefreshConfig(pdom->sdkdom); if (waitJob(job)) return -1; - retdom = prlsdkLoadDomain(privconn, pdom->sdkdom, dom); - return retdom ? 0 : -1; + return prlsdkLoadDomain(privconn, dom); } static int prlsdkSendEvent(vzConnPtr privconn, @@ -1623,15 +1572,25 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn, unsigned char *uuid) { virDomainObjPtr dom = NULL; + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; - dom = prlsdkAddDomain(privconn, uuid); - if (dom == NULL) - return; + dom = virDomainObjListFindByUUID(privconn->domains, uuid); + if (!dom) { + sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid); + if (sdkdom == PRL_INVALID_HANDLE) + goto cleanup; + + if (!(dom = prlsdkNewDomainByHandle(privconn, sdkdom))) + goto cleanup; + } prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED, VIR_DOMAIN_EVENT_DEFINED_ADDED); - virObjectUnlock(dom); + cleanup: + if (dom) + virObjectUnlock(dom); + PrlHandle_Free(sdkdom); return; } diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h index ff6be07..c2d9cb4 100644 --- a/src/vz/vz_sdk.h +++ b/src/vz/vz_sdk.h @@ -30,9 +30,10 @@ int prlsdkConnect(vzConnPtr privconn); void prlsdkDisconnect(vzConnPtr privconn); int prlsdkLoadDomains(vzConnPtr privconn); -virDomainObjPtr -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid); int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom); +int +prlsdkLoadDomain(vzConnPtr privconn, + virDomainObjPtr dom); int prlsdkSubscribeToPCSEvents(vzConnPtr privconn); void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn); PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom); @@ -77,3 +78,5 @@ int prlsdkGetVcpuStats(virDomainObjPtr dom, int idx, unsigned long long *time); int prlsdkGetMemoryStats(virDomainObjPtr dom, virDomainMemoryStatPtr stats, unsigned int nr_stats); +void +prlsdkDomObjFreePrivate(void *p); diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 894f7dd..a1ddad0 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "virjson.h" #include "vz_utils.h" +#include "vz_sdk.h" #include "virstring.h" #include "datatypes.h" @@ -134,3 +135,46 @@ vzGetOutput(const char *binary, ...) return outbuf; } + +virDomainObjPtr +vzNewDomain(vzConnPtr privconn, char *name, const unsigned char *uuid) +{ + virDomainDefPtr def = NULL; + virDomainObjPtr dom = NULL; + vzDomObjPtr pdom = NULL; + + if (!(def = virDomainDefNewFull(name, uuid, -1))) + goto error; + + if (VIR_ALLOC(pdom) < 0) + goto error; + + if (virCondInit(&pdom->cache.cond) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize condition")); + goto error; + } + pdom->cache.stats = PRL_INVALID_HANDLE; + pdom->cache.count = -1; + + if (STREQ(privconn->drivername, "vz")) + def->virtType = VIR_DOMAIN_VIRT_VZ; + else + def->virtType = VIR_DOMAIN_VIRT_PARALLELS; + + if (!(dom = virDomainObjListAdd(privconn->domains, def, + privconn->xmlopt, + 0, NULL))) + goto error; + + dom->privateData = pdom; + dom->privateDataFreeFunc = prlsdkDomObjFreePrivate; + dom->persistent = 1; + return dom; + + error: + if (pdom && pdom->cache.count == -1) + virCondDestroy(&pdom->cache.cond); + virDomainDefFree(def); + VIR_FREE(pdom); + return NULL; +} diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index 417f821..0976193 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -90,6 +90,10 @@ char * vzGetOutput(const char *binary, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; void vzDriverLock(vzConnPtr driver); void vzDriverUnlock(vzConnPtr driver); +virDomainObjPtr +vzNewDomain(vzConnPtr privconn, + char *name, + const unsigned char *uuid); # define PARALLELS_BLOCK_STATS_FOREACH(OP) \ OP(rd_req, VIR_DOMAIN_BLOCK_STATS_READ_REQ, "read_requests") \ -- 1.8.3.1