4:33 a.m.
This is an alternative proposal to:
https://www.redhat.com/archives/libvir-list/2019-May/msg00830.html
The problem I'm trying to fix is described here:
https://www.redhat.com/archives/libvir-list/2019-May/msg00810.html
Michal Prívozník (9):
virDomainObjListAddLocked: Drop useless @cleanup label
virDomainObjListAddObjLocked: Don't expect vm->def to be set
virDomainObjListAddLocked: Set vm->def only in success path
virDomainObjIsActive: Allow vm->def to be NULL
virDomainObjListAdd: Leave def assigning as an exercise for caller
qemu: Allow vm->def == NULL in job control APIs
qemu: Grab modify job for changing domain XML
lxc: Grab modify job for changing domain XML
libxl: Grab modify job for changing domain XML
src/bhyve/bhyve_driver.c | 10 +++++---
src/conf/domain_conf.h | 2 +-
src/conf/virdomainobjlist.c | 48 ++++++++++++++----------------------
src/conf/virdomainobjlist.h | 3 +--
src/libxl/libxl_domain.c | 3 ++-
src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++------------
src/libxl/libxl_migration.c | 14 +++++------
src/lxc/lxc_domain.c | 3 ++-
src/lxc/lxc_driver.c | 23 +++++++++++------
src/openvz/openvz_conf.c | 12 ++++-----
src/openvz/openvz_driver.c | 17 +++++++------
src/qemu/qemu_domain.c | 30 ++++++++++++++---------
src/qemu/qemu_driver.c | 49 ++++++++++++++++++++++++++-----------
src/qemu/qemu_migration.c | 13 +++++++---
src/test/test_driver.c | 21 +++++++++-------
src/vmware/vmware_conf.c | 4 +--
src/vmware/vmware_driver.c | 9 +++----
src/vz/vz_sdk.c | 4 ++-
18 files changed, 183 insertions(+), 130 deletions(-)
--
2.21.0
4:33 a.m.
New subject: [libvirt] [PATCH 1/9] virDomainObjListAddLocked: Drop useless @cleanup label
It's a premature optimization. It's perfectly acceptable for
'error' label to deal with @vm == NULL case.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index f5a7ecc4a4..d58d25f847 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -332,7 +332,7 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
}
if (!(vm = virDomainObjNew(xmlopt)))
- goto cleanup;
+ goto error;
vm->def = def;
if (virDomainObjListAddObjLocked(doms, vm) < 0) {
@@ -340,7 +340,7 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
goto error;
}
}
- cleanup:
+
return vm;
error:
--
2.21.0
2:40 a.m.
New subject: [libvirt] [PATCH 1/9] virDomainObjListAddLocked: Drop useless @cleanup label
On 30.05.2019 12:33, Michal Privoznik wrote:
It's a premature optimization. It's perfectly acceptable for
'error' label to deal with @vm == NULL case.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:33 a.m.
New subject: [libvirt] [PATCH 2/9] virDomainObjListAddObjLocked: Don't expect vm->def to be set
In near future vm->def might be not set when calling this
function. Therefore, have caller explicitly state what UUID and
name the domain object should be stored under.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index d58d25f847..28cccd0035 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -223,6 +223,8 @@ virDomainObjListFindByName(virDomainObjListPtr doms,
/**
* @doms: Domain object list pointer
* @vm: Domain object to be added
+ * @uuid: domain UUID
+ * @name: domain name
*
* Upon entry @vm should have at least 1 ref and be locked.
*
@@ -238,16 +240,18 @@ virDomainObjListFindByName(virDomainObjListPtr doms,
*/
static int
virDomainObjListAddObjLocked(virDomainObjListPtr doms,
- virDomainObjPtr vm)
+ virDomainObjPtr vm,
+ const unsigned char *uuid,
+ const char *name)
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(vm->def->uuid, uuidstr);
+ virUUIDFormat(uuid, uuidstr);
if (virHashAddEntry(doms->objs, uuidstr, vm) < 0)
return -1;
virObjectRef(vm);
- if (virHashAddEntry(doms->objsName, vm->def->name, vm) < 0) {
+ if (virHashAddEntry(doms->objsName, name, vm) < 0) {
virHashRemoveEntry(doms->objs, uuidstr);
return -1;
}
@@ -335,7 +339,7 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
goto error;
vm->def = def;
- if (virDomainObjListAddObjLocked(doms, vm) < 0) {
+ if (virDomainObjListAddObjLocked(doms, vm, def->uuid, def->name) < 0) {
vm->def = NULL;
goto error;
}
@@ -564,7 +568,7 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms,
goto error;
}
- if (virDomainObjListAddObjLocked(doms, obj) < 0)
+ if (virDomainObjListAddObjLocked(doms, obj, obj->def->uuid,
obj->def->name) < 0)
goto error;
if (notify)
--
2.21.0
2:44 a.m.
New subject: [libvirt] [PATCH 2/9] virDomainObjListAddObjLocked: Don't expect vm->def to be set
On 30.05.2019 12:33, Michal Privoznik wrote:
In near future vm->def might be not set when calling this
function. Therefore, have caller explicitly state what UUID and
name the domain object should be stored under.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:33 a.m.
New subject: [libvirt] [PATCH 3/9] virDomainObjListAddLocked: Set vm->def only in success path
Because of past limitation of virDomainObjListAddObjLocked() we
had to set vm->def even before the object was on the domains
list.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 28cccd0035..1086aec421 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -337,12 +337,11 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
if (!(vm = virDomainObjNew(xmlopt)))
goto error;
+
+ if (virDomainObjListAddObjLocked(doms, vm, def->uuid, def->name) < 0)
+ goto error;
+
vm->def = def;
-
- if (virDomainObjListAddObjLocked(doms, vm, def->uuid, def->name) < 0) {
- vm->def = NULL;
- goto error;
- }
}
return vm;
--
2.21.0
2:46 a.m.
New subject: [libvirt] [PATCH 3/9] virDomainObjListAddLocked: Set vm->def only in success path
On 30.05.2019 12:33, Michal Privoznik wrote:
Because of past limitation of virDomainObjListAddObjLocked() we
had to set vm->def even before the object was on the domains
list.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:33 a.m.
New subject: [libvirt] [PATCH 4/9] virDomainObjIsActive: Allow vm->def to be NULL
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fa0756b634..e1b5df7e46 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2766,7 +2766,7 @@ int virDomainDefValidate(virDomainDefPtr def,
static inline bool
virDomainObjIsActive(virDomainObjPtr dom)
{
- return dom->def->id != -1;
+ return dom->def && dom->def->id != -1;
}
int virDomainObjCheckActive(virDomainObjPtr dom);
--
2.21.0
2:47 a.m.
New subject: [libvirt] [PATCH 4/9] virDomainObjIsActive: Allow vm->def to be NULL
On 30.05.2019 12:33, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fa0756b634..e1b5df7e46 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2766,7 +2766,7 @@ int virDomainDefValidate(virDomainDefPtr def,
static inline bool
virDomainObjIsActive(virDomainObjPtr dom)
{
- return dom->def->id != -1;
+ return dom->def && dom->def->id != -1;
}
int virDomainObjCheckActive(virDomainObjPtr dom);
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:34 a.m.
New subject: [libvirt] [PATCH 5/9] virDomainObjListAdd: Leave def assigning as an exercise for caller
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>
---
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 2f58a1da95..e353d7a300 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) {
@@ -2143,10 +2145,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) {
@@ -2722,9 +2724,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
3:04 a.m.
New subject: [libvirt] [PATCH 5/9] virDomainObjListAdd: Leave def assigning as an exercise for caller
On 30.05.2019 12:34, Michal Privoznik wrote:
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>
---
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(-)
Let's remove now unused VIR_DOMAIN_OBJ_LIST_ADD_LIVE entirely.
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 552a7cf..54e7871 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -38,8 +38,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
const char *name);
enum {
- VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 0),
};
virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
virDomainDefPtr def,
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:34 a.m.
New subject: [libvirt] [PATCH 6/9] qemu: Allow vm->def == NULL in job control APIs
Soon we will be acquiring job for every virDomainObjAssignDef()
call. This, however, means that in some cases vm->def might not
be set just yet. Fortunately, the only thing from domain
definition used in qemuDomainObjBeginJob()/qemuDomainObjEndJob()
(and friends) is the domain name and even that is used only for
debug printings. Therefore, we can safely replace obj->def->name
with some NULLSTR() magic.
There is one other place where vm->def might be used -
qemuDomainObjSaveJob() - but this function does something only
for active domains which already have vm->def pointer set.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e50e84a3b2..c61d39b12b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7175,6 +7175,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
const char *blocker = NULL;
const char *agentBlocker = NULL;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
int ret = -1;
unsigned long long duration = 0;
unsigned long long agentDuration = 0;
@@ -7185,7 +7186,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(asyncJob),
- obj, obj->def->name,
+ obj, domName,
qemuDomainJobTypeToString(priv->job.active),
qemuDomainAgentJobTypeToString(priv->job.agentActive),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
@@ -7209,7 +7210,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
if (nowait)
goto cleanup;
- VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj,
obj->def->name);
+ VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, domName);
if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then)
< 0)
goto error;
}
@@ -7218,7 +7219,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
if (nowait)
goto cleanup;
- VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj,
obj->def->name);
+ VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, domName);
if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) <
0)
goto error;
}
@@ -7237,7 +7238,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)",
qemuDomainJobTypeToString(job),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
priv->job.active = job;
priv->job.owner = virThreadSelfID();
priv->job.ownerAPI = virThreadJobGet();
@@ -7245,7 +7246,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
} else {
VIR_DEBUG("Started async job: %s (vm=%p name=%s)",
qemuDomainAsyncJobTypeToString(asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetAsyncJob(priv);
if (VIR_ALLOC(priv->job.current) < 0)
goto cleanup;
@@ -7263,7 +7264,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
VIR_DEBUG("Started agent job: %s (vm=%p name=%s job=%s async=%s)",
qemuDomainAgentJobTypeToString(agentJob),
- obj, obj->def->name,
+ obj, domName,
qemuDomainJobTypeToString(priv->job.active),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
priv->job.agentActive = agentJob;
@@ -7294,7 +7295,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(asyncJob),
- obj->def->name,
+ domName,
qemuDomainJobTypeToString(priv->job.active),
qemuDomainAgentJobTypeToString(priv->job.agentActive),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -7507,13 +7508,14 @@ qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainJob job = priv->job.active;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
VIR_DEBUG("Stopping job: %s (async=%s vm=%p name=%s)",
qemuDomainJobTypeToString(job),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetJob(priv);
if (qemuDomainTrackJob(job))
@@ -7528,13 +7530,14 @@ qemuDomainObjEndAgentJob(virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainAgentJob agentJob = priv->job.agentActive;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
VIR_DEBUG("Stopping agent job: %s (async=%s vm=%p name=%s)",
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetAgentJob(priv);
/* We indeed need to wake up ALL threads waiting because
@@ -7549,6 +7552,7 @@ qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainJob job = priv->job.active;
qemuDomainAgentJob agentJob = priv->job.agentActive;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
@@ -7556,7 +7560,7 @@ qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetJob(priv);
qemuDomainObjResetAgentJob(priv);
@@ -7571,12 +7575,13 @@ void
qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
VIR_DEBUG("Stopping async job: %s (vm=%p name=%s)",
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetAsyncJob(priv);
qemuDomainObjSaveJob(driver, obj);
--
2.21.0
3:29 a.m.
New subject: [libvirt] [PATCH 6/9] qemu: Allow vm->def == NULL in job control APIs
On 30.05.2019 12:34, Michal Privoznik wrote:
Soon we will be acquiring job for every virDomainObjAssignDef()
call. This, however, means that in some cases vm->def might not
be set just yet. Fortunately, the only thing from domain
definition used in qemuDomainObjBeginJob()/qemuDomainObjEndJob()
(and friends) is the domain name and even that is used only for
debug printings. Therefore, we can safely replace obj->def->name
with some NULLSTR() magic.
There is one other place where vm->def might be used -
qemuDomainObjSaveJob() - but this function does something only
for active domains which already have vm->def pointer set.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e50e84a3b2..c61d39b12b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7175,6 +7175,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
const char *blocker = NULL;
const char *agentBlocker = NULL;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
int ret = -1;
unsigned long long duration = 0;
unsigned long long agentDuration = 0;
@@ -7185,7 +7186,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(asyncJob),
- obj, obj->def->name,
+ obj, domName,
qemuDomainJobTypeToString(priv->job.active),
qemuDomainAgentJobTypeToString(priv->job.agentActive),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
@@ -7209,7 +7210,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
if (nowait)
goto cleanup;
- VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj,
obj->def->name);
+ VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, domName);
if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock,
then) < 0)
goto error;
}
@@ -7218,7 +7219,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
if (nowait)
goto cleanup;
- VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj,
obj->def->name);
+ VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, domName);
if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then)
< 0)
goto error;
}
@@ -7237,7 +7238,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)",
qemuDomainJobTypeToString(job),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
priv->job.active = job;
priv->job.owner = virThreadSelfID();
priv->job.ownerAPI = virThreadJobGet();
@@ -7245,7 +7246,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
} else {
VIR_DEBUG("Started async job: %s (vm=%p name=%s)",
qemuDomainAsyncJobTypeToString(asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetAsyncJob(priv);
if (VIR_ALLOC(priv->job.current) < 0)
goto cleanup;
@@ -7263,7 +7264,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
VIR_DEBUG("Started agent job: %s (vm=%p name=%s job=%s async=%s)",
qemuDomainAgentJobTypeToString(agentJob),
- obj, obj->def->name,
+ obj, domName,
qemuDomainJobTypeToString(priv->job.active),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
priv->job.agentActive = agentJob;
@@ -7294,7 +7295,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(asyncJob),
- obj->def->name,
+ domName,
qemuDomainJobTypeToString(priv->job.active),
qemuDomainAgentJobTypeToString(priv->job.agentActive),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
@@ -7507,13 +7508,14 @@ qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr
obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainJob job = priv->job.active;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
VIR_DEBUG("Stopping job: %s (async=%s vm=%p name=%s)",
qemuDomainJobTypeToString(job),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetJob(priv);
if (qemuDomainTrackJob(job))
@@ -7528,13 +7530,14 @@ qemuDomainObjEndAgentJob(virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainAgentJob agentJob = priv->job.agentActive;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
VIR_DEBUG("Stopping agent job: %s (async=%s vm=%p name=%s)",
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetAgentJob(priv);
/* We indeed need to wake up ALL threads waiting because
@@ -7549,6 +7552,7 @@ qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainJob job = priv->job.active;
qemuDomainAgentJob agentJob = priv->job.agentActive;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
@@ -7556,7 +7560,7 @@ qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetJob(priv);
qemuDomainObjResetAgentJob(priv);
@@ -7571,12 +7575,13 @@ void
qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
priv->jobs_queued--;
VIR_DEBUG("Stopping async job: %s (vm=%p name=%s)",
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
- obj, obj->def->name);
+ obj, domName);
qemuDomainObjResetAsyncJob(priv);
qemuDomainObjSaveJob(driver, obj);
Printing NULL instead of name does not look nice to me even this is just debug messages.
AFAIK in case of qemu driver we drop the lock during jobs only for active domains so
on defineXML we can acquire job only for active domains too, then we don't need to
print NULLs.
Not sure about other drivers though...
Nikolay
4:34 a.m.
New subject: [libvirt] [PATCH 7/9] qemu: Grab modify job for changing domain XML
Changing domain definition must be guarded with acquiring modify
job. The problem is if there is a thread executing say
qemuDomainSetMemoryStatsPeriod() which is an API that acquires
modify job and then possibly unlock the domain object and locks
it again. However, becasue virDomainObjAssignDef() does not take
a job (only object lock) it may have changed the domain
definition while the other thread unlocked the domain object in
order to talk on the monitor. For instance:
Thread1:
1) lookup domain object and lock it
2) acquire job
3) get pointers to live and persistent defs
4) unlock the domain object
5) talk to qemu on the monitor
Thread2 (Execute qemuDomainDefineXMLFlags):
1) lookup domain object and lock it
2) virDomainObjAssignDef() which overwrites persistent def and
frees the old one
3) unlock domain object
Thread1:
6) lock the domain object again
7) try to access the persistent def via pointer stored in 3)
Now this will crash because the pointer from step 3) points to a
memory that was freed.
However, if Thread2 waited and acquired modify job (just like
Thread1) then these two would serialize and no harm would be
done.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 3 ++-
src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++----
src/qemu/qemu_migration.c | 7 +++++++
3 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c61d39b12b..fff2f1932e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8784,7 +8784,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
return;
}
- qemuDomainRemoveInactiveCommon(driver, vm);
+ if (vm->def)
+ qemuDomainRemoveInactiveCommon(driver, vm);
virDomainObjListRemove(driver->domains, vm);
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8bbac339e0..77cb7e4b87 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1706,9 +1706,16 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
+ qemuDomainObjEndJob(driver, vm);
+
if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START,
flags) < 0) {
qemuDomainRemoveInactiveJob(driver, vm);
@@ -6976,6 +6983,11 @@ qemuDomainRestoreFlags(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
@@ -6988,6 +7000,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
priv = vm->privateData;
priv->hookRun = true;
}
+ qemuDomainObjEndJob(driver, vm);
if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_RESTORE,
flags) < 0)
@@ -7651,6 +7664,11 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
driver->xmlopt, 0)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
@@ -7673,7 +7691,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
vm->persistent = 0;
qemuDomainRemoveInactiveJob(driver, vm);
}
- goto cleanup;
+ goto endjob;
}
event = virDomainEventLifecycleNewFromObj(vm,
@@ -7685,6 +7703,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
VIR_INFO("Creating domain '%s'", vm->def->name);
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
+ endjob:
+ qemuDomainObjEndJob(driver, vm);
+
cleanup:
virDomainDefFree(oldDef);
virDomainDefFree(def);
@@ -16889,14 +16910,14 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
- virDomainObjAssignDef(vm, def, true, NULL);
- def = NULL;
-
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
qemuDomainRemoveInactive(driver, vm);
goto cleanup;
}
+ virDomainObjAssignDef(vm, def, true, NULL);
+ def = NULL;
+
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
qemuDomainRemoveInactive(driver, vm);
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9e19c923ee..32325c9588 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2408,9 +2408,16 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
+ qemuDomainObjEndJob(driver, vm);
+
priv = vm->privateData;
if (VIR_STRDUP(priv->origname, origname) < 0)
goto cleanup;
--
2.21.0
4:07 a.m.
New subject: [libvirt] [PATCH 7/9] qemu: Grab modify job for changing domain XML
On 30.05.2019 12:34, Michal Privoznik wrote:
Changing domain definition must be guarded with acquiring modify
job. The problem is if there is a thread executing say
qemuDomainSetMemoryStatsPeriod() which is an API that acquires
modify job and then possibly unlock the domain object and locks
it again. However, becasue virDomainObjAssignDef() does not take
a job (only object lock) it may have changed the domain
definition while the other thread unlocked the domain object in
order to talk on the monitor. For instance:
Thread1:
1) lookup domain object and lock it
2) acquire job
3) get pointers to live and persistent defs
4) unlock the domain object
5) talk to qemu on the monitor
Thread2 (Execute qemuDomainDefineXMLFlags):
1) lookup domain object and lock it
2) virDomainObjAssignDef() which overwrites persistent def and
frees the old one
3) unlock domain object
Thread1:
6) lock the domain object again
7) try to access the persistent def via pointer stored in 3)
Now this will crash because the pointer from step 3) points to a
memory that was freed.
However, if Thread2 waited and acquired modify job (just like
Thread1) then these two would serialize and no harm would be
done.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 3 ++-
src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++----
src/qemu/qemu_migration.c | 7 +++++++
3 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c61d39b12b..fff2f1932e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8784,7 +8784,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
return;
}
- qemuDomainRemoveInactiveCommon(driver, vm);
+ if (vm->def)
+ qemuDomainRemoveInactiveCommon(driver, vm);
virDomainObjListRemove(driver->domains, vm);
Hmm, virDomainObjListRemoveLocked uses vm->def too.
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8bbac339e0..77cb7e4b87 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1706,9 +1706,16 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
+ qemuDomainObjEndJob(driver, vm);
+
Looks like we can begin VIR_DOMAIN_JOB_OPERATION_START from the start instead
of acquiring job twice.
if (qemuProcessBeginJob(driver, vm,
VIR_DOMAIN_JOB_OPERATION_START,
flags) < 0) {
qemuDomainRemoveInactiveJob(driver, vm);
@@ -6976,6 +6983,11 @@ qemuDomainRestoreFlags(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
@@ -6988,6 +7000,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
priv = vm->privateData;
priv->hookRun = true;
}> + qemuDomainObjEndJob(driver, vm);
Same here
if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_RESTORE,
flags) < 0)
@@ -7651,6 +7664,11 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
driver->xmlopt, 0)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
@@ -7673,7 +7691,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
vm->persistent = 0;
qemuDomainRemoveInactiveJob(driver, vm);
}
- goto cleanup;
+ goto endjob;
}
event = virDomainEventLifecycleNewFromObj(vm,
@@ -7685,6 +7703,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
VIR_INFO("Creating domain '%s'", vm->def->name);
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
+ endjob:
+ qemuDomainObjEndJob(driver, vm);
+
cleanup:
virDomainDefFree(oldDef);
virDomainDefFree(def);
Additionally we need to replace qemuDomainRemoveInactiveJob with qemuDomainRemoveInactive
as we have a job already.
@@ -16889,14 +16910,14 @@ static virDomainPtr
qemuDomainQemuAttach(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
- virDomainObjAssignDef(vm, def, true, NULL);
- def = NULL;
-
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
qemuDomainRemoveInactive(driver, vm);
goto cleanup;
}
+ virDomainObjAssignDef(vm, def, true, NULL);
+ def = NULL;
+
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
qemuDomainRemoveInactive(driver, vm);
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9e19c923ee..32325c9588 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2408,9 +2408,16 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
+ qemuDomainObjEndJob(driver, vm);
+
priv = vm->privateData;
if (VIR_STRDUP(priv->origname, origname) < 0)
goto cleanup;
On migration we acquire job already too ...
Nikolay
9:23 a.m.
New subject: [libvirt] [PATCH 7/9] qemu: Grab modify job for changing domain XML
On 6/4/19 11:07 AM, Nikolay Shirokovskiy wrote:
On 30.05.2019 12:34, Michal Privoznik wrote:
> Changing domain definition must be guarded with acquiring modify
> job. The problem is if there is a thread executing say
> qemuDomainSetMemoryStatsPeriod() which is an API that acquires
> modify job and then possibly unlock the domain object and locks
> it again. However, becasue virDomainObjAssignDef() does not take
> a job (only object lock) it may have changed the domain
> definition while the other thread unlocked the domain object in
> order to talk on the monitor. For instance:
>
> Thread1:
> 1) lookup domain object and lock it
> 2) acquire job
> 3) get pointers to live and persistent defs
> 4) unlock the domain object
> 5) talk to qemu on the monitor
>
> Thread2 (Execute qemuDomainDefineXMLFlags):
> 1) lookup domain object and lock it
> 2) virDomainObjAssignDef() which overwrites persistent def and
> frees the old one
> 3) unlock domain object
>
> Thread1:
> 6) lock the domain object again
> 7) try to access the persistent def via pointer stored in 3)
>
> Now this will crash because the pointer from step 3) points to a
> memory that was freed.
>
> However, if Thread2 waited and acquired modify job (just like
> Thread1) then these two would serialize and no harm would be
> done.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 3 ++-
> src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++----
> src/qemu/qemu_migration.c | 7 +++++++
> 3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c61d39b12b..fff2f1932e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8784,7 +8784,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
> return;
> }
>
> - qemuDomainRemoveInactiveCommon(driver, vm);
> + if (vm->def)
> + qemuDomainRemoveInactiveCommon(driver, vm);
>
> virDomainObjListRemove(driver->domains, vm);
Hmm, virDomainObjListRemoveLocked uses vm->def too.
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8bbac339e0..77cb7e4b87 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1706,9 +1706,16 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
> VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
> goto cleanup;
>
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
> + qemuDomainRemoveInactive(driver, vm);
> + goto cleanup;
> + }
> +
> virDomainObjAssignDef(vm, def, true, NULL);
> def = NULL;
>
> + qemuDomainObjEndJob(driver, vm);
> +
Looks like we can begin VIR_DOMAIN_JOB_OPERATION_START from the start instead
of acquiring job twice.
That is an async job. We shouldn't rely on async job when modifying a
domain.
> if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START,
> flags) < 0) {
> qemuDomainRemoveInactiveJob(driver, vm);
> @@ -6976,6 +6983,11 @@ qemuDomainRestoreFlags(virConnectPtr conn,
> VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
> goto cleanup;
>
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
> + qemuDomainRemoveInactive(driver, vm);
> + goto cleanup;
> + }
> +
> virDomainObjAssignDef(vm, def, true, NULL);
> def = NULL;
>
> @@ -6988,6 +7000,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
> priv = vm->privateData;
> priv->hookRun = true;
> }> + qemuDomainObjEndJob(driver, vm);
Same here
>
> if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_RESTORE,
> flags) < 0)
> @@ -7651,6 +7664,11 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
> driver->xmlopt, 0)))
> goto cleanup;
>
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
> + qemuDomainRemoveInactive(driver, vm);
> + goto cleanup;
> + }
> +
> virDomainObjAssignDef(vm, def, false, &oldDef);
> def = NULL;
>
> @@ -7673,7 +7691,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
> vm->persistent = 0;
> qemuDomainRemoveInactiveJob(driver, vm);
> }
> - goto cleanup;
> + goto endjob;
> }
>
> event = virDomainEventLifecycleNewFromObj(vm,
> @@ -7685,6 +7703,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
> VIR_INFO("Creating domain '%s'", vm->def->name);
> dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
>
> + endjob:
> + qemuDomainObjEndJob(driver, vm);
> +
> cleanup:
> virDomainDefFree(oldDef);
> virDomainDefFree(def);
Additionally we need to replace qemuDomainRemoveInactiveJob with
qemuDomainRemoveInactive
as we have a job already.
I'm planning on wrapping what you suggested in your reply to the cover
letter into a single function. This way, the change won't be that
intrusive and a lot of these changes won't be made.
Michal
9:30 a.m.
New subject: [libvirt] [PATCH 7/9] qemu: Grab modify job for changing domain XML
On 04.06.2019 17:23, Michal Privoznik wrote:
On 6/4/19 11:07 AM, Nikolay Shirokovskiy wrote:
>
>
> On 30.05.2019 12:34, Michal Privoznik wrote:
>> Changing domain definition must be guarded with acquiring modify
>> job. The problem is if there is a thread executing say
>> qemuDomainSetMemoryStatsPeriod() which is an API that acquires
>> modify job and then possibly unlock the domain object and locks
>> it again. However, becasue virDomainObjAssignDef() does not take
>> a job (only object lock) it may have changed the domain
>> definition while the other thread unlocked the domain object in
>> order to talk on the monitor. For instance:
>>
>> Thread1:
>> 1) lookup domain object and lock it
>> 2) acquire job
>> 3) get pointers to live and persistent defs
>> 4) unlock the domain object
>> 5) talk to qemu on the monitor
>>
>> Thread2 (Execute qemuDomainDefineXMLFlags):
>> 1) lookup domain object and lock it
>> 2) virDomainObjAssignDef() which overwrites persistent def and
>> frees the old one
>> 3) unlock domain object
>>
>> Thread1:
>> 6) lock the domain object again
>> 7) try to access the persistent def via pointer stored in 3)
>>
>> Now this will crash because the pointer from step 3) points to a
>> memory that was freed.
>>
>> However, if Thread2 waited and acquired modify job (just like
>> Thread1) then these two would serialize and no harm would be
>> done.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/qemu/qemu_domain.c | 3 ++-
>> src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++----
>> src/qemu/qemu_migration.c | 7 +++++++
>> 3 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index c61d39b12b..fff2f1932e 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -8784,7 +8784,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
>> return;
>> }
>> - qemuDomainRemoveInactiveCommon(driver, vm);
>> + if (vm->def)
>> + qemuDomainRemoveInactiveCommon(driver, vm);
>> virDomainObjListRemove(driver->domains, vm);
>
> Hmm, virDomainObjListRemoveLocked uses vm->def too.
>
>> }
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 8bbac339e0..77cb7e4b87 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1706,9 +1706,16 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr
conn,
>> VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
>> goto cleanup;
>> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
>> + qemuDomainRemoveInactive(driver, vm);
>> + goto cleanup;
>> + }
>> +
>> virDomainObjAssignDef(vm, def, true, NULL);
>> def = NULL;
>> + qemuDomainObjEndJob(driver, vm);
>> +
>
> Looks like we can begin VIR_DOMAIN_JOB_OPERATION_START from the start instead
> of acquiring job twice.
That is an async job. We shouldn't rely on async job when modifying a domain.
To me it looks ok. We don't drop vm lock between begin job and assigning definition
so other threads have no chance to see invalid state.
Nikolay
4:34 a.m.
New subject: [libvirt] [PATCH 8/9] lxc: Grab modify job for changing domain XML
The reasoning here is the same as in qemu driver fixed in
previous commit. Long story short, changing an XML of a domain
requires modify job to be acquired.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/lxc/lxc_domain.c | 3 ++-
src/lxc/lxc_driver.c | 23 ++++++++++++++++-------
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index 51a9fd36eb..609cfa6dae 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -93,6 +93,7 @@ virLXCDomainObjBeginJob(virLXCDriverPtr driver ATTRIBUTE_UNUSED,
virLXCDomainObjPrivatePtr priv = obj->privateData;
unsigned long long now;
unsigned long long then;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
if (virTimeMillisNow(&now) < 0)
return -1;
@@ -117,7 +118,7 @@ virLXCDomainObjBeginJob(virLXCDriverPtr driver ATTRIBUTE_UNUSED,
VIR_WARN("Cannot start job (%s) for domain %s;"
" current job is (%s) owned by (%d)",
virLXCDomainJobTypeToString(job),
- obj->def->name,
+ domName,
virLXCDomainJobTypeToString(priv->job.active),
priv->job.owner);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index eaf26563f5..2bd1274c90 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -448,6 +448,12 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
driver->xmlopt, 0)))
goto cleanup;
+ if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
+ if (!vm->persistent)
+ virDomainObjListRemove(driver->domains, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -455,7 +461,7 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
if (virDomainSaveConfig(cfg->configDir, driver->caps,
vm->newDef ? vm->newDef : vm->def) < 0) {
virDomainObjListRemove(driver->domains, vm);
- goto cleanup;
+ goto endjob;
}
event = virDomainEventLifecycleNewFromObj(vm,
@@ -466,6 +472,9 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
+ endjob:
+ virLXCDomainObjEndJob(driver, vm);
+
cleanup:
virDomainDefFree(def);
virDomainDefFree(oldDef);
@@ -1196,15 +1205,15 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
+ if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
+ if (!vm->persistent)
+ virDomainObjListRemove(driver->domains, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
- if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
- if (!vm->persistent)
- virDomainObjListRemove(driver->domains, vm);
- goto cleanup;
- }
-
if (virLXCProcessStart(conn, driver, vm,
nfiles, files,
(flags & VIR_DOMAIN_START_AUTODESTROY),
--
2.21.0
4:34 a.m.
New subject: [libvirt] [PATCH 9/9] libxl: Grab modify job for changing domain XML
The reasoning here is the same as in qemu driver fixed in
previous commit. Long story short, changing an XML of a domain
requires modify job to be acquired.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libxl/libxl_domain.c | 3 ++-
src/libxl/libxl_driver.c | 34 ++++++++++++++++++++++++----------
src/libxl/libxl_migration.c | 10 ++++------
3 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 2d8569e592..a2c90f0461 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -115,6 +115,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
libxlDomainObjPrivatePtr priv = obj->privateData;
unsigned long long now;
unsigned long long then;
+ const char *domName = NULLSTR(obj->def ? obj->def->name : NULL);
if (virTimeMillisNow(&now) < 0)
return -1;
@@ -141,7 +142,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
VIR_WARN("Cannot start job (%s) for domain %s;"
" current job is (%s) owned by (%d)",
libxlDomainJobTypeToString(job),
- obj->def->name,
+ domName,
libxlDomainJobTypeToString(priv->job.active),
priv->job.owner);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 809d298ac1..1bd7148345 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -606,22 +606,29 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
0)))
goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ virDomainObjListRemove(driver->domains, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
if (virDomainDefSetVcpusMax(vm->def, d_info.vcpu_max_id + 1, driver->xmlopt))
- goto cleanup;
+ goto endjob;
if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
- goto cleanup;
+ goto endjob;
vm->def->mem.cur_balloon = d_info.current_memkb;
if (libxlDriverGetDom0MaxmemConf(cfg, &maxmem) < 0)
maxmem = d_info.current_memkb;
virDomainDefSetMemoryTotal(vm->def, maxmem);
ret = 0;
+ endjob:
+ libxlDomainObjEndJob(driver, vm);
cleanup:
libxl_dominfo_dispose(&d_info);
@@ -1035,15 +1042,15 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
- virDomainObjAssignDef(vm, def, true, NULL);
- def = NULL;
-
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
if (!vm->persistent)
virDomainObjListRemove(driver->domains, vm);
goto cleanup;
}
+ virDomainObjAssignDef(vm, def, true, NULL);
+ def = NULL;
+
if (libxlDomainStartNew(driver, vm,
(flags & VIR_DOMAIN_START_PAUSED) != 0) < 0) {
if (!vm->persistent)
@@ -1955,15 +1962,15 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
- virDomainObjAssignDef(vm, def, true, NULL);
- def = NULL;
-
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
if (!vm->persistent)
virDomainObjListRemove(driver->domains, vm);
goto cleanup;
}
+ virDomainObjAssignDef(vm, def, true, NULL);
+ def = NULL;
+
ret = libxlDomainStartRestore(driver, vm,
(flags & VIR_DOMAIN_SAVE_PAUSED) != 0,
fd, hdr.version);
@@ -2855,16 +2862,20 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
0)))
goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ virDomainObjListRemove(driver->domains, vm);
+ goto cleanup;
+ }
+
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
-
vm->persistent = 1;
if (virDomainSaveConfig(cfg->configDir,
cfg->caps,
vm->newDef ? vm->newDef : vm->def) < 0) {
virDomainObjListRemove(driver->domains, vm);
- goto cleanup;
+ goto endjob;
}
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
@@ -2874,6 +2885,9 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
VIR_DOMAIN_EVENT_DEFINED_ADDED :
VIR_DOMAIN_EVENT_DEFINED_UPDATED);
+ endjob:
+ libxlDomainObjEndJob(driver, vm);
+
cleanup:
virDomainDefFree(def);
virDomainDefFree(oldDef);
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 35403380d0..64bc10bc34 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -571,9 +571,6 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
- virDomainObjAssignDef(vm, *def, true, NULL);
- *def = NULL;
-
/*
* Unless an error is encountered in this function, the job will
* be terminated in the finish phase.
@@ -581,6 +578,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto error;
+ virDomainObjAssignDef(vm, *def, true, NULL);
+ *def = NULL;
priv = vm->privateData;
if (taint_hook) {
@@ -680,9 +679,6 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
- virDomainObjAssignDef(vm, *def, true, NULL);
- *def = NULL;
-
/*
* Unless an error is encountered in this function, the job will
* be terminated in the finish phase.
@@ -690,6 +686,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto error;
+ virDomainObjAssignDef(vm, *def, true, NULL);
+ *def = NULL;
priv = vm->privateData;
if (taint_hook) {
--
2.21.0
5:53 a.m.
On 30.05.2019 12:33, Michal Privoznik wrote:
This is an alternative proposal to:
https://www.redhat.com/archives/libvir-list/2019-May/msg00830.html
The problem I'm trying to fix is described here:
https://www.redhat.com/archives/libvir-list/2019-May/msg00810.html
Michal Prívozník (9):
virDomainObjListAddLocked: Drop useless @cleanup label
virDomainObjListAddObjLocked: Don't expect vm->def to be set
virDomainObjListAddLocked: Set vm->def only in success path
virDomainObjIsActive: Allow vm->def to be NULL
virDomainObjListAdd: Leave def assigning as an exercise for caller
qemu: Allow vm->def == NULL in job control APIs
qemu: Grab modify job for changing domain XML
lxc: Grab modify job for changing domain XML
libxl: Grab modify job for changing domain XML
src/bhyve/bhyve_driver.c | 10 +++++---
src/conf/domain_conf.h | 2 +-
src/conf/virdomainobjlist.c | 48 ++++++++++++++----------------------
src/conf/virdomainobjlist.h | 3 +--
src/libxl/libxl_domain.c | 3 ++-
src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++------------
src/libxl/libxl_migration.c | 14 +++++------
src/lxc/lxc_domain.c | 3 ++-
src/lxc/lxc_driver.c | 23 +++++++++++------
src/openvz/openvz_conf.c | 12 ++++-----
src/openvz/openvz_driver.c | 17 +++++++------
src/qemu/qemu_domain.c | 30 ++++++++++++++---------
src/qemu/qemu_driver.c | 49 ++++++++++++++++++++++++++-----------
src/qemu/qemu_migration.c | 13 +++++++---
src/test/test_driver.c | 21 +++++++++-------
src/vmware/vmware_conf.c | 4 +--
src/vmware/vmware_driver.c | 9 +++----
src/vz/vz_sdk.c | 4 ++-
18 files changed, 183 insertions(+), 130 deletions(-)
In patches 8 and 9 there are same issues as in qemu part patches.
In order to overcome NULL names in acquire job and remove inactive domain functions
I suggest using next approach:
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt, 0)))
goto cleanup;
if (!vm->def) {
virDomainObjAssignDef(vm, def, false, NULL);
def = NULL;
}
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
qemuDomainRemoveInactive(driver, vm);
goto cleanup;
}
if (def) {
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
}
Then we don't need other patches that support case when vm->def == NULL.
First assign can be moved to virDomainObjListAddLocked to the branch
where new object is created although virDomainObjListAdd will have
a bit weird semantics in this case.
Nikolay
9:23 a.m.
On 6/4/19 12:53 PM, Nikolay Shirokovskiy wrote:
On 30.05.2019 12:33, Michal Privoznik wrote:
> This is an alternative proposal to:
>
> https://www.redhat.com/archives/libvir-list/2019-May/msg00830.html
>
> The problem I'm trying to fix is described here:
>
> https://www.redhat.com/archives/libvir-list/2019-May/msg00810.html
>
> Michal Prívozník (9):
> virDomainObjListAddLocked: Drop useless @cleanup label
> virDomainObjListAddObjLocked: Don't expect vm->def to be set
> virDomainObjListAddLocked: Set vm->def only in success path
> virDomainObjIsActive: Allow vm->def to be NULL
> virDomainObjListAdd: Leave def assigning as an exercise for caller
> qemu: Allow vm->def == NULL in job control APIs
> qemu: Grab modify job for changing domain XML
> lxc: Grab modify job for changing domain XML
> libxl: Grab modify job for changing domain XML
>
> src/bhyve/bhyve_driver.c | 10 +++++---
> src/conf/domain_conf.h | 2 +-
> src/conf/virdomainobjlist.c | 48 ++++++++++++++----------------------
> src/conf/virdomainobjlist.h | 3 +--
> src/libxl/libxl_domain.c | 3 ++-
> src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++------------
> src/libxl/libxl_migration.c | 14 +++++------
> src/lxc/lxc_domain.c | 3 ++-
> src/lxc/lxc_driver.c | 23 +++++++++++------
> src/openvz/openvz_conf.c | 12 ++++-----
> src/openvz/openvz_driver.c | 17 +++++++------
> src/qemu/qemu_domain.c | 30 ++++++++++++++---------
> src/qemu/qemu_driver.c | 49 ++++++++++++++++++++++++++-----------
> src/qemu/qemu_migration.c | 13 +++++++---
> src/test/test_driver.c | 21 +++++++++-------
> src/vmware/vmware_conf.c | 4 +--
> src/vmware/vmware_driver.c | 9 +++----
> src/vz/vz_sdk.c | 4 ++-
> 18 files changed, 183 insertions(+), 130 deletions(-)
>
In patches 8 and 9 there are same issues as in qemu part patches.
In order to overcome NULL names in acquire job and remove inactive domain functions
I suggest using next approach:
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt, 0)))
goto cleanup;
if (!vm->def) {
virDomainObjAssignDef(vm, def, false, NULL);
def = NULL;
}
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
qemuDomainRemoveInactive(driver, vm);
goto cleanup;
}
if (def) {
virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
}
Then we don't need other patches that support case when vm->def == NULL.
First assign can be moved to virDomainObjListAddLocked to the branch
where new object is created although virDomainObjListAdd will have
a bit weird semantics in this case.
And that's why I did not want to do that. Assigning definition only in
some cases is bad design IMO.
But what you suggest may actually work. Let me write the patches and
test it.
Michal
1962
days inactive
1967
days old
20 comments
2 participants
participants (2)
-
Michal Privoznik -
Nikolay Shirokovskiy