4:09 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2019-May/msg00858.html
diff to v1:
- 1/9 from the original series is pushed now. It's independent from the
feature.
- Acquiring job is done in a way suggested by Nikolay in review of v1.
Michal PrÃvoznÃk (11):
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
virDomainObjListAdd: Remove unused flag
qemu: Grab modify job for changing domain XML
qemu_domain: Allow qemuDomainObjListAdd to keep job upon return
lxc: Grab modify job for changing domain XML
lxc_domain: Allow lxcDomainObjListAdd to keep job upon return
libxl: Grab modify job for changing domain XML
libxl_domain: Allow libxlDomainObjListAdd to keep job upon return
src/bhyve/bhyve_driver.c | 10 +++---
src/conf/domain_conf.h | 2 +-
src/conf/virdomainobjlist.c | 44 +++++++++---------------
src/conf/virdomainobjlist.h | 6 ++--
src/libxl/libxl_domain.c | 68 +++++++++++++++++++++++++++++++++++++
src/libxl/libxl_domain.h | 8 +++++
src/libxl/libxl_driver.c | 40 +++++-----------------
src/libxl/libxl_migration.c | 28 +++------------
src/lxc/lxc_domain.c | 68 +++++++++++++++++++++++++++++++++++++
src/lxc/lxc_domain.h | 8 +++++
src/lxc/lxc_driver.c | 28 ++++++---------
src/openvz/openvz_conf.c | 12 +++----
src/openvz/openvz_driver.c | 17 +++++-----
src/qemu/qemu_domain.c | 66 +++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 7 ++++
src/qemu/qemu_driver.c | 43 ++++++++---------------
src/qemu/qemu_migration.c | 7 ++--
src/test/test_driver.c | 21 +++++++-----
src/vmware/vmware_conf.c | 4 +--
src/vmware/vmware_driver.c | 9 +++--
src/vz/vz_sdk.c | 4 ++-
21 files changed, 326 insertions(+), 174 deletions(-)
--
2.21.0
4:09 a.m.
New subject: [libvirt] [PATCH v2 01/11] 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>
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.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
4:09 a.m.
New subject: [libvirt] [PATCH v2 02/11] 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>
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.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
4:09 a.m.
New subject: [libvirt] [PATCH v2 03/11] virDomainObjIsActive: Allow vm->def to be NULL
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.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 4c3ab07062..92c734e13d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2767,7 +2767,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
4:09 a.m.
New subject: [libvirt] [PATCH v2 04/11] 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>
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
4:09 a.m.
New subject: [libvirt] [PATCH v2 05/11] virDomainObjListAdd: Remove unused flag
After the previous commit the VIR_DOMAIN_OBJ_LIST_ADD_LIVE flag
is not used anymore. Let's remove it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 552a7cfaf2..54e7871c67 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,
--
2.21.0
7:34 a.m.
New subject: [libvirt] [PATCH v2 05/11] virDomainObjListAdd: Remove unused flag
On 05.06.2019 12:09, Michal Privoznik wrote:
After the previous commit the VIR_DOMAIN_OBJ_LIST_ADD_LIVE flag
is not used anymore. Let's remove it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/virdomainobjlist.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 552a7cfaf2..54e7871c67 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:09 a.m.
New subject: [libvirt] [PATCH v2 06/11] 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 | 55 +++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 6 +++++
src/qemu/qemu_driver.c | 27 ++++++-------------
src/qemu/qemu_migration.c | 5 +---
4 files changed, 70 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4d3a8868b2..f6b677c69e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7055,6 +7055,61 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
};
+/**
+ * qemuDomainObjListAdd:
+ * @driver: qemu driver
+ * @def: domain definition
+ * @oldDef: previous domain definition
+ * @live: whether @def is live definition
+ * @flags: an bitwise-OR of virDomainObjListAdd flags
+ *
+ * Add a domain onto the list of domain object and sets its
+ * definition. If @oldDef is not NULL and there was pre-existing
+ * definition it's returned in @oldDef.
+ *
+ * In addition to that, if definition of an existing domain is
+ * changed a MODIFY job is acquired prior to that.
+ *
+ * Returns: domain object pointer on success,
+ * NULL otherwise.
+ */
+virDomainObjPtr
+qemuDomainObjListAdd(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags)
+{
+ virDomainObjPtr vm = NULL;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
+ return NULL;
+
+ /* At this point, @vm is locked. If it doesn't have any
+ * definition set, then the call above just added it and
+ * there can't be anybody else using the object. It's safe to
+ * just set the definition without acquiring job. */
+ if (!vm->def) {
+ virDomainObjAssignDef(vm, def, live, oldDef);
+ VIR_RETURN_PTR(vm);
+ }
+
+ /* Bad luck. Domain was pre-existing and this call is trying
+ * to update its definition. Modify job MUST be acquired. */
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ virDomainObjEndAPI(&vm);
+ return NULL;
+ }
+
+ virDomainObjAssignDef(vm, def, live, oldDef);
+
+ qemuDomainObjEndJob(driver, vm);
+
+ return vm;
+}
+
+
static void
qemuDomainObjSaveJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
{
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f92f0dbc27..f469f8eaca 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -543,6 +543,12 @@ void qemuDomainEventFlush(int timer, void *opaque);
void qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
virDomainObjPtr vm);
+virDomainObjPtr qemuDomainObjListAdd(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags);
+
int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainJob job)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8bbac339e0..fa93a686b7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1701,12 +1701,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ 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,
@@ -2405,6 +2402,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon, period);
+ sleep(60);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto endjob;
if (r < 0) {
@@ -6971,12 +6969,9 @@ qemuDomainRestoreFlags(virConnectPtr conn,
def = tmp;
}
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (flags & VIR_DOMAIN_SAVE_RUNNING)
@@ -7647,11 +7642,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt, 0)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, &oldDef, false, 0)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -16884,12 +16876,9 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
if (qemuAssignDeviceAliases(def, qemuCaps) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ 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 9e19c923ee..13f0bc7e45 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2403,12 +2403,9 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
QEMU_MIGRATION_COOKIE_CAPS)))
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, *def,
- driver->xmlopt,
+ if (!(vm = qemuDomainObjListAdd(driver, *def, NULL, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
priv = vm->privateData;
--
2.21.0
4:32 a.m.
New subject: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML
On Wed, Jun 05, 2019 at 11:09:14 +0200, 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 | 55 +++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 6 +++++
src/qemu/qemu_driver.c | 27 ++++++-------------
src/qemu/qemu_migration.c | 5 +---
4 files changed, 70 insertions(+), 23 deletions(-)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8bbac339e0..fa93a686b7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1701,12 +1701,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ 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,
@@ -2405,6 +2402,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon, period);
+ sleep(60);
Debugging leftovers?
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto endjob;
if (r < 0) {
4:35 a.m.
New subject: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML
On 6/5/19 11:32 AM, Peter Krempa wrote:
On Wed, Jun 05, 2019 at 11:09:14 +0200, 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 | 55 +++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_domain.h | 6 +++++
> src/qemu/qemu_driver.c | 27 ++++++-------------
> src/qemu/qemu_migration.c | 5 +---
> 4 files changed, 70 insertions(+), 23 deletions(-)
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8bbac339e0..fa93a686b7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1701,12 +1701,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
> if (virDomainCreateXMLEnsureACL(conn, def) < 0)
> goto cleanup;
>
> - if (!(vm = virDomainObjListAdd(driver->domains, def,
> - driver->xmlopt,
> - VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
> + if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
> + 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,
> @@ -2405,6 +2402,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
>
> qemuDomainObjEnterMonitor(driver, vm);
> r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon,
period);
> + sleep(60);
Debugging leftovers?
Oh, right O:-)
Consider removed.
Michal
7:34 a.m.
New subject: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML
On 05.06.2019 12:09, 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 | 55 +++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 6 +++++
src/qemu/qemu_driver.c | 27 ++++++-------------
src/qemu/qemu_migration.c | 5 +---
4 files changed, 70 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4d3a8868b2..f6b677c69e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7055,6 +7055,61 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
};
+/**
+ * qemuDomainObjListAdd:
+ * @driver: qemu driver
+ * @def: domain definition
+ * @oldDef: previous domain definition
+ * @live: whether @def is live definition
+ * @flags: an bitwise-OR of virDomainObjListAdd flags
+ *
+ * Add a domain onto the list of domain object and sets its
+ * definition. If @oldDef is not NULL and there was pre-existing
+ * definition it's returned in @oldDef.
+ *
+ * In addition to that, if definition of an existing domain is
+ * changed a MODIFY job is acquired prior to that.
+ *
+ * Returns: domain object pointer on success,
+ * NULL otherwise.
+ */
+virDomainObjPtr
+qemuDomainObjListAdd(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags)
+{
+ virDomainObjPtr vm = NULL;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
+ return NULL;
+
+ /* At this point, @vm is locked. If it doesn't have any
+ * definition set, then the call above just added it and
+ * there can't be anybody else using the object. It's safe to
+ * just set the definition without acquiring job. */
+ if (!vm->def) {
+ virDomainObjAssignDef(vm, def, live, oldDef);
+ VIR_RETURN_PTR(vm);
Not sure why use VIR_RETURN_PTR
+ }
+
+ /* Bad luck. Domain was pre-existing and this call is trying
+ * to update its definition. Modify job MUST be acquired. */
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
+ virDomainObjEndAPI(&vm);
+ return NULL;
+ }
+
+ virDomainObjAssignDef(vm, def, live, oldDef);
+
+ qemuDomainObjEndJob(driver, vm);
+
+ return vm;
+}
+
+
static void
qemuDomainObjSaveJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
{
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f92f0dbc27..f469f8eaca 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -543,6 +543,12 @@ void qemuDomainEventFlush(int timer, void *opaque);
void qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
virDomainObjPtr vm);
+virDomainObjPtr qemuDomainObjListAdd(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags);
+
int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainJob job)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8bbac339e0..fa93a686b7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1701,12 +1701,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ 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,
@@ -2405,6 +2402,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int
period,
qemuDomainObjEnterMonitor(driver, vm);
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon, period);
+ sleep(60);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto endjob;
if (r < 0) {
@@ -6971,12 +6969,9 @@ qemuDomainRestoreFlags(virConnectPtr conn,
def = tmp;
}
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (flags & VIR_DOMAIN_SAVE_RUNNING)
@@ -7647,11 +7642,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt, 0)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, &oldDef, false, 0)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
Unfortunately in case of virDomainSaveConfig failure we change definitions again
without a job. This will not cause crash because of memory leak upon restoring
definition and cause only reverting changes of concurrent API call :) Other places of qemu
driver
do not have this issue because they acquire a job right after adding domain
to list.
Ohhh, I see this will be fixed in next patch....
With/without VIR_RETURN_PTR: Reviewed-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
PS
I still wonder why not using single job for all API affairs? It looks cleaner
and have less unnessesary execution paths in multithreading scenario. What's
the difference if job is async?
Nikolay
8:51 a.m.
New subject: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML
On 05.06.2019 12:09, 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 | 55 +++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 6 +++++
src/qemu/qemu_driver.c | 27 ++++++-------------
src/qemu/qemu_migration.c | 5 +---
4 files changed, 70 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4d3a8868b2..f6b677c69e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7055,6 +7055,61 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
};
+/**
+ * qemuDomainObjListAdd:
+ * @driver: qemu driver
+ * @def: domain definition
+ * @oldDef: previous domain definition
+ * @live: whether @def is live definition
+ * @flags: an bitwise-OR of virDomainObjListAdd flags
+ *
+ * Add a domain onto the list of domain object and sets its
+ * definition. If @oldDef is not NULL and there was pre-existing
+ * definition it's returned in @oldDef.
+ *
+ * In addition to that, if definition of an existing domain is
+ * changed a MODIFY job is acquired prior to that.
+ *
+ * Returns: domain object pointer on success,
+ * NULL otherwise.
+ */
+virDomainObjPtr
+qemuDomainObjListAdd(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags)
+{
+ virDomainObjPtr vm = NULL;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
+ return NULL;
+
+ /* At this point, @vm is locked. If it doesn't have any
+ * definition set, then the call above just added it and
+ * there can't be anybody else using the object. It's safe to
+ * just set the definition without acquiring job. */
+ if (!vm->def) {
+ virDomainObjAssignDef(vm, def, live, oldDef);
+ VIR_RETURN_PTR(vm);
+ }
+
+ /* Bad luck. Domain was pre-existing and this call is trying
+ * to update its definition. Modify job MUST be acquired. */
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+ qemuDomainRemoveInactive(driver, vm);
Here we can remove transient domain.
Nikolay
4:09 a.m.
New subject: [libvirt] [PATCH v2 07/11] qemu_domain: Allow qemuDomainObjListAdd to keep job upon return
In some cases, caller of qemuDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust qemuDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 17 ++++++++++++++---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 24 +++++++++++-------------
src/qemu/qemu_migration.c | 2 +-
4 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f6b677c69e..b0b3fa5fd8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7061,6 +7061,7 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
* @def: domain definition
* @oldDef: previous domain definition
* @live: whether @def is live definition
+ * @keepJob: whether to leave MODIFY job set on returned object
* @flags: an bitwise-OR of virDomainObjListAdd flags
*
* Add a domain onto the list of domain object and sets its
@@ -7070,6 +7071,10 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
* In addition to that, if definition of an existing domain is
* changed a MODIFY job is acquired prior to that.
*
+ * If @keepJob is true, then the MODIFY job is not ended upon
+ * successful return from this function. This might be handy if
+ * caller would try to acquire the job anyway.
+ *
* Returns: domain object pointer on success,
* NULL otherwise.
*/
@@ -7078,9 +7083,11 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags)
{
virDomainObjPtr vm = NULL;
+ bool defSet = false;
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
return NULL;
@@ -7091,7 +7098,9 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
* just set the definition without acquiring job. */
if (!vm->def) {
virDomainObjAssignDef(vm, def, live, oldDef);
- VIR_RETURN_PTR(vm);
+ defSet = true;
+ if (!keepJob)
+ VIR_RETURN_PTR(vm);
}
/* Bad luck. Domain was pre-existing and this call is trying
@@ -7102,9 +7111,11 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
return NULL;
}
- virDomainObjAssignDef(vm, def, live, oldDef);
+ if (!defSet)
+ virDomainObjAssignDef(vm, def, live, oldDef);
- qemuDomainObjEndJob(driver, vm);
+ if (!keepJob)
+ qemuDomainObjEndJob(driver, vm);
return vm;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f469f8eaca..52b708babb 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -547,6 +547,7 @@ virDomainObjPtr qemuDomainObjListAdd(virQEMUDriverPtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags);
int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fa93a686b7..34359c9500 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1701,7 +1701,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true, false,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
def = NULL;
@@ -6969,7 +6969,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
def = tmp;
}
- if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true, false,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
def = NULL;
@@ -7642,7 +7642,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = qemuDomainObjListAdd(driver, def, &oldDef, false, 0)))
+ if (!(vm = qemuDomainObjListAdd(driver, def, &oldDef, false, true, 0)))
goto cleanup;
def = NULL;
@@ -7663,9 +7663,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
/* Brand new domain. Remove it */
VIR_INFO("Deleting domain '%s'", vm->def->name);
vm->persistent = 0;
- qemuDomainRemoveInactiveJob(driver, vm);
+ qemuDomainRemoveInactive(driver, vm);
}
- goto cleanup;
+ goto endjob;
}
event = virDomainEventLifecycleNewFromObj(vm,
@@ -7677,6 +7677,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);
@@ -16876,25 +16879,20 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
if (qemuAssignDeviceAliases(def, qemuCaps) < 0)
goto cleanup;
- if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true,
+ if (!(vm = qemuDomainObjListAdd(driver, def, NULL, true, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
def = NULL;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
- qemuDomainRemoveInactive(driver, vm);
- goto cleanup;
- }
-
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
qemuDomainRemoveInactive(driver, vm);
- qemuDomainObjEndJob(driver, vm);
- goto cleanup;
+ goto endjob;
}
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
+ endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 13f0bc7e45..4e1c57c273 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2403,7 +2403,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
QEMU_MIGRATION_COOKIE_CAPS)))
goto cleanup;
- if (!(vm = qemuDomainObjListAdd(driver, *def, NULL, true,
+ if (!(vm = qemuDomainObjListAdd(driver, *def, NULL, true, false,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
*def = NULL;
--
2.21.0
7:37 a.m.
New subject: [libvirt] [PATCH v2 07/11] qemu_domain: Allow qemuDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote:
In some cases, caller of qemuDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust qemuDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 17 ++++++++++++++---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 24 +++++++++++-------------
src/qemu/qemu_migration.c | 2 +-
4 files changed, 27 insertions(+), 17 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
8:39 a.m.
New subject: [libvirt] [PATCH v2 07/11] qemu_domain: Allow qemuDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote:
In some cases, caller of qemuDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust qemuDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 17 ++++++++++++++---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 24 +++++++++++-------------
src/qemu/qemu_migration.c | 2 +-
4 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f6b677c69e..b0b3fa5fd8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7061,6 +7061,7 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
* @def: domain definition
* @oldDef: previous domain definition
* @live: whether @def is live definition
+ * @keepJob: whether to leave MODIFY job set on returned object
* @flags: an bitwise-OR of virDomainObjListAdd flags
*
* Add a domain onto the list of domain object and sets its
@@ -7070,6 +7071,10 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
* In addition to that, if definition of an existing domain is
* changed a MODIFY job is acquired prior to that.
*
+ * If @keepJob is true, then the MODIFY job is not ended upon
+ * successful return from this function. This might be handy if
+ * caller would try to acquire the job anyway.
+ *
* Returns: domain object pointer on success,
* NULL otherwise.
*/
@@ -7078,9 +7083,11 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags)
{
virDomainObjPtr vm = NULL;
+ bool defSet = false;
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
return NULL;
@@ -7091,7 +7098,9 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
* just set the definition without acquiring job. */
if (!vm->def) {
virDomainObjAssignDef(vm, def, live, oldDef);
- VIR_RETURN_PTR(vm);
+ defSet = true;
+ if (!keepJob)
+ VIR_RETURN_PTR(vm);
}
Just realized. If we got in this branch and have @keepJob = true
and later fail to aqcuire job and remove vm entirely then we
free @def which is unexpected by callers.
Nikolay
4:09 a.m.
New subject: [libvirt] [PATCH v2 08/11] 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 | 57 ++++++++++++++++++++++++++++++++++++++++++++
src/lxc/lxc_domain.h | 7 ++++++
src/lxc/lxc_driver.c | 11 +++------
3 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index 51a9fd36eb..800999cbed 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -47,6 +47,63 @@ VIR_ENUM_IMPL(virLXCDomainJob,
VIR_LOG_INIT("lxc.lxc_domain");
+
+/**
+ * lxcDomainObjListAdd:
+ * @driver: LXC driver
+ * @def: domain definition
+ * @oldDef: previous domain definition
+ * @live: whether @def is live definition
+ * @flags: an bitwise-OR of virDomainObjListAdd flags
+ *
+ * Add a domain onto the list of domain object and sets its
+ * definition. If @oldDef is not NULL and there was pre-existing
+ * definition it's returned in @oldDef.
+ *
+ * In addition to that, if definition of an existing domain is
+ * changed a MODIFY job is acquired prior to that.
+ *
+ * Returns: domain object pointer on success,
+ * NULL otherwise.
+ */
+virDomainObjPtr
+lxcDomainObjListAdd(virLXCDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags)
+{
+ virDomainObjPtr vm = NULL;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
+ return NULL;
+
+ /* At this point, @vm is locked. If it doesn't have any
+ * definition set, then the call above just added it and
+ * there can't be anybody else using the object. It's safe to
+ * just set the definition without acquiring job. */
+ if (!vm->def) {
+ virDomainObjAssignDef(vm, def, live, oldDef);
+ VIR_RETURN_PTR(vm);
+ }
+
+ /* Bad luck. Domain was pre-existing and this call is trying
+ * to update its definition. Modify job MUST be acquired. */
+ if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
+ if (!vm->persistent)
+ virDomainObjListRemove(driver->domains, vm);
+ virDomainObjEndAPI(&vm);
+ return NULL;
+ }
+
+ virDomainObjAssignDef(vm, def, live, oldDef);
+
+ virLXCDomainObjEndJob(driver, vm);
+
+ return vm;
+}
+
+
static int
virLXCDomainObjInitJob(virLXCDomainObjPrivatePtr priv)
{
diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
index 2048481829..522cf7917d 100644
--- a/src/lxc/lxc_domain.h
+++ b/src/lxc/lxc_domain.h
@@ -96,6 +96,13 @@ extern virDomainXMLNamespace virLXCDriverDomainXMLNamespace;
extern virDomainXMLPrivateDataCallbacks virLXCDriverPrivateDataCallbacks;
extern virDomainDefParserConfig virLXCDriverDomainDefParserConfig;
+virDomainObjPtr
+lxcDomainObjListAdd(virLXCDriverPtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags);
+
int
virLXCDomainObjBeginJob(virLXCDriverPtr driver,
virDomainObjPtr obj,
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index eaf26563f5..41e6b59927 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -444,12 +444,10 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags)
goto cleanup;
}
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt, 0)))
+ if (!(vm = lxcDomainObjListAdd(driver, def, &oldDef, false, 0)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
+
vm->persistent = 1;
if (virDomainSaveConfig(cfg->configDir, driver->caps,
@@ -1191,12 +1189,9 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
}
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
+ if (!(vm = lxcDomainObjListAdd(driver, def, NULL, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) {
--
2.21.0
8:09 a.m.
New subject: [libvirt] [PATCH v2 08/11] lxc: Grab modify job for changing domain XML
On 05.06.2019 12:09, Michal Privoznik wrote:
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 | 57 ++++++++++++++++++++++++++++++++++++++++++++
src/lxc/lxc_domain.h | 7 ++++++
src/lxc/lxc_driver.c | 11 +++------
3 files changed, 67 insertions(+), 8 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:09 a.m.
New subject: [libvirt] [PATCH v2 09/11] lxc_domain: Allow lxcDomainObjListAdd to keep job upon return
In some cases, caller of lxcDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust lxcDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/lxc/lxc_domain.c | 17 ++++++++++++++---
src/lxc/lxc_domain.h | 1 +
src/lxc/lxc_driver.c | 19 ++++++++-----------
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index 800999cbed..2249d38757 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -54,6 +54,7 @@ VIR_LOG_INIT("lxc.lxc_domain");
* @def: domain definition
* @oldDef: previous domain definition
* @live: whether @def is live definition
+ * @keepJob: whether to leave MODIFY job set on returned object*
* @flags: an bitwise-OR of virDomainObjListAdd flags
*
* Add a domain onto the list of domain object and sets its
@@ -63,6 +64,10 @@ VIR_LOG_INIT("lxc.lxc_domain");
* In addition to that, if definition of an existing domain is
* changed a MODIFY job is acquired prior to that.
*
+ * If @keepJob is true, then the MODIFY job is not ended upon
+ * successful return from this function. This might be handy if
+ * caller would try to acquire the job anyway.
+ *
* Returns: domain object pointer on success,
* NULL otherwise.
*/
@@ -71,9 +76,11 @@ lxcDomainObjListAdd(virLXCDriverPtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags)
{
virDomainObjPtr vm = NULL;
+ bool defSet = false;
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
return NULL;
@@ -84,7 +91,9 @@ lxcDomainObjListAdd(virLXCDriverPtr driver,
* just set the definition without acquiring job. */
if (!vm->def) {
virDomainObjAssignDef(vm, def, live, oldDef);
- VIR_RETURN_PTR(vm);
+ defSet = true;
+ if (!keepJob)
+ VIR_RETURN_PTR(vm);
}
/* Bad luck. Domain was pre-existing and this call is trying
@@ -96,9 +105,11 @@ lxcDomainObjListAdd(virLXCDriverPtr driver,
return NULL;
}
- virDomainObjAssignDef(vm, def, live, oldDef);
+ if (!defSet)
+ virDomainObjAssignDef(vm, def, live, oldDef);
- virLXCDomainObjEndJob(driver, vm);
+ if (!keepJob)
+ virLXCDomainObjEndJob(driver, vm);
return vm;
}
diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
index 522cf7917d..539914e0a8 100644
--- a/src/lxc/lxc_domain.h
+++ b/src/lxc/lxc_domain.h
@@ -101,6 +101,7 @@ lxcDomainObjListAdd(virLXCDriverPtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags);
int
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 41e6b59927..e77df15772 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -444,7 +444,7 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
goto cleanup;
}
- if (!(vm = lxcDomainObjListAdd(driver, def, &oldDef, false, 0)))
+ if (!(vm = lxcDomainObjListAdd(driver, def, &oldDef, false, true, 0)))
goto cleanup;
def = NULL;
@@ -453,7 +453,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,
@@ -464,6 +464,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);
@@ -1189,26 +1192,19 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
}
- if (!(vm = lxcDomainObjListAdd(driver, def, NULL, true,
+ if (!(vm = lxcDomainObjListAdd(driver, def, NULL, true, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
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),
VIR_DOMAIN_RUNNING_BOOTED) < 0) {
virDomainAuditStart(vm, "booted", false);
- virLXCDomainObjEndJob(driver, vm);
if (!vm->persistent)
virDomainObjListRemove(driver->domains, vm);
- goto cleanup;
+ goto endjob;
}
event = virDomainEventLifecycleNewFromObj(vm,
@@ -1218,6 +1214,7 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
+ endjob:
virLXCDomainObjEndJob(driver, vm);
cleanup:
--
2.21.0
8:14 a.m.
New subject: [libvirt] [PATCH v2 09/11] lxc_domain: Allow lxcDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote:
In some cases, caller of lxcDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust lxcDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/lxc/lxc_domain.c | 17 ++++++++++++++---
src/lxc/lxc_domain.h | 1 +
src/lxc/lxc_driver.c | 19 ++++++++-----------
3 files changed, 23 insertions(+), 14 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:09 a.m.
New subject: [libvirt] [PATCH v2 10/11] 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 | 57 +++++++++++++++++++++++++++++++++++++
src/libxl/libxl_domain.h | 7 +++++
src/libxl/libxl_driver.c | 24 ++++------------
src/libxl/libxl_migration.c | 14 +++------
4 files changed, 73 insertions(+), 29 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 2d8569e592..f2e1af52e5 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -65,6 +65,63 @@ libxlDomainObjPrivateOnceInit(void)
VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate);
+
+/**
+ * libxlDomainObjListAdd:
+ * @driver: libxl driver
+ * @def: domain definition
+ * @oldDef: previous domain definition
+ * @live: whether @def is live definition
+ * @flags: an bitwise-OR of virDomainObjListAdd flags
+ *
+ * Add a domain onto the list of domain object and sets its
+ * definition. If @oldDef is not NULL and there was pre-existing
+ * definition it's returned in @oldDef.
+ *
+ * In addition to that, if definition of an existing domain is
+ * changed a MODIFY job is acquired prior to that.
+ *
+ * Returns: domain object pointer on success,
+ * NULL otherwise.
+ */
+virDomainObjPtr
+libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags)
+{
+ virDomainObjPtr vm = NULL;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
+ return NULL;
+
+ /* At this point, @vm is locked. If it doesn't have any
+ * definition set, then the call above just added it and
+ * there can't be anybody else using the object. It's safe to
+ * just set the definition without acquiring job. */
+ if (!vm->def) {
+ virDomainObjAssignDef(vm, def, live, oldDef);
+ VIR_RETURN_PTR(vm);
+ }
+
+ /* Bad luck. Domain was pre-existing and this call is trying
+ * to update its definition. Modify job MUST be acquired. */
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ if (!vm->persistent)
+ virDomainObjListRemove(driver->domains, vm);
+ virDomainObjEndAPI(&vm);
+ return NULL;
+ }
+
+ virDomainObjAssignDef(vm, def, live, oldDef);
+
+ libxlDomainObjEndJob(driver, vm);
+
+ return vm;
+}
+
+
static int
libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
{
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 769ee8ec4d..b5d5127e91 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -83,6 +83,13 @@ extern const struct libxl_event_hooks ev_hooks;
int
libxlDomainObjPrivateInitCtx(virDomainObjPtr vm);
+virDomainObjPtr
+libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags);
+
int
libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
virDomainObjPtr obj,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 809d298ac1..9c9a30bb90 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -601,12 +601,8 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
if (virUUIDParse("00000000-0000-0000-0000-000000000000", def->uuid) <
0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- 0)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 0)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -1030,12 +1026,9 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -1950,12 +1943,9 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
if (virDomainRestoreFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
+ if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -2850,12 +2840,8 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- 0)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 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 35403380d0..d0b0142900 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -566,12 +566,9 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
&mig, &xmlout, &taint_hook) < 0)
goto error;
- if (!(vm = virDomainObjListAdd(driver->domains, *def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = libxlDomainObjListAdd(driver, *def, NULL, true,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
-
- virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
/*
@@ -675,12 +672,9 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
&mig, &xmlout, &taint_hook) < 0)
goto error;
- if (!(vm = virDomainObjListAdd(driver->domains, *def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = libxlDomainObjListAdd(driver, *def, NULL, true,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
-
- virDomainObjAssignDef(vm, *def, true, NULL);
*def = NULL;
/*
--
2.21.0
8:19 a.m.
New subject: [libvirt] [PATCH v2 10/11] libxl: Grab modify job for changing domain XML
On 05.06.2019 12:09, Michal Privoznik wrote:
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 | 57 +++++++++++++++++++++++++++++++++++++
src/libxl/libxl_domain.h | 7 +++++
src/libxl/libxl_driver.c | 24 ++++------------
src/libxl/libxl_migration.c | 14 +++------
4 files changed, 73 insertions(+), 29 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 2d8569e592..f2e1af52e5 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -65,6 +65,63 @@ libxlDomainObjPrivateOnceInit(void)
VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate);
+
+/**
+ * libxlDomainObjListAdd:
+ * @driver: libxl driver
+ * @def: domain definition
+ * @oldDef: previous domain definition
+ * @live: whether @def is live definition
+ * @flags: an bitwise-OR of virDomainObjListAdd flags
+ *
+ * Add a domain onto the list of domain object and sets its
+ * definition. If @oldDef is not NULL and there was pre-existing
+ * definition it's returned in @oldDef.
+ *
+ * In addition to that, if definition of an existing domain is
+ * changed a MODIFY job is acquired prior to that.
+ *
+ * Returns: domain object pointer on success,
+ * NULL otherwise.
+ */
+virDomainObjPtr
+libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags)
+{
+ virDomainObjPtr vm = NULL;
+
+ if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
+ return NULL;
+
+ /* At this point, @vm is locked. If it doesn't have any
+ * definition set, then the call above just added it and
+ * there can't be anybody else using the object. It's safe to
+ * just set the definition without acquiring job. */
+ if (!vm->def) {
+ virDomainObjAssignDef(vm, def, live, oldDef);
+ VIR_RETURN_PTR(vm);
+ }
+
+ /* Bad luck. Domain was pre-existing and this call is trying
+ * to update its definition. Modify job MUST be acquired. */
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ if (!vm->persistent)
+ virDomainObjListRemove(driver->domains, vm);
+ virDomainObjEndAPI(&vm);
+ return NULL;
+ }
+
+ virDomainObjAssignDef(vm, def, live, oldDef);
+
+ libxlDomainObjEndJob(driver, vm);
+
+ return vm;
+}
+
+
static int
libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
{
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 769ee8ec4d..b5d5127e91 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -83,6 +83,13 @@ extern const struct libxl_event_hooks ev_hooks;
int
libxlDomainObjPrivateInitCtx(virDomainObjPtr vm);
+virDomainObjPtr
+libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
+ virDomainDefPtr def,
+ virDomainDefPtr *oldDef,
+ bool live,
+ unsigned int flags);
+
int
libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
virDomainObjPtr obj,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 809d298ac1..9c9a30bb90 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -601,12 +601,8 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
if (virUUIDParse("00000000-0000-0000-0000-000000000000", def->uuid)
< 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- 0)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 0)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, false, &oldDef);
def = NULL;
vm->persistent = 1;
@@ -1030,12 +1026,9 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
- VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
+ VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
-
- virDomainObjAssignDef(vm, def, true, NULL);
def = NULL;
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
@@ -1950,12 +1943,9 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
if (virDomainRestoreFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = virDomainObjListAdd(driver->domains, def,
- driver->xmlopt,
+ if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
just indentation, so
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
4:09 a.m.
New subject: [libvirt] [PATCH v2 11/11] libxl_domain: Allow libxlDomainObjListAdd to keep job upon return
In some cases, caller of libxlDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust libxlDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libxl/libxl_domain.c | 17 ++++++++++++++---
src/libxl/libxl_domain.h | 1 +
src/libxl/libxl_driver.c | 24 +++++++-----------------
src/libxl/libxl_migration.c | 18 ++----------------
4 files changed, 24 insertions(+), 36 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index f2e1af52e5..8e45ac512a 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -72,6 +72,7 @@ VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate);
* @def: domain definition
* @oldDef: previous domain definition
* @live: whether @def is live definition
+ * @keepJob: whether to leave MODIFY job set on returned object
* @flags: an bitwise-OR of virDomainObjListAdd flags
*
* Add a domain onto the list of domain object and sets its
@@ -81,6 +82,10 @@ VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate);
* In addition to that, if definition of an existing domain is
* changed a MODIFY job is acquired prior to that.
*
+ * If @keepJob is true, then the MODIFY job is not ended upon
+ * successful return from this function. This might be handy if
+ * caller would try to acquire the job anyway.
+ *
* Returns: domain object pointer on success,
* NULL otherwise.
*/
@@ -89,9 +94,11 @@ libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags)
{
virDomainObjPtr vm = NULL;
+ bool defSet = false;
if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, flags)))
return NULL;
@@ -102,7 +109,9 @@ libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
* just set the definition without acquiring job. */
if (!vm->def) {
virDomainObjAssignDef(vm, def, live, oldDef);
- VIR_RETURN_PTR(vm);
+ defSet = true;
+ if (!keepJob)
+ VIR_RETURN_PTR(vm);
}
/* Bad luck. Domain was pre-existing and this call is trying
@@ -114,9 +123,11 @@ libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
return NULL;
}
- virDomainObjAssignDef(vm, def, live, oldDef);
+ if (!defSet)
+ virDomainObjAssignDef(vm, def, live, oldDef);
- libxlDomainObjEndJob(driver, vm);
+ if (!keepJob)
+ libxlDomainObjEndJob(driver, vm);
return vm;
}
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index b5d5127e91..b04bbb6c51 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -88,6 +88,7 @@ libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
virDomainDefPtr def,
virDomainDefPtr *oldDef,
bool live,
+ bool keepJob,
unsigned int flags);
int
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 9c9a30bb90..8a9a8baf79 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -601,7 +601,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
if (virUUIDParse("00000000-0000-0000-0000-000000000000", def->uuid) <
0)
goto cleanup;
- if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 0)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, false, 0)))
goto cleanup;
def = NULL;
@@ -1026,17 +1026,11 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (virDomainCreateXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
+ if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
def = NULL;
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
- if (!vm->persistent)
- virDomainObjListRemove(driver->domains, vm);
- goto cleanup;
- }
-
if (libxlDomainStartNew(driver, vm,
(flags & VIR_DOMAIN_START_PAUSED) != 0) < 0) {
if (!vm->persistent)
@@ -1943,17 +1937,11 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
if (virDomainRestoreFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
+ if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto cleanup;
def = NULL;
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
- if (!vm->persistent)
- virDomainObjListRemove(driver->domains, vm);
- goto cleanup;
- }
-
ret = libxlDomainStartRestore(driver, vm,
(flags & VIR_DOMAIN_SAVE_PAUSED) != 0,
fd, hdr.version);
@@ -2840,7 +2828,7 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 0)))
+ if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, true, 0)))
goto cleanup;
def = NULL;
@@ -2850,7 +2838,7 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
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);
@@ -2859,6 +2847,8 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flag
!oldDef ?
VIR_DOMAIN_EVENT_DEFINED_ADDED :
VIR_DOMAIN_EVENT_DEFINED_UPDATED);
+ endjob:
+ libxlDomainObjEndJob(driver, vm);
cleanup:
virDomainDefFree(def);
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index d0b0142900..e498846371 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -566,18 +566,11 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
&mig, &xmlout, &taint_hook) < 0)
goto error;
- if (!(vm = libxlDomainObjListAdd(driver, *def, NULL, true,
+ if (!(vm = libxlDomainObjListAdd(driver, *def, NULL, true, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
*def = NULL;
- /*
- * Unless an error is encountered in this function, the job will
- * be terminated in the finish phase.
- */
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
- goto error;
-
priv = vm->privateData;
if (taint_hook) {
@@ -672,18 +665,11 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
&mig, &xmlout, &taint_hook) < 0)
goto error;
- if (!(vm = libxlDomainObjListAdd(driver, *def, NULL, true,
+ if (!(vm = libxlDomainObjListAdd(driver, *def, NULL, true, true,
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
goto error;
*def = NULL;
- /*
- * Unless an error is encountered in this function, the job will
- * be terminated in the finish phase.
- */
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
- goto error;
-
priv = vm->privateData;
if (taint_hook) {
--
2.21.0
8:26 a.m.
New subject: [libvirt] [PATCH v2 11/11] libxl_domain: Allow libxlDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote:
In some cases, caller of libxlDomainObjListAdd() tries to acquire
MODIFY job after the call. Let's adjust libxlDomainObjListAdd() so
that it will keep the job set upon return (if requested by
caller).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libxl/libxl_domain.c | 17 ++++++++++++++---
src/libxl/libxl_domain.h | 1 +
src/libxl/libxl_driver.c | 24 +++++++-----------------
src/libxl/libxl_migration.c | 18 ++----------------
4 files changed, 24 insertions(+), 36 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
1961
days inactive
1962
days old
22 comments
3 participants
participants (3)
-
Michal Privoznik -
Nikolay Shirokovskiy -
Peter Krempa