5:09 p.m.
Some driver have concept of jobs (e.g. qemu, lxc, libxl and vz).
This means that whenever something wants to modify a domain
object it needs to acquire job (to mutually exclude with other
threads trying to modify the same object). And aforementioned
drivers do that more or less successfully. Except for
virDomainObjListAdd() which may modify the domain object without
acquiring job. This is harmful because if a thread that's
executing say an API in qemu driver unlock a domain object and
locks it again the object has changed even though the thread
acquired job before.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/bhyve/bhyve_driver.c | 10 ++++++----
src/conf/virdomainobjlist.c | 29 +++++++----------------------
src/conf/virdomainobjlist.h | 3 +--
src/libxl/libxl_driver.c | 22 ++++++++++++----------
src/libxl/libxl_migration.c | 12 ++++++------
src/lxc/lxc_driver.c | 10 +++++-----
src/openvz/openvz_conf.c | 12 ++++++------
src/openvz/openvz_driver.c | 17 +++++++++--------
src/qemu/qemu_driver.c | 22 +++++++++++-----------
src/qemu/qemu_migration.c | 6 +++---
src/test/test_driver.c | 21 ++++++++++++---------
src/vmware/vmware_conf.c | 4 ++--
src/vmware/vmware_driver.c | 9 ++++-----
src/vz/vz_sdk.c | 4 +++-
14 files changed, 87 insertions(+), 94 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 061888ab0b..23b916406a 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -540,9 +540,10 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
goto cleanup;
if (!(vm = virDomainObjListAdd(privconn->domains, def,
- privconn->xmlopt,
- 0, &oldDef)))
+ privconn->xmlopt, 0)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -941,9 +942,10 @@ bhyveDomainCreateXML(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(privconn->domains, def,
privconn->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (virBhyveProcessStart(conn, privconn, vm,
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 1086aec421..e2b829b91c 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -265,12 +265,7 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms,
* virDomainObjListAddLocked:
*
* If flags & VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE then
- * this will refuse updating an existing def if the
- * current def is Live
- *
- * If flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE then
- * the @def being added is assumed to represent a
- * live config, not a future inactive config
+ * this will fail if domain is already active or starting up.
*
* The returned @vm from this function will be locked and ref
* counted. The caller is expected to use virDomainObjEndAPI
@@ -280,15 +275,11 @@ static virDomainObjPtr
virDomainObjListAddLocked(virDomainObjListPtr doms,
virDomainDefPtr def,
virDomainXMLOptionPtr xmlopt,
- unsigned int flags,
- virDomainDefPtr *oldDef)
+ unsigned int flags)
{
virDomainObjPtr vm;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- if (oldDef)
- *oldDef = NULL;
-
/* See if a VM with matching UUID already exists */
if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
if (vm->removing) {
@@ -320,11 +311,6 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
goto error;
}
}
-
- virDomainObjAssignDef(vm,
- def,
- !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
- oldDef);
} else {
/* UUID does not match, but if a name matches, refuse it */
if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) {
@@ -340,8 +326,6 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
if (virDomainObjListAddObjLocked(doms, vm, def->uuid, def->name) < 0)
goto error;
-
- vm->def = def;
}
return vm;
@@ -355,13 +339,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virDomainDefPtr def,
virDomainXMLOptionPtr xmlopt,
- unsigned int flags,
- virDomainDefPtr *oldDef)
+ unsigned int flags)
{
virDomainObjPtr ret;
virObjectRWLockWrite(doms);
- ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
+ ret = virDomainObjListAddLocked(doms, def, xmlopt, flags);
virObjectRWUnlock(doms);
return ret;
}
@@ -513,9 +496,11 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
goto error;
- if (!(dom = virDomainObjListAddLocked(doms, def, xmlopt, 0, &oldDef)))
+ if (!(dom = virDomainObjListAddLocked(doms, def, xmlopt, 0)))
goto error;
+ virDomainObjAssignDef(dom, def, false, &oldDef);
+
dom->autostart = autostart;
if (notify)
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index c1ffee76ad..552a7cfaf2 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -44,8 +44,7 @@ enum {
virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virDomainDefPtr def,
virDomainXMLOptionPtr xmlopt,
- unsigned int flags,
- virDomainDefPtr *oldDef);
+ unsigned int flags);
typedef int (*virDomainObjListRenameCallback)(virDomainObjPtr dom,
const char *new_name,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 2b9c6f1866..809d298ac1 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -603,9 +603,10 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- 0,
- &oldDef)))
+ 0)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -1031,10 +1032,10 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -1951,10 +1952,10 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -2851,9 +2852,10 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- 0,
- &oldDef)))
+ 0)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 76bcb66a11..35403380d0 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -568,10 +568,10 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
if (!(vm = virDomainObjListAdd(driver->domains, *def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
+
+ virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
/*
@@ -677,10 +677,10 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
if (!(vm = virDomainObjListAdd(driver->domains, *def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
+
+ virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
/*
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 9db2a02dee..eaf26563f5 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -445,10 +445,10 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags)
}
if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- 0, &oldDef)))
+ driver->xmlopt, 0)))
goto cleanup;
+ virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -1193,10 +1193,10 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index be5f89ea45..2ee8af067c 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -517,7 +517,8 @@ int openvzLoadDomains(struct openvz_driver *driver)
line = outbuf;
while (line[0] != '\0') {
- unsigned int flags = 0;
+ bool active = false;
+
if (virStrToLong_i(line, &status, 10, &veid) < 0 ||
*status++ != ' ' ||
(line = strchr(status, '\n')) == NULL) {
@@ -578,17 +579,16 @@ int openvzLoadDomains(struct openvz_driver *driver)
openvzReadMemConf(def, veid);
virUUIDFormat(def->uuid, uuidstr);
- flags = VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE;
- if (STRNEQ(status, "stopped"))
- flags |= VIR_DOMAIN_OBJ_LIST_ADD_LIVE;
+ active = STRNEQ(status, "stopped");
if (!(dom = virDomainObjListAdd(driver->domains,
def,
driver->xmlopt,
- flags,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ virDomainObjAssignDef(dom, def, active, NULL);
+
if (STREQ(status, "stopped")) {
virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
VIR_DOMAIN_SHUTOFF_UNKNOWN);
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 39eeb2c12e..6d4f7325b5 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -918,9 +918,10 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int fla
goto cleanup;
if (!(vm = virDomainObjListAdd(driver->domains, vmdef,
- driver->xmlopt,
- 0, NULL)))
+ driver->xmlopt, 0)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
vmdef = NULL;
vm->persistent = 1;
@@ -1007,10 +1008,10 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml,
if (!(vm = virDomainObjListAdd(driver->domains,
vmdef,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, vmdef, true, NULL);
vmdef = NULL;
/* All OpenVZ domains seem to be persistent - this is a bit of a violation
* of this libvirt API which is intended for transient domain creation */
@@ -2174,10 +2175,10 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (!uri_in) {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 42b1ce2521..8bbac339e0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1703,10 +1703,10 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START,
@@ -6973,10 +6973,10 @@ qemuDomainRestoreFlags(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (flags & VIR_DOMAIN_SAVE_RUNNING)
@@ -7648,9 +7648,10 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
goto cleanup;
if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- 0, &oldDef)))
+ driver->xmlopt, 0)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -16885,11 +16886,10 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 32b3040473..9e19c923ee 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2405,10 +2405,10 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
if (!(vm = virDomainObjListAdd(driver->domains, *def,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
priv = vm->privateData;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index cae2521b21..1215deffd3 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -913,11 +913,13 @@ testParseDomains(testDriverPtr privconn,
!(obj = virDomainObjListAdd(privconn->domains,
def,
privconn->xmlopt,
- 0, NULL))) {
+ 0))) {
virDomainDefFree(def);
goto error;
}
+ virDomainObjAssignDef(obj, def, false, NULL);
+
if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0)
goto error;
@@ -1620,10 +1622,10 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
if (!(dom = virDomainObjListAdd(privconn->domains,
def,
privconn->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(dom, def, true, NULL);
def = NULL;
if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) {
@@ -2167,10 +2169,10 @@ testDomainRestoreFlags(virConnectPtr conn,
if (!(dom = virDomainObjListAdd(privconn->domains,
def,
privconn->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+
+ virDomainObjAssignDef(dom, def, true, NULL);
def = NULL;
if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) {
@@ -2746,9 +2748,10 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn,
if (!(dom = virDomainObjListAdd(privconn->domains,
def,
privconn->xmlopt,
- 0,
- &oldDef)))
+ 0)))
goto cleanup;
+
+ virDomainObjAssignDef(dom, def, false, &oldDef);
def = NULL;
dom->persistent = 1;
diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c
index 963e7a9876..3ab7125b60 100644
--- a/src/vmware/vmware_conf.c
+++ b/src/vmware/vmware_conf.c
@@ -169,10 +169,10 @@ vmwareLoadDomains(struct vmware_driver *driver)
}
if (!(vm = virDomainObjListAdd(driver->domains, vmdef,
- driver->xmlopt,
- 0, NULL)))
+ driver->xmlopt, 0)))
goto cleanup;
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
pDomain = vm->privateData;
if (VIR_STRDUP(pDomain->vmxPath, vmxPath) < 0)
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 1bc8a06c39..d27313095d 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -442,10 +442,10 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int fla
if (!(vm = virDomainObjListAdd(driver->domains,
vmdef,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ virDomainObjAssignDef(vm, vmdef, false, NULL);
pDomain = vm->privateData;
if (VIR_STRDUP(pDomain->vmxPath, vmxPath) < 0)
goto cleanup;
@@ -702,11 +702,10 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml,
if (!(vm = virDomainObjListAdd(driver->domains,
vmdef,
driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
- NULL)))
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ virDomainObjAssignDef(vm, vmdef, true, NULL);
pDomain = vm->privateData;
if (VIR_STRDUP(pDomain->vmxPath, vmxPath) < 0)
goto cleanup;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 478443298f..4e934ec526 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1955,9 +1955,11 @@ prlsdkLoadDomain(vzDriverPtr driver,
virObjectLock(driver);
if (!(olddom = virDomainObjListFindByUUID(driver->domains, def->uuid)))
- dom = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0,
NULL);
+ dom = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0);
virObjectUnlock(driver);
+ virDomainObjAssignDef(dom, def, false, NULL);
+
if (olddom) {
virDomainDefFree(def);
return olddom;
--
2.21.0