[libvirt] [PATCH 0/9] Grab modify job for changing domain XML
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
It's a premature optimization. It's perfectly acceptable for 'error' label to deal with @vm == NULL case. Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com> --- src/conf/virdomainobjlist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
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@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
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@redhat.com> --- src/conf/virdomainobjlist.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
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@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
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@redhat.com> --- src/conf/virdomainobjlist.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@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
On 30.05.2019 12:33, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@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@virtuozzo.com>
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@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
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@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@virtuozzo.com>
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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
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
participants (2)
-
Michal Privoznik -
Nikolay Shirokovskiy