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

28.01.2016 18:38, Mikhail Feoktistov пишет: > 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. prlsdkNewDomain() 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 prlsdkNewDomain() > 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 prlsdkNewDomain(). > 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 > prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain(). > --- > src/vz/vz_driver.c | 13 ++- > src/vz/vz_sdk.c | 253 > +++++++++++++++++++++++++++-------------------------- > src/vz/vz_sdk.h | 9 +- > 3 files changed, 146 insertions(+), 129 deletions(-) > > diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c > index 91a48b6..521efd4 100644 > --- a/src/vz/vz_driver.c > +++ b/src/vz/vz_driver.c > @@ -685,6 +685,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); > @@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const > char *xml, unsigned int flags) > olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid); > if (olddom == NULL) { > virResetLastError(); > + newdom = prlsdkNewDomain(privconn, def->name, def->uuid); > + if (!newdom) > + goto cleanup; > if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { > if (prlsdkCreateVm(conn, def)) > goto cleanup; > @@ -713,8 +717,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; > @@ -755,6 +758,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 6fb2a97..9d2bdab 100644 > --- a/src/vz/vz_sdk.c > +++ b/src/vz/vz_sdk.c > @@ -1236,58 +1236,35 @@ 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) > +int > +prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom) > { > - virDomainObjPtr dom = NULL; > 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; > + vzDomObjPtr pdom = dom->privateData; > virCheckNonNullArgGoto(privconn, error); > - virCheckNonNullArgGoto(sdkdom, error); > + virCheckNonNullArgGoto(dom, error); > + > + 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; > - } > + def->virtType = dom->def->virtType; > + def->id = dom->def->id; > - if (STREQ(privconn->drivername, "vz")) > - def->virtType = VIR_DOMAIN_VIRT_VZ; > - else > - def->virtType = VIR_DOMAIN_VIRT_PARALLELS; > - > - def->id = -1; > - > - if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0) > + if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid)) > goto error; > def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; > @@ -1318,29 +1295,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; > @@ -1363,38 +1344,6 @@ 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; > @@ -1404,24 +1353,27 @@ prlsdkLoadDomain(vzConnPtr privconn, > 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); > + /* 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; > - 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 > @@ -1429,11 +1381,13 @@ 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; > virDomainObjPtr dom; > + unsigned char uuid[VIR_UUID_BUFLEN]; > + char *name = NULL; > job = PrlSrv_GetVmListEx(privconn->server, PVTF_VM | PVTF_CT); > @@ -1447,61 +1401,89 @@ prlsdkLoadDomains(vzConnPtr privconn) > pret = PrlResult_GetParamByIndex(result, i, &sdkdom); > if (PRL_FAILED(pret)) { > logPrlError(pret); > - PrlHandle_Free(sdkdom); > goto error; > } > - dom = prlsdkLoadDomain(privconn, sdkdom, NULL); > - PrlHandle_Free(sdkdom); > + if (prlsdkGetDomainIds(sdkdom, &name, uuid)) > + goto error; > - if (!dom) > + if (!(dom = prlsdkNewDomain(privconn, name, uuid))) > goto error; > - else > - virObjectUnlock(dom); > + > + if (prlsdkLoadDomain(privconn, dom)) { > + virDomainObjListRemove(privconn->domains, dom); > + goto error; > + } > + > + VIR_FREE(name); > + PrlHandle_Free(sdkdom); > + sdkdom = PRL_INVALID_HANDLE; > + virObjectUnlock(dom); > } > PrlHandle_Free(result); > return 0; > error: > + VIR_FREE(name); > + 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); > +} > + > + > +virDomainObjPtr > +prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char > *uuid) > +{ Since this function doesn't communicate with prlsdk its name prefix looks strange to me. Also I would think about this function to vz_driver.c, but I don't insist on it.

> + virDomainDefPtr def = NULL; > + virDomainObjPtr dom = NULL; > + vzDomObjPtr pdom = NULL; > + > + if (!(def = virDomainDefNewFull(name, uuid, -1))) > + goto error; > + > + 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; > + } > + > + 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: > + virDomainDefFree(def); > + VIR_FREE(pdom); > + return NULL; > } > static int prlsdkSendEvent(vzConnPtr privconn, > @@ -1618,15 +1600,36 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn, > unsigned char *uuid) > { > virDomainObjPtr dom = NULL; > + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; > + char *name = NULL; > - 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 (prlsdkGetDomainIds(sdkdom, &name, NULL)) > + goto cleanup; > + > + if (!(dom = prlsdkNewDomain(privconn, name, uuid))) > + goto cleanup; > + > + if (prlsdkLoadDomain(privconn, dom)) { > + virDomainObjListRemove(privconn->domains, dom); > + dom = NULL; > + goto cleanup; > + } > + } > prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED, > VIR_DOMAIN_EVENT_DEFINED_ADDED); > - virObjectUnlock(dom); > + cleanup: > + if (dom) > + virObjectUnlock(dom); > + VIR_FREE(name); > + PrlHandle_Free(sdkdom); > return; > } > diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h > index ff6be07..f2d3e35 100644 > --- a/src/vz/vz_sdk.h > +++ b/src/vz/vz_sdk.h > @@ -30,9 +30,14 @@ 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); > +virDomainObjPtr > +prlsdkNewDomain(vzConnPtr privconn, > + char *name, > + const unsigned char *uuid); > +int > +prlsdkLoadDomain(vzConnPtr privconn, > + virDomainObjPtr dom); > int prlsdkSubscribeToPCSEvents(vzConnPtr privconn); > void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn); > PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);