[libvirt] [PATCH 0/9] vz: define and other cleanups
'vz: cleanup loading domain code' is main patch and 'vz: use domain list infrastructure to deal with private domain' is a preparation for it. Other patches are mostly minor bugfixes and cleanups. After the main patch driver lock is locked for very small time so massively parallel domain definition is not serialized by vz driver anymore. NOTE: Applied on top of: [PATCH] vz: get rid of unused home state variable in private domain obj [PATCH 0/5] fix destination domain synchronization Nikolay Shirokovskiy (9): vz: remove unnecessary labels in simple API calls vz: restore accidentally removed locks around close callback calls vz: fix leaks in prlsdkCreate* functions vz: make error handling idiomatic in prlsdkCreateVm vz: use domain list infrastructure to deal with private domain vz: cleanup loading domain code vz: dont remove domain from list on client object error vz: use single variable for domain vz: don't fail unregister on sending event error src/vz/vz_driver.c | 154 +++++++++++++---------------------- src/vz/vz_sdk.c | 229 ++++++++++++++++++++++++----------------------------- src/vz/vz_sdk.h | 11 +-- src/vz/vz_utils.c | 58 ++++++-------- src/vz/vz_utils.h | 7 +- 5 files changed, 194 insertions(+), 265 deletions(-) -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 103 ++++++++++++++++++----------------------------------- 1 file changed, 34 insertions(+), 69 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index e2a0d1f..0350b7d 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -528,23 +528,21 @@ static virDomainPtr vzDomainLookupByID(virConnectPtr conn, int id) { vzConnPtr privconn = conn->privateData; - virDomainPtr ret = NULL; + virDomainPtr ret; virDomainObjPtr dom; dom = virDomainObjListFindByID(privconn->driver->domains, id); if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; + return NULL; } ret = virGetDomain(conn, dom->def->name, dom->def->uuid); if (ret) ret->id = dom->def->id; - cleanup: - if (dom) - virObjectUnlock(dom); + virObjectUnlock(dom); return ret; } @@ -552,7 +550,7 @@ static virDomainPtr vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { vzConnPtr privconn = conn->privateData; - virDomainPtr ret = NULL; + virDomainPtr ret; virDomainObjPtr dom; dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); @@ -562,16 +560,14 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return NULL; } ret = virGetDomain(conn, dom->def->name, dom->def->uuid); if (ret) ret->id = dom->def->id; - cleanup: - if (dom) - virObjectUnlock(dom); + virObjectUnlock(dom); return ret; } @@ -579,7 +575,7 @@ static virDomainPtr vzDomainLookupByName(virConnectPtr conn, const char *name) { vzConnPtr privconn = conn->privateData; - virDomainPtr ret = NULL; + virDomainPtr ret; virDomainObjPtr dom; dom = virDomainObjListFindByName(privconn->driver->domains, name); @@ -587,14 +583,13 @@ vzDomainLookupByName(virConnectPtr conn, const char *name) if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching name '%s'"), name); - goto cleanup; + return NULL; } ret = virGetDomain(conn, dom->def->name, dom->def->uuid); if (ret) ret->id = dom->def->id; - cleanup: virDomainObjEndAPI(&dom); return ret; } @@ -641,17 +636,14 @@ static char * vzDomainGetOSType(virDomainPtr domain) { virDomainObjPtr dom; - char *ret = NULL; if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return NULL; ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(dom->def->os.type))); - cleanup: - if (dom) - virObjectUnlock(dom); + virObjectUnlock(dom); return ret; } @@ -659,17 +651,12 @@ static int vzDomainIsPersistent(virDomainPtr domain) { virDomainObjPtr dom; - int ret = -1; if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return -1; - ret = 1; - - cleanup: - if (dom) - virObjectUnlock(dom); - return ret; + virObjectUnlock(dom); + return 1; } static int @@ -677,19 +664,16 @@ vzDomainGetState(virDomainPtr domain, int *state, int *reason, unsigned int flags) { virDomainObjPtr dom; - int ret = -1; + virCheckFlags(0, -1); if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return -1; *state = virDomainObjGetState(dom, reason); - ret = 0; - cleanup: - if (dom) - virObjectUnlock(dom); - return ret; + virObjectUnlock(dom); + return 0; } static char * @@ -703,16 +687,14 @@ vzDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) /* Flags checked by virDomainDefFormat */ if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return NULL; def = (flags & VIR_DOMAIN_XML_INACTIVE) && dom->newDef ? dom->newDef : dom->def; ret = virDomainDefFormat(def, privconn->driver->caps, flags); - cleanup: - if (dom) - virObjectUnlock(dom); + virObjectUnlock(dom); return ret; } @@ -720,18 +702,14 @@ static int vzDomainGetAutostart(virDomainPtr domain, int *autostart) { virDomainObjPtr dom; - int ret = -1; if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return -1; *autostart = dom->autostart; - ret = 0; - cleanup: - if (dom) - virObjectUnlock(dom); - return ret; + virObjectUnlock(dom); + return 0; } static virDomainPtr @@ -882,7 +860,7 @@ vzDomainGetVcpus(virDomainPtr domain, int ret = -1; if (!(dom = vzDomObjFromDomainRef(domain))) - goto cleanup; + return -1; if (!virDomainObjIsActive(dom)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -916,8 +894,7 @@ vzDomainGetVcpus(virDomainPtr domain, ret = maxinfo; cleanup: - if (dom) - virDomainObjEndAPI(&dom); + virDomainObjEndAPI(&dom); return ret; } @@ -954,17 +931,13 @@ vzConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { vzConnPtr privconn = conn->privateData; - int ret = -1; if (virObjectEventStateDeregisterID(conn, privconn->driver->domainEventState, callbackID) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - return ret; + return 0; } static int vzDomainSuspend(virDomainPtr domain) @@ -1346,8 +1319,7 @@ vzDomainBlockStats(virDomainPtr domain, const char *path, ret = 0; cleanup: - if (dom) - virDomainObjEndAPI(&dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1448,24 +1420,22 @@ static int vzDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) { - virDomainObjPtr dom = NULL; - int ret = -1; + virDomainObjPtr dom; + int ret; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return -1; if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = virDomainDefGetVcpusMax(dom->def); else ret = virDomainDefGetVcpus(dom->def); - cleanup: - if (dom) - virObjectUnlock(dom); + virObjectUnlock(dom); return ret; } @@ -1479,19 +1449,14 @@ static int vzDomainGetMaxVcpus(virDomainPtr domain) static int vzDomainIsUpdated(virDomainPtr domain) { virDomainObjPtr dom; - int ret = -1; /* As far as VZ domains are always updated (e.g. current==persistent), * we just check for domain existence */ if (!(dom = vzDomObjFromDomain(domain))) - goto cleanup; + return -1; - ret = 0; - - cleanup: - if (dom) - virObjectUnlock(dom); - return ret; + virObjectUnlock(dom); + return 0; } static int vzConnectGetMaxVcpus(virConnectPtr conn ATTRIBUTE_UNUSED, -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 103 ++++++++++++++++++----------------------------------- 1 file changed, 34 insertions(+), 69 deletions(-)
I'm not sure it is necessary, but will not object pushing this not to violate your intents to make the world better. Thus, ACK
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 0350b7d..01dd204 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1519,6 +1519,8 @@ vzConnectRegisterCloseCallback(virConnectPtr conn, vzConnPtr privconn = conn->privateData; int ret = -1; + virObjectLock(privconn->driver); + if (virConnectCloseCallbackDataGetCallback(privconn->closeCallback) != NULL) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("A close callback is already registered")); @@ -1530,6 +1532,7 @@ vzConnectRegisterCloseCallback(virConnectPtr conn, ret = 0; cleanup: + virObjectUnlock(privconn->driver); return ret; } @@ -1540,6 +1543,7 @@ vzConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb) vzConnPtr privconn = conn->privateData; int ret = -1; + virObjectLock(privconn->driver); if (virConnectCloseCallbackDataGetCallback(privconn->closeCallback) != cb) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -1551,6 +1555,7 @@ vzConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb) ret = 0; cleanup: + virObjectUnlock(privconn->driver); return ret; } -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 5 +++++ 1 file changed, 5 insertions(+) ACK
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 603a965..563fc6c 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3743,6 +3743,9 @@ prlsdkCreateVm(vzDriverPtr driver, virDomainDefPtr def) cleanup: PrlHandle_Free(sdkdom); + PrlHandle_Free(srvconf); + PrlHandle_Free(result); + return ret; } @@ -3809,6 +3812,7 @@ prlsdkCreateCt(vzDriverPtr driver, virDomainDefPtr def) cleanup: PrlHandle_Free(sdkdom); + PrlHandle_Free(result); return ret; } -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 563fc6c..40979c6 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3733,13 +3733,14 @@ prlsdkCreateVm(vzDriverPtr driver, virDomainDefPtr def) pret = PrlVmCfg_SetOfflineManagementEnabled(sdkdom, 0); prlsdkCheckRetGoto(pret, cleanup); - ret = prlsdkDoApplyConfig(driver, sdkdom, def, NULL); - if (ret) + if (prlsdkDoApplyConfig(driver, sdkdom, def, NULL) < 0) goto cleanup; job = PrlVm_Reg(sdkdom, "", 1); if (PRL_FAILED(waitJob(job))) - ret = -1; + goto cleanup; + + ret = 0; cleanup: PrlHandle_Free(sdkdom); -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
ACK
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 7 ++++++- src/vz/vz_sdk.c | 13 ------------- src/vz/vz_sdk.h | 2 -- src/vz/vz_utils.c | 34 ++++++++++++++++++++++++++-------- src/vz/vz_utils.h | 3 +++ 5 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 01dd204..8abf6f1 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -294,6 +294,10 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return ret; } +static virDomainXMLPrivateDataCallbacks vzDomainXMLPrivateDataCallbacksPtr = { + .alloc = vzDomObjAlloc, + .free = vzDomObjFree, +}; static virDomainDefParserConfig vzDomainDefParserConfig = { .macPrefix = {0x42, 0x1C, 0x00}, @@ -316,7 +320,8 @@ vzDriverObjNew(void) if (!(driver->caps = vzBuildCapabilities()) || !(driver->xmlopt = virDomainXMLOptionNew(&vzDomainDefParserConfig, - NULL, NULL)) || + &vzDomainXMLPrivateDataCallbacksPtr, + NULL)) || !(driver->domains = virDomainObjListNew()) || !(driver->domainEventState = virObjectEventStateNew()) || (vzInitVersion(driver) < 0) || diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 40979c6..d1eb6f6 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -470,19 +470,6 @@ prlsdkGetDomainState(PRL_HANDLE sdkdom, VIRTUAL_MACHINE_STATE_PTR vmState) return ret; } -void -prlsdkDomObjFreePrivate(void *p) -{ - vzDomObjPtr pdom = p; - - if (!pdom) - return; - - PrlHandle_Free(pdom->sdkdom); - PrlHandle_Free(pdom->stats); - VIR_FREE(p); -}; - static int prlsdkAddDomainVideoInfo(PRL_HANDLE sdkdom, virDomainDefPtr def) { diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h index 03c6aa1..9d143ce 100644 --- a/src/vz/vz_sdk.h +++ b/src/vz/vz_sdk.h @@ -78,8 +78,6 @@ int prlsdkGetVcpuStats(PRL_HANDLE sdkstas, int idx, unsigned long long *time); int prlsdkGetMemoryStats(PRL_HANDLE sdkstas, virDomainMemoryStatPtr stats, unsigned int nr_stats); -void -prlsdkDomObjFreePrivate(void *p); /* memsize is in MiB */ int prlsdkSetMemsize(virDomainObjPtr dom, unsigned int memsize); int diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index db23b72..eef9db2 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -164,15 +164,10 @@ vzNewDomain(vzDriverPtr driver, const 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; - - pdom->stats = PRL_INVALID_HANDLE; def->virtType = VIR_DOMAIN_VIRT_VZ; if (!(dom = virDomainObjListAdd(driver->domains, def, @@ -180,14 +175,11 @@ vzNewDomain(vzDriverPtr driver, const char *name, const unsigned char *uuid) 0, NULL))) goto error; - dom->privateData = pdom; - dom->privateDataFreeFunc = prlsdkDomObjFreePrivate; dom->persistent = 1; return dom; error: virDomainDefFree(def); - VIR_FREE(pdom); return NULL; } @@ -481,3 +473,29 @@ int vzGetDefaultSCSIModel(vzDriverPtr driver, } return 0; } + +void* +vzDomObjAlloc(void) +{ + vzDomObjPtr pdom = NULL; + + if (VIR_ALLOC(pdom) < 0) + return NULL; + + pdom->stats = PRL_INVALID_HANDLE; + + return pdom; +} + +void +vzDomObjFree(void* p) +{ + vzDomObjPtr pdom = p; + + if (!pdom) + return; + + PrlHandle_Free(pdom->sdkdom); + PrlHandle_Free(pdom->stats); + VIR_FREE(pdom); +}; diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index a65ad21..9069340 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -102,6 +102,9 @@ struct vzDomObj { typedef struct vzDomObj *vzDomObjPtr; +void* vzDomObjAlloc(void); +void vzDomObjFree(void *p); + virDomainObjPtr vzDomObjFromDomain(virDomainPtr domain); virDomainObjPtr vzDomObjFromDomainRef(virDomainPtr domain); -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 7 ++++++- src/vz/vz_sdk.c | 13 ------------- src/vz/vz_sdk.h | 2 -- src/vz/vz_utils.c | 34 ++++++++++++++++++++++++++-------- src/vz/vz_utils.h | 3 +++ 5 files changed, 35 insertions(+), 24 deletions(-)
ACK
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> --- src/vz/vz_driver.c | 14 +--- src/vz/vz_sdk.c | 184 +++++++++++++++++++++++++---------------------------- src/vz/vz_sdk.h | 9 ++- src/vz/vz_utils.c | 24 ------- src/vz/vz_utils.h | 4 -- 5 files changed, 94 insertions(+), 141 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 8abf6f1..545dc79 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -733,7 +733,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; @@ -741,9 +740,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; @@ -757,7 +753,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; @@ -805,7 +801,6 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) virObjectUnlock(newdom); } virDomainDefFree(def); - virObjectUnlock(driver); return retdom; } @@ -2626,7 +2621,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); @@ -2656,11 +2650,8 @@ vzDomainMigrateFinish3Params(virConnectPtr dconn, 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); @@ -2674,7 +2665,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 d1eb6f6..612a059 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -412,37 +412,6 @@ prlsdkUUIDParse(const char *uuidstr, unsigned char *uuid) } 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) { PRL_HANDLE job = PRL_INVALID_HANDLE; @@ -1242,36 +1211,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) { @@ -1495,8 +1434,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; @@ -1506,24 +1453,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; @@ -1588,20 +1537,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); @@ -1611,12 +1578,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 @@ -1642,7 +1608,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); @@ -1658,6 +1624,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) { @@ -1668,7 +1666,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, @@ -1787,15 +1785,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 9d143ce..e32001a 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); @@ -97,5 +98,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 eef9db2..b629bc3 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 9069340..20ba1fd 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 -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
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> --- src/vz/vz_driver.c | 14 +--- src/vz/vz_sdk.c | 184 +++++++++++++++++++++++++---------------------------- src/vz/vz_sdk.h | 9 ++- src/vz/vz_utils.c | 24 ------- src/vz/vz_utils.h | 4 -- 5 files changed, 94 insertions(+), 141 deletions(-)
Yeah, looks reasonable and works fine. ACK
After domain is in the domains list let's keep it there. This is approach taken by qemu driver and vz vzDomainMigrateFinish3Params too. It quite reasonable, driver domain object is fully constructed and can be discovered by client later. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 545dc79..de06443 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -794,12 +794,8 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) cleanup: if (olddom) virObjectUnlock(olddom); - if (newdom) { - if (!retdom) - virDomainObjListRemove(driver->domains, newdom); - else - virObjectUnlock(newdom); - } + if (newdom) + virObjectUnlock(newdom); virDomainDefFree(def); return retdom; } -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
After domain is in the domains list let's keep it there. This is approach taken by qemu driver and vz vzDomainMigrateFinish3Params too. It quite reasonable, driver domain object is fully constructed and can be discovered by client later.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
ACK
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index de06443..a131aa1 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -723,8 +723,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) vzConnPtr privconn = conn->privateData; virDomainPtr retdom = NULL; virDomainDefPtr def; - virDomainObjPtr olddom = NULL; - virDomainObjPtr newdom = NULL; + virDomainObjPtr dom = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; vzDriverPtr driver = privconn->driver; @@ -737,8 +736,8 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) parse_flags)) == NULL) goto cleanup; - olddom = virDomainObjListFindByUUID(driver->domains, def->uuid); - if (olddom == NULL) { + dom = virDomainObjListFindByUUID(driver->domains, def->uuid); + if (dom == NULL) { virResetLastError(); if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (prlsdkCreateVm(driver, def)) @@ -753,12 +752,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; } - if (!(newdom = prlsdkAddDomainByUUID(driver, def->uuid))) + if (!(dom = prlsdkAddDomainByUUID(driver, def->uuid))) goto cleanup; } else { int state, reason; - state = virDomainObjGetState(olddom, &reason); + state = virDomainObjGetState(dom, &reason); if (state == VIR_DOMAIN_SHUTOFF && reason == VIR_DOMAIN_SHUTOFF_SAVED) { @@ -772,17 +771,17 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) * So forbid this operation, if config is changed. If it's * not changed - just do nothing. */ - if (!virDomainDefCheckABIStability(olddom->def, def)) { + if (!virDomainDefCheckABIStability(dom->def, def)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Can't change domain configuration " "in managed save state")); goto cleanup; } } else { - if (prlsdkApplyConfig(driver, olddom, def)) + if (prlsdkApplyConfig(driver, dom, def)) goto cleanup; - if (prlsdkUpdateDomain(driver, olddom)) + if (prlsdkUpdateDomain(driver, dom)) goto cleanup; } } @@ -792,10 +791,8 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) retdom->id = def->id; cleanup: - if (olddom) - virObjectUnlock(olddom); - if (newdom) - virObjectUnlock(newdom); + if (dom) + virObjectUnlock(dom); virDomainDefFree(def); return retdom; } -- 1.8.3.1
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) ACK
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 612a059..12691ba 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1669,21 +1669,19 @@ prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom) return prlsdkLoadDomain(driver, pdom->sdkdom, dom) ? 0 : -1; } -static int prlsdkSendEvent(vzDriverPtr driver, - virDomainObjPtr dom, - virDomainEventType lvEventType, - int lvEventTypeDetails) +static void +prlsdkSendEvent(vzDriverPtr driver, + virDomainObjPtr dom, + virDomainEventType lvEventType, + int lvEventTypeDetails) { - virObjectEventPtr event = NULL; + virObjectEventPtr event; event = virDomainEventLifecycleNewFromObj(dom, lvEventType, lvEventTypeDetails); if (!event) - return -1; - - virObjectEventStateQueue(driver->domainEventState, event); - return 0; + virObjectEventStateQueue(driver->domainEventState, event); } static void @@ -3887,9 +3885,8 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla for (i = 0; i < dom->def->nnets; i++) prlsdkCleanupBridgedNet(driver, dom->def->nets[i]); - if (prlsdkSendEvent(driver, dom, VIR_DOMAIN_EVENT_UNDEFINED, - VIR_DOMAIN_EVENT_UNDEFINED_REMOVED) < 0) - goto cleanup; + prlsdkSendEvent(driver, dom, VIR_DOMAIN_EVENT_UNDEFINED, + VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); -- 1.8.3.1
On 14.06.2016 11:46, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 612a059..12691ba 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1669,21 +1669,19 @@ prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom) return prlsdkLoadDomain(driver, pdom->sdkdom, dom) ? 0 : -1; }
-static int prlsdkSendEvent(vzDriverPtr driver, - virDomainObjPtr dom, - virDomainEventType lvEventType, - int lvEventTypeDetails) +static void +prlsdkSendEvent(vzDriverPtr driver, + virDomainObjPtr dom, + virDomainEventType lvEventType, + int lvEventTypeDetails) { - virObjectEventPtr event = NULL; + virObjectEventPtr event;
event = virDomainEventLifecycleNewFromObj(dom, lvEventType, lvEventTypeDetails); if (!event) - return -1;
Phhh, if (event) of course.
- - virObjectEventStateQueue(driver->domainEventState, event); - return 0; + virObjectEventStateQueue(driver->domainEventState, event); }
static void @@ -3887,9 +3885,8 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla for (i = 0; i < dom->def->nnets; i++) prlsdkCleanupBridgedNet(driver, dom->def->nets[i]);
- if (prlsdkSendEvent(driver, dom, VIR_DOMAIN_EVENT_UNDEFINED, - VIR_DOMAIN_EVENT_UNDEFINED_REMOVED) < 0) - goto cleanup; + prlsdkSendEvent(driver, dom, VIR_DOMAIN_EVENT_UNDEFINED, + VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
virDomainObjListRemove(driver->domains, dom);
14-Jun-16 12:09, Nikolay Shirokovskiy пишет:
On 14.06.2016 11:46, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 612a059..12691ba 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1669,21 +1669,19 @@ prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom) return prlsdkLoadDomain(driver, pdom->sdkdom, dom) ? 0 : -1; }
-static int prlsdkSendEvent(vzDriverPtr driver, - virDomainObjPtr dom, - virDomainEventType lvEventType, - int lvEventTypeDetails) +static void +prlsdkSendEvent(vzDriverPtr driver, + virDomainObjPtr dom, + virDomainEventType lvEventType, + int lvEventTypeDetails) { - virObjectEventPtr event = NULL; + virObjectEventPtr event;
event = virDomainEventLifecycleNewFromObj(dom, lvEventType, lvEventTypeDetails); if (!event) - return -1; Phhh, if (event) of course.
ACK with phhh
14-Jun-16 11:45, Nikolay Shirokovskiy пишет:
'vz: cleanup loading domain code' is main patch and 'vz: use domain list infrastructure to deal with private domain' is a preparation for it. Other patches are mostly minor bugfixes and cleanups.
After the main patch driver lock is locked for very small time so massively parallel domain definition is not serialized by vz driver anymore.
NOTE: Applied on top of: [PATCH] vz: get rid of unused home state variable in private domain obj [PATCH 0/5] fix destination domain synchronization
Nikolay Shirokovskiy (9): vz: remove unnecessary labels in simple API calls vz: restore accidentally removed locks around close callback calls vz: fix leaks in prlsdkCreate* functions vz: make error handling idiomatic in prlsdkCreateVm vz: use domain list infrastructure to deal with private domain vz: cleanup loading domain code vz: dont remove domain from list on client object error vz: use single variable for domain vz: don't fail unregister on sending event error
src/vz/vz_driver.c | 154 +++++++++++++---------------------- src/vz/vz_sdk.c | 229 ++++++++++++++++++++++++----------------------------- src/vz/vz_sdk.h | 11 +-- src/vz/vz_utils.c | 58 ++++++-------- src/vz/vz_utils.h | 7 +- 5 files changed, 194 insertions(+), 265 deletions(-)
Pushed the series. Thanks! Maxim
participants (2)
-
Maxim Nestratov -
Nikolay Shirokovskiy