[libvirt] [PATCH v2 00/11] Grab modify job for changing domain XML
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
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> Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@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
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> Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@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
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@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
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> Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@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
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@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
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@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@virtuozzo.com>
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 | 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
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@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) {
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@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
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@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@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
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@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
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@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
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@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@virtuozzo.com>
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@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
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 | 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
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@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@virtuozzo.com>
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@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
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@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@virtuozzo.com>
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 | 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
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@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@virtuozzo.com>
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@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
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@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@virtuozzo.com>
participants (3)
-
Michal Privoznik -
Nikolay Shirokovskiy -
Peter Krempa