[libvirt] [PATCH 0/3] vz: make vz driver more responsive
Patches 1 and 2 are tiny fixes and cleanups. 3 has the essence. Nikolay Shirokovskiy (3): vz: use state variable sdkdom in prlsdkApplyConfig vz: remove redundant variable in prlsdkHandleVmAddedEvent vz: make vz driver more responsive src/vz/vz_driver.c | 183 ++++++++++++++++++++++++++++++++++--------------- src/vz/vz_sdk.c | 195 ++++++++++++++++++++++++++++++++++------------------- src/vz/vz_utils.c | 50 ++++++++++++++ src/vz/vz_utils.h | 7 ++ 4 files changed, 310 insertions(+), 125 deletions(-) -- 1.8.3.1
sdk domain handle is unique per connection so there is no sense to query it again if we have it in vzDomObjPtr. Side effect of prlsdkSdkDomainLookupByUUID is refreshing domain config is of no use too as PrlVm_BeginEdit do it too. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 12691ba..092954d 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3659,28 +3659,22 @@ prlsdkApplyConfig(vzDriverPtr driver, virDomainObjPtr dom, virDomainDefPtr new) { - PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; + vzDomObjPtr privdom = dom->privateData; PRL_HANDLE job = PRL_INVALID_HANDLE; int ret; - sdkdom = prlsdkSdkDomainLookupByUUID(driver, dom->def->uuid); - if (sdkdom == PRL_INVALID_HANDLE) - return -1; - - job = PrlVm_BeginEdit(sdkdom); + job = PrlVm_BeginEdit(privdom->sdkdom); if (PRL_FAILED(waitJob(job))) return -1; - ret = prlsdkDoApplyConfig(driver, sdkdom, new, dom->def); + ret = prlsdkDoApplyConfig(driver, privdom->sdkdom, new, dom->def); if (ret == 0) { - job = PrlVm_CommitEx(sdkdom, PVCF_DETACH_HDD_BUNDLE); + job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); if (PRL_FAILED(waitJob(job))) ret = -1; } - PrlHandle_Free(sdkdom); - return ret; } -- 1.8.3.1
16-Jun-16 14:59, Nikolay Shirokovskiy пишет:
sdk domain handle is unique per connection so there is no sense to query it again if we have it in vzDomObjPtr. Side effect of prlsdkSdkDomainLookupByUUID is refreshing domain config is of no use too as PrlVm_BeginEdit do it too.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) ACK
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 092954d..c9f89ab 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1781,7 +1781,6 @@ prlsdkHandleVmAddedEvent(vzDriverPtr driver, unsigned char *uuid) { virDomainObjPtr dom = NULL; - PRL_HANDLE sdkdom = PRL_INVALID_HANDLE; if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)) && !(dom = prlsdkAddDomainByUUID(driver, uuid))) @@ -1793,7 +1792,6 @@ prlsdkHandleVmAddedEvent(vzDriverPtr driver, cleanup: if (dom) virObjectUnlock(dom); - PrlHandle_Free(sdkdom); return; } -- 1.8.3.1
Current vz driver implementation is not usable when it comes to long runnig operations. Migration or saving a domain blocks all other operations even query ones which are expecteted to be available. This patch address this problem. All vz driver API calls fall into next 3 groups: 1. only query domain cache (virDomainObj, vz cache statistic) examples are vzDomainGetState, vzDomainGetXMLDesc etc. 2. use thread shared sdkdom object examples are vzDomainSetMemoryFlags, vzDomainAttachDevice etc. 3. use no thread shared sdkdom object nor domain cache examples are vzDomainSnapshotListNames, vzDomainSnapshotGetXMLDesc etc API calls from group 1 don't need to be changed as they hold domain lock only for short period of time. This calls [1] are easily distinguished. They query domain object thru libvirt common code or query vz sdk statistics handle thru vz sdk sync operations. vzDomainInterfaceStats is the only exception. It uses sdkdom object to convert interface name to its vz sdk stack index which could not be saved in domain cache. Interface statistic is available thru this stack index as a key rather than name. As a result we can have accidental 'not known interface' errors on quering intrerface stats. The reason is that in the process of updating domain configuration we drop all devices and then recreate them again in sdkdom object and domain lock can be dropped meanwhile (to remove networks for existing bridged interfaces and(or) (re)create new ones). We can fix this by changing the way we support bridged interfaces or by reordering operations and changing bridged networks beforehand. Anyway this is better then moving this API call into 2 group and making it an exclusive job. As to API calls from group 2 first thread shared sdkdom object need to be explained. vz sdk has only one handle for given domain thus threads needs exclusive access to operate on it. These calls are fixed to drop and reacquire domain lock on any lengthy operations - namely waiting the result of async vz sdk operation. As lock is dropped we need to take extra reference to domain object if it is not taken already as domain object can be deleted from list while lock is dropped. As this operations use thread shared sdkdom object the simpliest way to make calls from group 2 consistent to each other is to make them mutually exclusive. This is done by taking/releasing job condition thru calling correspondent job routine. This approach makes group 1 and group 2 calls consistent to each other too. Not all calls of group 2 change the domain cache but those that do update it thru prlsdkUpdateDomain which hold the lock thoughout the update. API calls from group [2] are easily distinguished too. They use beginEdit/commit to change domain configuration (vzDomainSetMemoryFlags) or/and update domain cache from sdkdom at the end of operation (vzDomainSuspend). There is a known issue however. Frankly speaking it is introduced by ealier patch '[PATCH 6/9] vz: cleanup loading domain code' from a different series. The patch significantly reduces amount of time when driver lock is hold when creating domain from API call or as a result of domain added event from vz sdk. The problem is these two paths race on using thread shared sdkdom as we don't have libvirt domain object and can not lock on it. However this don't invalidates the patch as we can't use the former approach of preadding domain into the list as we need name at least and name is not given by event. Anyway i'm against adding half baked object into the list. Eventually this race can be fixed by extra measures. As to current situation races with different configurations are unlikely and race when adding domain thru vz driver and simultaneous event from vz sdk is not dangerous as configuration is the same. The last group [3] is API calls that need only sdkdom object to make vz sdk call which don't change thread shared sdkdom object or domain cache in any way. For now these are mostly domain snapshot API calls. The changes are similar to those of group 2 - add extra reference and drop/reacquire the lock on waiting vz async call result. One can simply take the immutable sdkdom object from the cache and drop the lock for the rest of operations but the chosen approach makes implementation of these API calls somewhat similar to those of from group 2 and thus a bit futureproof. As calls of group 3 don't need vz driver domain/vz sdk cache in any way they consistent with respect to API calls from groups 1 and 3. There is another exception. Calls to make-snapshot/revert-to-snapshot/migrate are moved to group 2. That is they are made mutually exclusive. The reason is that libvirt API supports control/query only for one job per domain and these are jobs that are likely to be queried/aborted. Appendix. [1] API calls that only query domain cache. (marked [*] are included for a different reason) .domainLookupByID = vzDomainLookupByID, /* 0.10.0 */ .domainLookupByUUID = vzDomainLookupByUUID, /* 0.10.0 */ .domainLookupByName = vzDomainLookupByName, /* 0.10.0 */ .domainGetOSType = vzDomainGetOSType, /* 0.10.0 */ .domainGetInfo = vzDomainGetInfo, /* 0.10.0 */ .domainGetState = vzDomainGetState, /* 0.10.0 */ .domainGetXMLDesc = vzDomainGetXMLDesc, /* 0.10.0 */ .domainIsPersistent = vzDomainIsPersistent, /* 0.10.0 */ .domainGetAutostart = vzDomainGetAutostart, /* 0.10.0 */ .domainGetVcpus = vzDomainGetVcpus, /* 1.2.6 */ .domainIsActive = vzDomainIsActive, /* 1.2.10 */ .domainIsUpdated = vzDomainIsUpdated, /* 1.2.21 */ .domainGetVcpusFlags = vzDomainGetVcpusFlags, /* 1.2.21 */ .domainGetMaxVcpus = vzDomainGetMaxVcpus, /* 1.2.21 */ .domainHasManagedSaveImage = vzDomainHasManagedSaveImage, /* 1.2.13 */ .domainGetMaxMemory = vzDomainGetMaxMemory, /* 1.2.15 */ .domainBlockStats = vzDomainBlockStats, /* 1.2.17 */ .domainBlockStatsFlags = vzDomainBlockStatsFlags, /* 1.2.17 */ .domainInterfaceStats = vzDomainInterfaceStats, /* 1.2.17 */ [*] .domainMemoryStats = vzDomainMemoryStats, /* 1.2.17 */ .domainMigrateBegin3Params = vzDomainMigrateBegin3Params, /* 1.3.5 */ .domainMigrateConfirm3Params = vzDomainMigrateConfirm3Params, /* 1.3.5 */ [2] API calls that use thread shared sdkdom object (marked [*] are included for a different reason) .domainSuspend = vzDomainSuspend, /* 0.10.0 */ .domainResume = vzDomainResume, /* 0.10.0 */ .domainDestroy = vzDomainDestroy, /* 0.10.0 */ .domainShutdown = vzDomainShutdown, /* 0.10.0 */ .domainCreate = vzDomainCreate, /* 0.10.0 */ .domainCreateWithFlags = vzDomainCreateWithFlags, /* 1.2.10 */ .domainReboot = vzDomainReboot, /* 1.3.0 */ .domainDefineXML = vzDomainDefineXML, /* 0.10.0 */ .domainDefineXMLFlags = vzDomainDefineXMLFlags, /* 1.2.12 */ (update part) .domainUndefine = vzDomainUndefine, /* 1.2.10 */ .domainAttachDevice = vzDomainAttachDevice, /* 1.2.15 */ .domainAttachDeviceFlags = vzDomainAttachDeviceFlags, /* 1.2.15 */ .domainDetachDevice = vzDomainDetachDevice, /* 1.2.15 */ .domainDetachDeviceFlags = vzDomainDetachDeviceFlags, /* 1.2.15 */ .domainSetUserPassword = vzDomainSetUserPassword, /* 1.3.6 */ .domainManagedSave = vzDomainManagedSave, /* 1.2.14 */ .domainSetMemoryFlags = vzDomainSetMemoryFlags, /* 1.3.4 */ .domainSetMemory = vzDomainSetMemory, /* 1.3.4 */ .domainRevertToSnapshot = vzDomainRevertToSnapshot, /* 1.3.5 */ [*] .domainSnapshotCreateXML = vzDomainSnapshotCreateXML, /* 1.3.5 */ [*] .domainMigratePerform3Params = vzDomainMigratePerform3Params, /* 1.3.5 */ [*] prlsdkHandleVmConfigEvent [3] API calls that do not use thread shared sdkdom object .domainManagedSaveRemove = vzDomainManagedSaveRemove, /* 1.2.14 */ .domainSnapshotNum = vzDomainSnapshotNum, /* 1.3.5 */ .domainSnapshotListNames = vzDomainSnapshotListNames, /* 1.3.5 */ .domainListAllSnapshots = vzDomainListAllSnapshots, /* 1.3.5 */ .domainSnapshotGetXMLDesc = vzDomainSnapshotGetXMLDesc, /* 1.3.5 */ .domainSnapshotNumChildren = vzDomainSnapshotNumChildren, /* 1.3.5 */ .domainSnapshotListChildrenNames = vzDomainSnapshotListChildrenNames, /* 1.3.5 */ .domainSnapshotListAllChildren = vzDomainSnapshotListAllChildren, /* 1.3.5 */ .domainSnapshotLookupByName = vzDomainSnapshotLookupByName, /* 1.3.5 */ .domainHasCurrentSnapshot = vzDomainHasCurrentSnapshot, /* 1.3.5 */ .domainSnapshotGetParent = vzDomainSnapshotGetParent, /* 1.3.5 */ .domainSnapshotCurrent = vzDomainSnapshotCurrent, /* 1.3.5 */ .domainSnapshotIsCurrent = vzDomainSnapshotIsCurrent, /* 1.3.5 */ .domainSnapshotHasMetadata = vzDomainSnapshotHasMetadata, /* 1.3.5 */ .domainSnapshotDelete = vzDomainSnapshotDelete, /* 1.3.5 */ [4] Known issues. 1. accidental errors on getting network statistics 2. race with simultaneous use of thread shared domain object on paths of adding domain thru API and adding domain on vz sdk domain added event. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 183 +++++++++++++++++++++++++++++++++++++---------------- src/vz/vz_sdk.c | 181 +++++++++++++++++++++++++++++++++++----------------- src/vz/vz_utils.c | 50 +++++++++++++++ src/vz/vz_utils.h | 7 ++ 4 files changed, 307 insertions(+), 114 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index a131aa1..c28e39e 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -726,6 +726,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) virDomainObjPtr dom = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; vzDriverPtr driver = privconn->driver; + bool job = false; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -736,7 +737,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) parse_flags)) == NULL) goto cleanup; - dom = virDomainObjListFindByUUID(driver->domains, def->uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, def->uuid); if (dom == NULL) { virResetLastError(); if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { @@ -778,6 +779,10 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; } } else { + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + if (prlsdkApplyConfig(driver, dom, def)) goto cleanup; @@ -791,8 +796,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) retdom->id = def->id; cleanup: - if (dom) - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); virDomainDefFree(def); return retdom; } @@ -994,17 +1000,28 @@ vzDomainUndefineFlags(virDomainPtr domain, { vzConnPtr privconn = domain->conn->privateData; virDomainObjPtr dom = NULL; - int ret; + int ret = -1; + bool job = false; virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + ret = prlsdkUnregisterDomain(privconn->driver, dom, flags); - if (ret) + + cleanup: + + if (job) + vzDomainObjEndJob(dom); + if (ret < 0) virObjectUnlock(dom); + virObjectUnref(dom); return ret; } @@ -1042,13 +1059,18 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) virDomainObjPtr dom = NULL; int state, reason; int ret = -1; + bool job = false; virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + state = virDomainObjGetState(dom, &reason); if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED)) { @@ -1060,7 +1082,9 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, prlsdkSuspend); cleanup: - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1073,7 +1097,7 @@ vzDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; state = virDomainObjGetState(dom, &reason); @@ -1084,7 +1108,7 @@ vzDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags) ret = prlsdkDomainManagedSaveRemove(dom); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1117,11 +1141,12 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, vzConnPtr privconn = domain->conn->privateData; virDomainDeviceDefPtr dev = NULL; virDomainObjPtr dom = NULL; + bool job = false; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -1132,6 +1157,10 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, if (dev == NULL) goto cleanup; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkAttachVolume(privconn->driver, dom, dev->data.disk); @@ -1158,7 +1187,9 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, ret = 0; cleanup: - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1175,11 +1206,12 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, vzConnPtr privconn = domain->conn->privateData; virDomainDeviceDefPtr dev = NULL; virDomainObjPtr dom = NULL; + bool job = false; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - dom = vzDomObjFromDomain(domain); + dom = vzDomObjFromDomainRef(domain); if (dom == NULL) return -1; @@ -1193,6 +1225,10 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, if (dev == NULL) goto cleanup; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkDetachVolume(dom, dev->data.disk); @@ -1219,7 +1255,9 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, ret = 0; cleanup: - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1238,14 +1276,22 @@ vzDomainSetUserPassword(virDomainPtr domain, { virDomainObjPtr dom = NULL; int ret = -1; + bool job = false; virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + ret = prlsdkDomainSetUserPassword(dom, user, password); - virObjectUnlock(dom); + cleanup: + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1558,21 +1604,28 @@ static int vzDomainSetMemoryFlagsImpl(virDomainPtr domain, unsigned long memory, { virDomainObjPtr dom = NULL; int ret = -1; + bool job = false; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; if (useflags && vzCheckConfigUpdateFlags(dom, &flags) < 0) goto cleanup; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + ret = prlsdkSetMemsize(dom, memory >> 10); cleanup: - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1639,7 +1692,7 @@ vzDomainSnapshotNum(virDomainPtr domain, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1649,7 +1702,7 @@ vzDomainSnapshotNum(virDomainPtr domain, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return n; } @@ -1667,7 +1720,7 @@ vzDomainSnapshotListNames(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1677,7 +1730,7 @@ vzDomainSnapshotListNames(virDomainPtr domain, cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return n; } @@ -1694,7 +1747,7 @@ vzDomainListAllSnapshots(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1704,7 +1757,7 @@ vzDomainListAllSnapshots(virDomainPtr domain, cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return n; } @@ -1721,7 +1774,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return NULL; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1738,7 +1791,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return xml; } @@ -1754,7 +1807,7 @@ vzDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1767,7 +1820,7 @@ vzDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return n; } @@ -1786,7 +1839,7 @@ vzDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1799,7 +1852,7 @@ vzDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return n; } @@ -1817,7 +1870,7 @@ vzDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1830,7 +1883,7 @@ vzDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return n; } @@ -1847,7 +1900,7 @@ vzDomainSnapshotLookupByName(virDomainPtr domain, virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return NULL; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1859,7 +1912,7 @@ vzDomainSnapshotLookupByName(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); virDomainSnapshotObjListFree(snapshots); return snapshot; @@ -1874,7 +1927,7 @@ vzDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1884,7 +1937,7 @@ vzDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1899,7 +1952,7 @@ vzDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return NULL; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1919,7 +1972,7 @@ vzDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return parent; } @@ -1934,7 +1987,7 @@ vzDomainSnapshotCurrent(virDomainPtr domain, unsigned int flags) virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return NULL; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1950,7 +2003,7 @@ vzDomainSnapshotCurrent(virDomainPtr domain, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return snapshot; } @@ -1965,7 +2018,7 @@ vzDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -1976,7 +2029,7 @@ vzDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, unsigned int flags) cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1992,7 +2045,7 @@ vzDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return -1; if (!(snapshots = prlsdkLoadSnapshots(dom))) @@ -2005,7 +2058,8 @@ vzDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); + return ret; } @@ -2022,10 +2076,11 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; virDomainSnapshotObjListPtr snapshots = NULL; virDomainSnapshotObjPtr current; + bool job = false; virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return NULL; if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, @@ -2044,6 +2099,10 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + /* snaphot name is ignored, it will be set to auto generated by sdk uuid */ if (prlsdkCreateSnapshot(dom, def->description) < 0) goto cleanup; @@ -2062,8 +2121,10 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, cleanup: virDomainSnapshotObjListFree(snapshots); - virObjectUnlock(dom); virDomainSnapshotDefFree(def); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return snapshot; } @@ -2076,13 +2137,13 @@ vzDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); - if (!(dom = vzDomObjFromDomain(snapshot->domain))) + if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) return -1; ret = prlsdkDeleteSnapshot(dom, snapshot->name, flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -2092,16 +2153,23 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) { virDomainObjPtr dom; int ret = -1; + bool job = false; virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1); if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + ret = prlsdkSwitchToSnapshot(dom, snapshot->name, flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED); - - virObjectUnlock(dom); + cleanup: + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -2436,6 +2504,7 @@ vzDomainMigratePerformStep(virDomainPtr domain, const char *miguri = NULL; const char *dname = NULL; vzMigrationCookiePtr mig = NULL; + bool job = false; virCheckFlags(VZ_MIGRATION_FLAGS, -1); @@ -2458,9 +2527,13 @@ vzDomainMigratePerformStep(virDomainPtr domain, VZ_MIGRATION_COOKIE_SESSION_UUID))) goto cleanup; - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) goto cleanup; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup; @@ -2473,8 +2546,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, ret = 0; cleanup: - if (dom) - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); virURIFree(vzuri); vzMigrationCookieFree(mig); @@ -2656,8 +2730,7 @@ vzDomainMigrateFinish3Params(virConnectPtr dconn, * is already finished. */ if (!domain) VIR_WARN("Can't provide domain '%s' after successfull migration.", name); - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return domain; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index c9f89ab..87bd6eb 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -192,6 +192,27 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, result, __FILE__, __FUNCTION__, __LINE__) static PRL_RESULT +getDomainJobResultHelper(PRL_HANDLE job, virDomainObjPtr dom, + unsigned int timeout, PRL_HANDLE *result, + const char *filename, const char *funcname, + size_t linenr) +{ + PRL_RESULT pret; + + if (dom) + virObjectUnlock(dom); + pret = getJobResultHelper(job, timeout, result, filename, funcname, linenr); + if (dom) + virObjectLock(dom); + + return pret; +} + +#define getDomainJobResult(job, dom, result) \ + getDomainJobResultHelper(job, dom, JOB_INFINIT_WAIT_TIMEOUT, \ + result, __FILE__, __FUNCTION__, __LINE__) + +static PRL_RESULT waitJobHelper(PRL_HANDLE job, unsigned int timeout, const char *filename, const char *funcname, size_t linenr) @@ -209,6 +230,26 @@ waitJobHelper(PRL_HANDLE job, unsigned int timeout, waitJobHelper(job, JOB_INFINIT_WAIT_TIMEOUT, __FILE__, \ __FUNCTION__, __LINE__) +static PRL_RESULT +waitDomainJobHelper(PRL_HANDLE job, virDomainObjPtr dom, unsigned int timeout, + const char *filename, const char *funcname, + size_t linenr) +{ + PRL_RESULT ret; + + if (dom) + virObjectUnlock(dom); + ret = waitJobHelper(job, timeout, filename, funcname, linenr); + if (dom) + virObjectLock(dom); + + return ret; +} + +#define waitDomainJob(job, dom) \ + waitDomainJobHelper(job, dom, JOB_INFINIT_WAIT_TIMEOUT, __FILE__, \ + __FUNCTION__, __LINE__) + typedef PRL_RESULT (*prlsdkParamGetterType)(PRL_HANDLE, char*, PRL_UINT32*); static char* @@ -412,7 +453,7 @@ prlsdkUUIDParse(const char *uuidstr, unsigned char *uuid) } static int -prlsdkGetDomainState(PRL_HANDLE sdkdom, VIRTUAL_MACHINE_STATE_PTR vmState) +prlsdkGetDomainState(virDomainObjPtr dom, PRL_HANDLE sdkdom, VIRTUAL_MACHINE_STATE_PTR vmState) { PRL_HANDLE job = PRL_INVALID_HANDLE; PRL_HANDLE result = PRL_INVALID_HANDLE; @@ -422,7 +463,7 @@ prlsdkGetDomainState(PRL_HANDLE sdkdom, VIRTUAL_MACHINE_STATE_PTR vmState) job = PrlVm_GetState(sdkdom); - if (PRL_FAILED(getJobResult(job, &result))) + if (PRL_FAILED(getDomainJobResult(job, dom, &result))) goto cleanup; pret = PrlResult_GetParamByIndex(result, 0, &vmInfo); @@ -1437,7 +1478,7 @@ prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def) /* if dom is NULL adds new domain into domain list * if dom not NULL updates given locked dom object. * - * Returned object is locked. + * Returned object is locked and referenced. */ static virDomainObjPtr @@ -1516,7 +1557,7 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; } - if (prlsdkGetDomainState(sdkdom, &domainState) < 0) + if (prlsdkGetDomainState(dom, sdkdom, &domainState) < 0) goto error; if (virDomainDefAddImplicitDevices(def) < 0) @@ -1545,7 +1586,7 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; virObjectLock(driver); - if (!(olddom = virDomainObjListFindByUUID(driver->domains, def->uuid))) + if (!(olddom = virDomainObjListFindByUUIDRef(driver->domains, def->uuid))) dom = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0, NULL); virObjectUnlock(driver); @@ -1556,6 +1597,7 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; } + virObjectRef(dom); pdom = dom->privateData; pdom->sdkdom = sdkdom; PrlHandle_AddRef(sdkdom); @@ -1608,8 +1650,8 @@ prlsdkLoadDomains(vzDriverPtr driver) pret = PrlResult_GetParamByIndex(result, i, &sdkdom); prlsdkCheckRetGoto(pret, error); - if ((dom = prlsdkLoadDomain(driver, sdkdom, NULL))) - virObjectUnlock(dom); + dom = prlsdkLoadDomain(driver, sdkdom, NULL); + virDomainObjEndAPI(&dom); PrlHandle_Free(sdkdom); sdkdom = PRL_INVALID_HANDLE; @@ -1663,7 +1705,7 @@ prlsdkUpdateDomain(vzDriverPtr driver, virDomainObjPtr dom) vzDomObjPtr pdom = dom->privateData; job = PrlVm_RefreshConfig(pdom->sdkdom); - if (waitJob(job)) + if (waitDomainJob(job, dom)) return -1; return prlsdkLoadDomain(driver, pdom->sdkdom, dom) ? 0 : -1; @@ -1760,11 +1802,16 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver, unsigned char *uuid) { virDomainObjPtr dom = NULL; + bool job = false; - dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) return; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + if (prlsdkUpdateDomain(driver, dom) < 0) goto cleanup; @@ -1772,7 +1819,9 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_DEFINED_UPDATED); cleanup: - virObjectUnlock(dom); + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return; } @@ -1782,7 +1831,7 @@ prlsdkHandleVmAddedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL; - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)) && + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid)) && !(dom = prlsdkAddDomainByUUID(driver, uuid))) goto cleanup; @@ -1790,8 +1839,7 @@ prlsdkHandleVmAddedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_DEFINED_ADDED); cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return; } @@ -1990,7 +2038,9 @@ prlsdkDomainChangeStateLocked(vzDriverPtr driver, virErrorNumber virerr; pdom = dom->privateData; + virObjectUnlock(dom); pret = chstate(pdom->sdkdom); + virObjectLock(dom); if (PRL_FAILED(pret)) { virResetLastError(); @@ -2019,12 +2069,21 @@ prlsdkDomainChangeState(virDomainPtr domain, vzConnPtr privconn = domain->conn->privateData; virDomainObjPtr dom; int ret = -1; + bool job = false; - if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) return -1; + if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, chstate); - virObjectUnlock(dom); + + cleanup: + if (job) + vzDomainObjEndJob(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -2783,6 +2842,7 @@ static const char * prlsdkFormatMac(virMacAddrPtr mac, char *macstr) } static int prlsdkAddNet(vzDriverPtr driver, + virDomainObjPtr dom, PRL_HANDLE sdkdom, virDomainNetDefPtr net, bool isCt) @@ -2990,7 +3050,7 @@ static int prlsdkAddNet(vzDriverPtr driver, job = PrlSrv_AddVirtualNetwork(driver->server, vnet, PRL_USE_VNET_NAME_FOR_BRIDGE_NAME); - if (PRL_FAILED(pret = waitJob(job))) + if (PRL_FAILED(pret = waitDomainJob(job, dom))) goto cleanup; pret = PrlVmDev_SetEmulatedType(sdknet, PNA_BRIDGED_ETHERNET); @@ -3016,7 +3076,9 @@ static int prlsdkAddNet(vzDriverPtr driver, } static void -prlsdkCleanupBridgedNet(vzDriverPtr driver, virDomainNetDefPtr net) +prlsdkCleanupBridgedNet(vzDriverPtr driver, + virDomainObjPtr dom, + virDomainNetDefPtr net) { PRL_RESULT pret; PRL_HANDLE vnet = PRL_INVALID_HANDLE; @@ -3032,8 +3094,7 @@ prlsdkCleanupBridgedNet(vzDriverPtr driver, virDomainNetDefPtr net) prlsdkCheckRetGoto(pret, cleanup); job = PrlSrv_DeleteVirtualNetwork(driver->server, vnet, 0); - if (PRL_FAILED(pret = waitJob(job))) - goto cleanup; + ignore_value(waitDomainJob(job, dom)); cleanup: PrlHandle_Free(vnet); @@ -3054,13 +3115,13 @@ int prlsdkAttachNet(vzDriverPtr driver, } job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return ret; - ret = prlsdkAddNet(driver, privdom->sdkdom, net, IS_CT(dom->def)); + ret = prlsdkAddNet(driver, dom, privdom->sdkdom, net, IS_CT(dom->def)); if (ret == 0) { job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return -1; } @@ -3119,20 +3180,20 @@ int prlsdkDetachNet(vzDriverPtr driver, } job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; sdknet = prlsdkFindNetByMAC(privdom->sdkdom, &net->mac); if (sdknet == PRL_INVALID_HANDLE) goto cleanup; - prlsdkCleanupBridgedNet(driver, net); + prlsdkCleanupBridgedNet(driver, dom, net); pret = PrlVmDev_Remove(sdknet); prlsdkCheckRetGoto(pret, cleanup); job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; ret = 0; @@ -3342,13 +3403,13 @@ prlsdkAttachVolume(vzDriverPtr driver, PRL_HANDLE job = PRL_INVALID_HANDLE; job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; ret = prlsdkAddDisk(driver, privdom->sdkdom, disk); if (ret == 0) { job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) { + if (PRL_FAILED(waitDomainJob(job, dom))) { ret = -1; goto cleanup; } @@ -3372,14 +3433,14 @@ prlsdkDetachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk) goto cleanup; job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; pret = PrlVmDev_Remove(sdkdisk); prlsdkCheckRetGoto(pret, cleanup); job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; ret = 0; @@ -3516,7 +3577,7 @@ prlsdkDomainSetUserPassword(virDomainObjPtr dom, PRL_HANDLE job = PRL_INVALID_HANDLE; job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; job = PrlVm_SetUserPasswd(privdom->sdkdom, @@ -3524,11 +3585,11 @@ prlsdkDomainSetUserPassword(virDomainObjPtr dom, password, 0); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; job = PrlVm_CommitEx(privdom->sdkdom, 0); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; ret = 0; @@ -3539,9 +3600,9 @@ prlsdkDomainSetUserPassword(virDomainObjPtr dom, static int prlsdkDoApplyConfig(vzDriverPtr driver, + virDomainObjPtr dom, PRL_HANDLE sdkdom, - virDomainDefPtr def, - virDomainDefPtr olddef) + virDomainDefPtr def) { PRL_RESULT pret; size_t i; @@ -3602,13 +3663,13 @@ prlsdkDoApplyConfig(vzDriverPtr driver, if (prlsdkRemoveBootDevices(sdkdom) < 0) goto error; - if (olddef) { - for (i = 0; i < olddef->nnets; i++) - prlsdkCleanupBridgedNet(driver, olddef->nets[i]); + if (dom) { + for (i = 0; i < dom->def->nnets; i++) + prlsdkCleanupBridgedNet(driver, dom, dom->def->nets[i]); } for (i = 0; i < def->nnets; i++) { - if (prlsdkAddNet(driver, sdkdom, def->nets[i], IS_CT(def)) < 0) + if (prlsdkAddNet(driver, dom, sdkdom, def->nets[i], IS_CT(def)) < 0) goto error; } @@ -3647,7 +3708,7 @@ prlsdkDoApplyConfig(vzDriverPtr driver, VIR_FREE(mask); for (i = 0; i < def->nnets; i++) - prlsdkCleanupBridgedNet(driver, def->nets[i]); + prlsdkCleanupBridgedNet(driver, dom, def->nets[i]); return -1; } @@ -3662,14 +3723,14 @@ prlsdkApplyConfig(vzDriverPtr driver, int ret; job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return -1; - ret = prlsdkDoApplyConfig(driver, privdom->sdkdom, new, dom->def); + ret = prlsdkDoApplyConfig(driver, dom, privdom->sdkdom, new); if (ret == 0) { job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) ret = -1; } @@ -3702,7 +3763,7 @@ prlsdkCreateVm(vzDriverPtr driver, virDomainDefPtr def) pret = PrlVmCfg_SetOfflineManagementEnabled(sdkdom, 0); prlsdkCheckRetGoto(pret, cleanup); - if (prlsdkDoApplyConfig(driver, sdkdom, def, NULL) < 0) + if (prlsdkDoApplyConfig(driver, NULL, sdkdom, def) < 0) goto cleanup; job = PrlVm_Reg(sdkdom, "", 1); @@ -3768,7 +3829,7 @@ prlsdkCreateCt(vzDriverPtr driver, virDomainDefPtr def) } - if (prlsdkDoApplyConfig(driver, sdkdom, def, NULL) < 0) + if (prlsdkDoApplyConfig(driver, NULL, sdkdom, def) < 0) goto cleanup; flags = PACF_NON_INTERACTIVE_MODE; @@ -3794,7 +3855,7 @@ prlsdkCreateCt(vzDriverPtr driver, virDomainDefPtr def) * Returns 0 if hard disks were successfully detached or not detected. */ static int -prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) +prlsdkDetachDomainHardDisks(virDomainObjPtr dom) { int ret = -1; PRL_RESULT pret; @@ -3802,9 +3863,11 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) PRL_UINT32 i; PRL_HANDLE job; PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE; + vzDomObjPtr pdom = dom->privateData; + PRL_HANDLE sdkdom = pdom->sdkdom; job = PrlVm_BeginEdit(sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; pret = PrlVmCfg_GetHardDisksCount(sdkdom, &hddCount); @@ -3822,7 +3885,7 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) } job = PrlVm_CommitEx(sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; ret = 0; @@ -3843,7 +3906,7 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla int ret = -1; int num; - if (prlsdkGetDomainState(privdom->sdkdom, &domainState) < 0) + if (prlsdkGetDomainState(dom, privdom->sdkdom, &domainState) < 0) return -1; if (VMS_SUSPENDED == domainState && @@ -3867,15 +3930,15 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla goto cleanup; } - if (prlsdkDetachDomainHardDisks(privdom->sdkdom)) + if (prlsdkDetachDomainHardDisks(dom)) goto cleanup; job = PrlVm_Delete(privdom->sdkdom, PRL_INVALID_HANDLE); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; for (i = 0; i < dom->def->nnets; i++) - prlsdkCleanupBridgedNet(driver, dom->def->nets[i]); + prlsdkCleanupBridgedNet(driver, dom, dom->def->nets[i]); prlsdkSendEvent(driver, dom, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); @@ -3896,7 +3959,7 @@ prlsdkDomainManagedSaveRemove(virDomainObjPtr dom) PRL_HANDLE job; job = PrlVm_DropSuspendedState(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return -1; return 0; @@ -4154,14 +4217,14 @@ int prlsdkSetMemsize(virDomainObjPtr dom, unsigned int memsize) PRL_RESULT pret; job = PrlVm_BeginEdit(privdom->sdkdom); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto error; pret = PrlVmCfg_SetRamSize(privdom->sdkdom, memsize); prlsdkCheckRetGoto(pret, error); job = PrlVm_CommitEx(privdom->sdkdom, 0); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto error; return 0; @@ -4331,7 +4394,7 @@ prlsdkLoadSnapshots(virDomainObjPtr dom) char *treexml = NULL; job = PrlVm_GetSnapshotsTreeEx(privdom->sdkdom, PGST_WITHOUT_SCREENSHOTS); - if (PRL_FAILED(getJobResult(job, &result))) + if (PRL_FAILED(getDomainJobResult(job, dom, &result))) goto cleanup; if (!(treexml = prlsdkGetStringParamVar(PrlResult_GetParamAsString, result))) @@ -4352,7 +4415,7 @@ int prlsdkCreateSnapshot(virDomainObjPtr dom, const char *description) job = PrlVm_CreateSnapshot(privdom->sdkdom, "", description ? : ""); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return -1; return 0; @@ -4364,7 +4427,7 @@ int prlsdkDeleteSnapshot(virDomainObjPtr dom, const char *uuid, bool children) PRL_HANDLE job; job = PrlVm_DeleteSnapshot(privdom->sdkdom, uuid, children); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return -1; return 0; @@ -4380,7 +4443,7 @@ int prlsdkSwitchToSnapshot(virDomainObjPtr dom, const char *uuid, bool paused) flags |= PSSF_SKIP_RESUME; job = PrlVm_SwitchToSnapshotEx(privdom->sdkdom, uuid, flags); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) return -1; return 0; @@ -4419,7 +4482,7 @@ int prlsdkMigrate(virDomainObjPtr dom, virURIPtr uri, PRL_TRUE ); - if (PRL_FAILED(waitJob(job))) + if (PRL_FAILED(waitDomainJob(job, dom))) goto cleanup; ret = 0; diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index b629bc3..dc8dbf3 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -33,6 +33,7 @@ #include "virstring.h" #include "datatypes.h" #include "virlog.h" +#include "virtime.h" #define VIR_FROM_THIS VIR_FROM_PARALLELS #define PRLSRVCTL "prlsrvctl" @@ -458,9 +459,17 @@ vzDomObjAlloc(void) if (VIR_ALLOC(pdom) < 0) return NULL; + if (virCondInit(&pdom->jobCond) < 0) + goto error; + pdom->stats = PRL_INVALID_HANDLE; return pdom; + + error: + VIR_FREE(pdom); + + return NULL; } void @@ -473,5 +482,46 @@ vzDomObjFree(void* p) PrlHandle_Free(pdom->sdkdom); PrlHandle_Free(pdom->stats); + virCondDestroy(&pdom->jobCond); VIR_FREE(pdom); }; + +#define VZ_JOB_WAIT_TIME (1000 * 30) + +int +vzDomainObjBeginJob(virDomainObjPtr dom) +{ + vzDomObjPtr pdom = dom->privateData; + unsigned long long now; + unsigned long long then; + + if (virTimeMillisNow(&now) < 0) + return -1; + then = now + VZ_JOB_WAIT_TIME; + + while (pdom->job) { + if (virCondWaitUntil(&pdom->jobCond, &dom->parent.lock, then) < 0) + goto error; + } + + pdom->job = true; + return 0; + + error: + if (errno == ETIMEDOUT) + virReportError(VIR_ERR_OPERATION_TIMEOUT, + "%s", _("cannot acquire state change lock")); + else + virReportSystemError(errno, + "%s", _("cannot acquire job mutex")); + return -1; +} + +void +vzDomainObjEndJob(virDomainObjPtr dom) +{ + vzDomObjPtr pdom = dom->privateData; + + pdom->job = false; + virCondSignal(&pdom->jobCond); +} diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index 20ba1fd..548b264 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -98,6 +98,8 @@ struct vzDomObj { int id; PRL_HANDLE sdkdom; PRL_HANDLE stats; + bool job; + virCond jobCond; }; typedef struct vzDomObj *vzDomObjPtr; @@ -136,3 +138,8 @@ vzGetDefaultSCSIModel(vzDriverPtr driver, OP(wr_bytes, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, "write_total") #endif + +int +vzDomainObjBeginJob(virDomainObjPtr dom); +void +vzDomainObjEndJob(virDomainObjPtr dom); -- 1.8.3.1
On 16.06.2016 14:59, Nikolay Shirokovskiy wrote:
Current vz driver implementation is not usable when it comes to long runnig operations. Migration or saving a domain blocks all other operations even query ones which are expecteted to be available. This patch address this problem.
...
@@ -2436,6 +2504,7 @@ vzDomainMigratePerformStep(virDomainPtr domain, const char *miguri = NULL; const char *dname = NULL; vzMigrationCookiePtr mig = NULL; + bool job = false;
virCheckFlags(VZ_MIGRATION_FLAGS, -1);
@@ -2458,9 +2527,13 @@ vzDomainMigratePerformStep(virDomainPtr domain, VZ_MIGRATION_COOKIE_SESSION_UUID))) goto cleanup;
- if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) goto cleanup;
+ if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup;
@@ -2473,8 +2546,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, ret = 0;
a mistake here makes migrating disfuntional, here is tiny fix to squash in: diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index f427555..02ff2e0 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2543,7 +2543,6 @@ vzDomainMigratePerformStep(virDomainPtr domain, goto cleanup; virDomainObjListRemove(privconn->driver->domains, dom); - dom = NULL; ret = 0; diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 92af4da..82380ca 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -527,8 +527,12 @@ vzDomainObjBeginJob(virDomainObjPtr dom) void vzDomainObjEndJob(virDomainObjPtr dom) { - vzDomObjPtr pdom = dom->privateData; + vzDomObjPtr pdom; + if (!dom) + return; + + pdom = dom->privateData; pdom->job.active = false; virCondSignal(&pdom->job.cond); }
On 17.06.2016 12:43, Nikolay Shirokovskiy wrote:
On 16.06.2016 14:59, Nikolay Shirokovskiy wrote:
Current vz driver implementation is not usable when it comes to long runnig operations. Migration or saving a domain blocks all other operations even query ones which are expecteted to be available. This patch address this problem.
...
@@ -2436,6 +2504,7 @@ vzDomainMigratePerformStep(virDomainPtr domain, const char *miguri = NULL; const char *dname = NULL; vzMigrationCookiePtr mig = NULL; + bool job = false;
virCheckFlags(VZ_MIGRATION_FLAGS, -1);
@@ -2458,9 +2527,13 @@ vzDomainMigratePerformStep(virDomainPtr domain, VZ_MIGRATION_COOKIE_SESSION_UUID))) goto cleanup;
- if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) goto cleanup;
+ if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup;
@@ -2473,8 +2546,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, ret = 0;
a mistake here makes migrating disfuntional, here is tiny fix to squash in:
here is correct fix )), it affects another place where we delete domain object at the end of operation Author: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Date: Fri Jun 17 13:28:39 2016 +0300 vz: quickfix: job fix for migration Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 1569278..6a508b3 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -1018,8 +1018,7 @@ vzDomainUndefineFlags(virDomainPtr domain, if (job) vzDomainObjEndJob(dom); - if (ret < 0) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); virObjectUnref(dom); return ret; @@ -2540,7 +2539,7 @@ vzDomainMigratePerformStep(virDomainPtr domain, goto cleanup; virDomainObjListRemove(privconn->driver->domains, dom); - dom = NULL; + virObjectLock(dom); ret = 0; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 551df10..205189a 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3943,6 +3943,7 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); + virObjectLock(dom); ret = 0; cleanup:
17-Jun-16 13:46, Nikolay Shirokovskiy пишет:
On 17.06.2016 12:43, Nikolay Shirokovskiy wrote:
On 16.06.2016 14:59, Nikolay Shirokovskiy wrote:
Current vz driver implementation is not usable when it comes to long runnig operations. Migration or saving a domain blocks all other operations even query ones which are expecteted to be available. This patch address this problem.
...
@@ -2436,6 +2504,7 @@ vzDomainMigratePerformStep(virDomainPtr domain, const char *miguri = NULL; const char *dname = NULL; vzMigrationCookiePtr mig = NULL; + bool job = false;
virCheckFlags(VZ_MIGRATION_FLAGS, -1);
@@ -2458,9 +2527,13 @@ vzDomainMigratePerformStep(virDomainPtr domain, VZ_MIGRATION_COOKIE_SESSION_UUID))) goto cleanup;
- if (!(dom = vzDomObjFromDomain(domain))) + if (!(dom = vzDomObjFromDomainRef(domain))) goto cleanup;
+ if (vzDomainObjBeginJob(dom) < 0) + goto cleanup; + job = true; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup;
@@ -2473,8 +2546,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, ret = 0; a mistake here makes migrating disfuntional, here is tiny fix to squash in:
here is correct fix )), it affects another place where we delete domain object at the end of operation
Though I agree this the approach of the whole series, this amendment to the 3/3 patch doesn't work either. Could you please resend the series rebased and corrected ?
This patch is not critical but nice to have. The original motivation was error message in logs on undefining domain thru vz driver. Undefine procedure drops domain lock while waiting for detaching disks vz sdk call. Meanwhile vz sdk event domain-config-changed arrives, its handler finds domain and is blocked waiting for job condition. After undefine API call finishes event processing procedes and tries to refreshes domain config thru existing vz sdk domain handle. Domain does not exists anymore and event processing fails. Everything is fine we just don't want to see error message in log for this particular case. Fortunately domain has flag that domain is removed from list. This also imply that vz sdk domain is also undefined. Thus if we check for this flag right after domain is locked again on accuiring job condition we gracefully handle this situation. Actually the race can happen in other situations too. Any time we wait for job condition in mutualy exclusive job in time when we acquire it vz sdk domain can cease to exist. So instead of general internal error we can return domain not found which is easier to handle. We don't need to patch other places in mutually exclusive jobs where domain lock is dropped as if job is started domain can't be undefine by mutually exclusive undefine job. In case of API calls that are not jobs (load snapshots etc) we'd better to check if domain exists every time domain lock is reacquired. Fortunately these calls drop domain lock only once to call appropriate vz sdk API call. The code of this patch is quite similar to qemu driver checks for is domain is active after acquiring a job. The difference only while qemu domain is operational while process is active vz domain is operational while domain exists. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/vz/vz_sdk.c | 32 +++++++++++++++++++++++++++++--- 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 6a508b3..67db2d9 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -716,6 +716,22 @@ vzDomainGetAutostart(virDomainPtr domain, int *autostart) return 0; } +static bool +vzDomainObjIsExist(virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!dom->removing) + return true; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); + + return false; +} + static virDomainPtr vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) { @@ -782,6 +798,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + if (prlsdkApplyConfig(driver, dom, def)) goto cleanup; @@ -1012,6 +1031,9 @@ vzDomainUndefineFlags(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkUnregisterDomain(privconn->driver, dom, flags); cleanup: @@ -1069,6 +1091,9 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + state = virDomainObjGetState(dom, &reason); if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED)) { @@ -1159,6 +1184,9 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkAttachVolume(privconn->driver, dom, dev->data.disk); @@ -1227,6 +1255,9 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkDetachVolume(dom, dev->data.disk); @@ -1284,6 +1315,9 @@ vzDomainSetUserPassword(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkDomainSetUserPassword(dom, user, password); cleanup: @@ -1617,6 +1651,9 @@ static int vzDomainSetMemoryFlagsImpl(virDomainPtr domain, unsigned long memory, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkSetMemsize(dom, memory >> 10); cleanup: @@ -2101,6 +2138,9 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + /* snaphot name is ignored, it will be set to auto generated by sdk uuid */ if (prlsdkCreateSnapshot(dom, def->description) < 0) goto cleanup; @@ -2162,6 +2202,9 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkSwitchToSnapshot(dom, snapshot->name, flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED); cleanup: @@ -2532,6 +2575,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 205189a..663e02d 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1812,6 +1812,9 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver, goto cleanup; job = true; + if (dom->removing) + goto cleanup; + if (prlsdkUpdateDomain(driver, dom) < 0) goto cleanup; @@ -2078,6 +2081,9 @@ prlsdkDomainChangeState(virDomainPtr domain, goto cleanup; job = true; + if (dom->removing) + goto cleanup; + ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, chstate); cleanup: @@ -3952,6 +3958,20 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla return ret; } +static void +vzDomainObjCheckExist(virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!dom->removing) + return; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); +} + int prlsdkDomainManagedSaveRemove(virDomainObjPtr dom) { @@ -3959,8 +3979,10 @@ prlsdkDomainManagedSaveRemove(virDomainObjPtr dom) PRL_HANDLE job; job = PrlVm_DropSuspendedState(privdom->sdkdom); - if (PRL_FAILED(waitDomainJob(job, dom))) + if (PRL_FAILED(waitDomainJob(job, dom))) { + vzDomainObjCheckExist(dom); return -1; + } return 0; } @@ -4394,8 +4416,10 @@ prlsdkLoadSnapshots(virDomainObjPtr dom) char *treexml = NULL; job = PrlVm_GetSnapshotsTreeEx(privdom->sdkdom, PGST_WITHOUT_SCREENSHOTS); - if (PRL_FAILED(getDomainJobResult(job, dom, &result))) + if (PRL_FAILED(getDomainJobResult(job, dom, &result))) { + vzDomainObjCheckExist(dom); goto cleanup; + } if (!(treexml = prlsdkGetStringParamVar(PrlResult_GetParamAsString, result))) goto cleanup; @@ -4427,8 +4451,10 @@ int prlsdkDeleteSnapshot(virDomainObjPtr dom, const char *uuid, bool children) PRL_HANDLE job; job = PrlVm_DeleteSnapshot(privdom->sdkdom, uuid, children); - if (PRL_FAILED(waitDomainJob(job, dom))) + if (PRL_FAILED(waitDomainJob(job, dom))) { + vzDomainObjCheckExist(dom); return -1; + } return 0; } -- 1.8.3.1
On 20.06.2016 10:40, Nikolay Shirokovskiy wrote:
This patch is not critical but nice to have. The original motivation was error message in logs on undefining domain thru vz driver. Undefine procedure drops domain lock while waiting for detaching disks vz sdk call. Meanwhile vz sdk event domain-config-changed arrives, its handler finds domain and is blocked waiting for job condition. After undefine API call finishes event processing procedes and tries to refreshes domain config thru existing vz sdk domain handle. Domain does not exists anymore and event processing fails. Everything is fine we just don't want to see error message in log for this particular case.
Fortunately domain has flag that domain is removed from list. This also imply that vz sdk domain is also undefined. Thus if we check for this flag right after domain is locked again on accuiring job condition we gracefully handle this situation.
Actually the race can happen in other situations too. Any time we wait for job condition in mutualy exclusive job in time when we acquire it vz sdk domain can cease to exist. So instead of general internal error we can return domain not found which is easier to handle. We don't need to patch other places in mutually exclusive jobs where domain lock is dropped as if job is started domain can't be undefine by mutually exclusive undefine job.
In case of API calls that are not jobs (load snapshots etc) we'd better to check if domain exists every time domain lock is reacquired. Fortunately these calls drop domain lock only once to call appropriate vz sdk API call.
The code of this patch is quite similar to qemu driver checks for is domain is active after acquiring a job. The difference only while qemu domain is operational while process is active vz domain is operational while domain exists.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/vz/vz_sdk.c | 32 +++++++++++++++++++++++++++++--- 2 files changed, 75 insertions(+), 3 deletions(-)
[snip] Another case of hindsight is 20/20 )) I think the below changes are not good and should be dropped from the patch. We can't rely on 'removing' flag in case other then mutually exclusive jobs.
+static void +vzDomainObjCheckExist(virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!dom->removing) + return; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); +} + int prlsdkDomainManagedSaveRemove(virDomainObjPtr dom) { @@ -3959,8 +3979,10 @@ prlsdkDomainManagedSaveRemove(virDomainObjPtr dom) PRL_HANDLE job;
job = PrlVm_DropSuspendedState(privdom->sdkdom); - if (PRL_FAILED(waitDomainJob(job, dom))) + if (PRL_FAILED(waitDomainJob(job, dom))) { + vzDomainObjCheckExist(dom); return -1; + }
return 0; } @@ -4394,8 +4416,10 @@ prlsdkLoadSnapshots(virDomainObjPtr dom) char *treexml = NULL;
job = PrlVm_GetSnapshotsTreeEx(privdom->sdkdom, PGST_WITHOUT_SCREENSHOTS); - if (PRL_FAILED(getDomainJobResult(job, dom, &result))) + if (PRL_FAILED(getDomainJobResult(job, dom, &result))) { + vzDomainObjCheckExist(dom); goto cleanup; + }
if (!(treexml = prlsdkGetStringParamVar(PrlResult_GetParamAsString, result))) goto cleanup; @@ -4427,8 +4451,10 @@ int prlsdkDeleteSnapshot(virDomainObjPtr dom, const char *uuid, bool children) PRL_HANDLE job;
job = PrlVm_DeleteSnapshot(privdom->sdkdom, uuid, children); - if (PRL_FAILED(waitDomainJob(job, dom))) + if (PRL_FAILED(waitDomainJob(job, dom))) { + vzDomainObjCheckExist(dom); return -1; + }
return 0; }
This patch is not critical but nice to have. The original motivation was error message in logs on undefining domain thru vz driver. Undefine procedure drops domain lock while waiting for detaching disks vz sdk call. Meanwhile vz sdk event domain-config-changed arrives, its handler finds domain and is blocked waiting for job condition. After undefine API call finishes event processing procedes and tries to refreshes domain config thru existing vz sdk domain handle. Domain does not exists anymore and event processing fails. Everything is fine we just don't want to see error message in log for this particular case. Fortunately domain has flag that domain is removed from list. This also imply that vz sdk domain is also undefined. Thus if we check for this flag right after domain is locked again on accuiring job condition we gracefully handle this situation. Actually the race can happen in other situations too. Any time we wait for job condition in mutualy exclusive job in time when we acquire it vz sdk domain can cease to exist. So instead of general internal error we can return domain not found which is easier to handle. We don't need to patch other places in mutually exclusive jobs where domain lock is dropped as if job is started domain can't be undefine by mutually exclusive undefine job. The code of this patch is quite similar to qemu driver checks for is domain is active after acquiring a job. The difference only while qemu domain is operational while process is active vz domain is operational while domain exists. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- Changes from v1: 1. drop changes for API calls that do not accquire job condition. 2. fix prlsdkDomainChangeState to report error properly. src/vz/vz_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/vz/vz_sdk.c | 13 +++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 1569278..711a354 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -716,6 +716,22 @@ vzDomainGetAutostart(virDomainPtr domain, int *autostart) return 0; } +static bool +vzDomainObjIsExist(virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!dom->removing) + return true; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); + + return false; +} + static virDomainPtr vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) { @@ -782,6 +798,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + if (prlsdkApplyConfig(driver, dom, def)) goto cleanup; @@ -1012,6 +1031,9 @@ vzDomainUndefineFlags(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkUnregisterDomain(privconn->driver, dom, flags); cleanup: @@ -1070,6 +1092,9 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + state = virDomainObjGetState(dom, &reason); if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED)) { @@ -1160,6 +1185,9 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkAttachVolume(privconn->driver, dom, dev->data.disk); @@ -1228,6 +1256,9 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkDetachVolume(dom, dev->data.disk); @@ -1285,6 +1316,9 @@ vzDomainSetUserPassword(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkDomainSetUserPassword(dom, user, password); cleanup: @@ -1618,6 +1652,9 @@ static int vzDomainSetMemoryFlagsImpl(virDomainPtr domain, unsigned long memory, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkSetMemsize(dom, memory >> 10); cleanup: @@ -2102,6 +2139,9 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + /* snaphot name is ignored, it will be set to auto generated by sdk uuid */ if (prlsdkCreateSnapshot(dom, def->description) < 0) goto cleanup; @@ -2163,6 +2203,9 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkSwitchToSnapshot(dom, snapshot->name, flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED); cleanup: @@ -2533,6 +2576,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, goto cleanup; job = true; + if (!vzDomainObjIsExist(dom)) + goto cleanup; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 551df10..ba3c28e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1812,6 +1812,9 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver, goto cleanup; job = true; + if (dom->removing) + goto cleanup; + if (prlsdkUpdateDomain(driver, dom) < 0) goto cleanup; @@ -2078,6 +2081,16 @@ prlsdkDomainChangeState(virDomainPtr domain, goto cleanup; job = true; + if (dom->removing) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); + goto cleanup; + } + ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, chstate); cleanup: -- 1.8.3.1
21-Jun-16 11:35, Nikolay Shirokovskiy пишет:
This patch is not critical but nice to have. The original motivation was error message in logs on undefining domain thru vz driver. Undefine procedure drops domain lock while waiting for detaching disks vz sdk call. Meanwhile vz sdk event domain-config-changed arrives, its handler finds domain and is blocked waiting for job condition. After undefine API call finishes event processing procedes and tries to refreshes domain config thru existing vz sdk domain handle. Domain does not exists anymore and event processing fails. Everything is fine we just don't want to see error message in log for this particular case.
Fortunately domain has flag that domain is removed from list. This also imply that vz sdk domain is also undefined. Thus if we check for this flag right after domain is locked again on accuiring job condition we gracefully handle this situation.
Actually the race can happen in other situations too. Any time we wait for job condition in mutualy exclusive job in time when we acquire it vz sdk domain can cease to exist. So instead of general internal error we can return domain not found which is easier to handle. We don't need to patch other places in mutually exclusive jobs where domain lock is dropped as if job is started domain can't be undefine by mutually exclusive undefine job.
The code of this patch is quite similar to qemu driver checks for is domain is active after acquiring a job. The difference only while qemu domain is operational while process is active vz domain is operational while domain exists.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> ---
Changes from v1: 1. drop changes for API calls that do not accquire job condition. 2. fix prlsdkDomainChangeState to report error properly.
src/vz/vz_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/vz/vz_sdk.c | 13 +++++++++++++ 2 files changed, 59 insertions(+)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 1569278..711a354 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -716,6 +716,22 @@ vzDomainGetAutostart(virDomainPtr domain, int *autostart) return 0; }
+static bool +vzDomainObjIsExist(virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!dom->removing) + return true; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); + + return false; +} +
I think this function is misleading as it sets error while a caller could only ask if there is a domain. Thus the usage of the functions implies that it should be called only when a caller doesn't expect a domain to be absent. Maybe we then should rename it to vzEnsureDomainExists ?
static virDomainPtr vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) { @@ -782,6 +798,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + if (prlsdkApplyConfig(driver, dom, def)) goto cleanup;
@@ -1012,6 +1031,9 @@ vzDomainUndefineFlags(virDomainPtr domain, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkUnregisterDomain(privconn->driver, dom, flags);
cleanup: @@ -1070,6 +1092,9 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + state = virDomainObjGetState(dom, &reason);
if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED)) { @@ -1160,6 +1185,9 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkAttachVolume(privconn->driver, dom, dev->data.disk); @@ -1228,6 +1256,9 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: ret = prlsdkDetachVolume(dom, dev->data.disk); @@ -1285,6 +1316,9 @@ vzDomainSetUserPassword(virDomainPtr domain, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkDomainSetUserPassword(dom, user, password);
cleanup: @@ -1618,6 +1652,9 @@ static int vzDomainSetMemoryFlagsImpl(virDomainPtr domain, unsigned long memory, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkSetMemsize(dom, memory >> 10);
cleanup: @@ -2102,6 +2139,9 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + /* snaphot name is ignored, it will be set to auto generated by sdk uuid */ if (prlsdkCreateSnapshot(dom, def->description) < 0) goto cleanup; @@ -2163,6 +2203,9 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + ret = prlsdkSwitchToSnapshot(dom, snapshot->name, flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED); cleanup: @@ -2533,6 +2576,9 @@ vzDomainMigratePerformStep(virDomainPtr domain, goto cleanup; job = true;
+ if (!vzDomainObjIsExist(dom)) + goto cleanup; + if (!(vzuri = vzParseVzURI(miguri))) goto cleanup;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 551df10..ba3c28e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1812,6 +1812,9 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver, goto cleanup; job = true;
+ if (dom->removing) + goto cleanup; + if (prlsdkUpdateDomain(driver, dom) < 0) goto cleanup;
@@ -2078,6 +2081,16 @@ prlsdkDomainChangeState(virDomainPtr domain, goto cleanup; job = true;
+ if (dom->removing) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(dom->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->def->name); + goto cleanup; + } + ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, chstate);
cleanup:
participants (2)
-
Maxim Nestratov -
Nikolay Shirokovskiy