[PATCH 0/4] virXXXListAdd: Transfer definition ownership
At a lot of places we have the following pattern: virXXXDef *def = parseDef(); if (!(obj = virXXXObjListAdd(def))) goto clenaup; def = NULL; cleanup: virXXXDefFree(def); The 'def = NULL' step is necessary because the ownership of the definition was transferred onto the object. Well, this approach is fragile as it relies on developers remembering to set the variable explicitly. If the virXXXObjListAdd() would take address of @def then the explicit set to NULL can be left out. Please note, I've reworked only a few virXXXObjListAdd() functions to see whether these are desired or not. If merged, I can post patches for the rest. Michal Prívozník (4): virInterfaceObjListAssignDef: Transfer definition ownership virSecretObjListAdd: Transfer definition ownership virStoragePoolObjListAdd: Transfer definition ownership virDomainObjListAdd: Transfer definition ownership src/bhyve/bhyve_driver.c | 6 ++---- src/ch/ch_driver.c | 7 ++----- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/conf/virdomainobjlist.c | 22 +++++++++++++--------- src/conf/virdomainobjlist.h | 2 +- src/conf/virinterfaceobj.c | 25 +++++++++++++++++++------ src/conf/virinterfaceobj.h | 2 +- src/conf/virsecretobj.c | 26 ++++++++++++++------------ src/conf/virsecretobj.h | 2 +- src/conf/virstorageobj.c | 29 ++++++++++++++++------------- src/conf/virstorageobj.h | 2 +- src/libxl/libxl_domain.c | 3 +-- src/libxl/libxl_driver.c | 30 ++++++++++-------------------- src/libxl/libxl_migration.c | 6 ++---- src/lxc/lxc_driver.c | 24 ++++++++---------------- src/openvz/openvz_conf.c | 3 +-- src/openvz/openvz_driver.c | 10 ++++------ src/qemu/qemu_driver.c | 30 ++++++++++-------------------- src/qemu/qemu_migration.c | 3 +-- src/qemu/qemu_snapshot.c | 9 +++------ src/secret/secret_driver.c | 4 ++-- src/storage/storage_driver.c | 5 ++--- src/test/test_driver.c | 31 ++++++++++++------------------- src/vmware/vmware_conf.c | 6 ++---- src/vmware/vmware_driver.c | 10 ++++------ 26 files changed, 137 insertions(+), 170 deletions(-) -- 2.32.0
Upon successful return from virInterfaceObjListAssignDef() the virInterfaceObj is the owner of secret definition. To make this ownership transfer even more visible, lets pass the definition as a double pointer and use g_steal_pointer(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virinterfaceobj.c | 25 +++++++++++++++++++------ src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 6 ++---- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index c5dfa6c7f5..daac74e88c 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -377,9 +377,8 @@ virInterfaceObjListCloneCb(void *payload, goto error; VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(data->dest, backup))) + if (!(obj = virInterfaceObjListAssignDef(data->dest, &backup))) goto error; - backup = NULL; virInterfaceObjEndAPI(&obj); virObjectUnlock(srcObj); @@ -420,25 +419,39 @@ virInterfaceObjListClone(virInterfaceObjList *interfaces) } +/** + * virInterfaceObjListAssignDef: + * @interfaces: virInterface object list + * @def: new definition + * + * Assigns new definition to either an existing or newly created + * virInterface object. Upon successful return the virInterface + * object is the owner of @def and callers should use + * virInterfaceObjGetDef() if they need to access the definition + * as @def is set to NULL. + * + * Returns: a virInterface object instance on success, or + * NULL on error. + */ virInterfaceObj * virInterfaceObjListAssignDef(virInterfaceObjList *interfaces, - virInterfaceDef *def) + virInterfaceDef **def) { virInterfaceObj *obj; virObjectRWLockWrite(interfaces); - if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, (*def)->name))) { virInterfaceDefFree(obj->def); } else { if (!(obj = virInterfaceObjNew())) goto error; - if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0) + if (virHashAddEntry(interfaces->objsName, (*def)->name, obj) < 0) goto error; virObjectRef(obj); } - obj->def = def; + obj->def = g_steal_pointer(def); virObjectRWUnlock(interfaces); return obj; diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index d60faa95f2..5927484167 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -59,7 +59,7 @@ virInterfaceObjListClone(virInterfaceObjList *interfaces); virInterfaceObj * virInterfaceObjListAssignDef(virInterfaceObjList *interfaces, - virInterfaceDef *def); + virInterfaceDef **def); void virInterfaceObjListRemove(virInterfaceObjList *interfaces, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e45748c5fc..92c0462170 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1141,9 +1141,8 @@ testParseInterfaces(testDriver *privconn, if (!def) return -1; - if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, &def))) return -1; - def = NULL; virInterfaceObjSetActive(obj, true); virInterfaceObjEndAPI(&obj); @@ -6203,9 +6202,8 @@ testInterfaceDefineXML(virConnectPtr conn, if ((def = virInterfaceDefParseString(xmlStr, flags)) == NULL) goto cleanup; - if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, &def)) == NULL) goto cleanup; - def = NULL; objdef = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, objdef->name, objdef->mac); -- 2.32.0
Upon successful return from virSecretObjListAdd() the virSecretObj is the owner of secret definition. To make this ownership transfer even more visible, lets pass the definition as a double pointer and use g_steal_pointer(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virsecretobj.c | 26 ++++++++++++++------------ src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 4 ++-- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 212cfe103c..3fbee6f6ec 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -313,13 +313,16 @@ virSecretObjListRemove(virSecretObjList *secrets, * @configDir: directory to place secret config files * @oldDef: Former secret def (e.g. a reload path perhaps) * - * Add the new @newdef to the secret obj table hash + * Add the new @newdef to the secret obj table hash. Upon + * successful the virSecret object is the owner of @newdef and + * callers should use virSecretObjGetDef() if they need to access + * the definition as @newdef is set to NULL. * * Returns: locked and ref'd secret or NULL if failure to add */ virSecretObj * virSecretObjListAdd(virSecretObjList *secrets, - virSecretDef *newdef, + virSecretDef **newdef, const char *configDir, virSecretDef **oldDef) { @@ -333,14 +336,14 @@ virSecretObjListAdd(virSecretObjList *secrets, if (oldDef) *oldDef = NULL; - virUUIDFormat(newdef->uuid, uuidstr); + virUUIDFormat((*newdef)->uuid, uuidstr); /* Is there a secret already matching this UUID */ if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { virObjectLock(obj); objdef = obj->def; - if (STRNEQ_NULLABLE(objdef->usage_id, newdef->usage_id)) { + if (STRNEQ_NULLABLE(objdef->usage_id, (*newdef)->usage_id)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), @@ -348,7 +351,7 @@ virSecretObjListAdd(virSecretObjList *secrets, goto cleanup; } - if (objdef->isprivate && !newdef->isprivate) { + if (objdef->isprivate && !(*newdef)->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; @@ -358,20 +361,20 @@ virSecretObjListAdd(virSecretObjList *secrets, *oldDef = objdef; else virSecretDefFree(objdef); - obj->def = newdef; + obj->def = g_steal_pointer(newdef); } else { /* No existing secret with same UUID, * try look for matching usage instead */ if ((obj = virSecretObjListFindByUsageLocked(secrets, - newdef->usage_type, - newdef->usage_id))) { + (*newdef)->usage_type, + (*newdef)->usage_id))) { virObjectLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), - uuidstr, newdef->usage_id); + uuidstr, (*newdef)->usage_id); goto cleanup; } @@ -388,7 +391,7 @@ virSecretObjListAdd(virSecretObjList *secrets, if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) goto cleanup; - obj->def = newdef; + obj->def = g_steal_pointer(newdef); virObjectRef(obj); } @@ -874,9 +877,8 @@ virSecretLoad(virSecretObjList *secrets, if (virSecretLoadValidateUUID(def, file) < 0) goto cleanup; - if (!(obj = virSecretObjListAdd(secrets, def, configDir, NULL))) + if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL))) goto cleanup; - def = NULL; if (virSecretLoadValue(obj) < 0) { virSecretObjListRemove(secrets, obj); diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index e91b9518eb..93b4a46acf 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -50,7 +50,7 @@ virSecretObjListRemove(virSecretObjList *secrets, virSecretObj * virSecretObjListAdd(virSecretObjList *secrets, - virSecretDef *newdef, + virSecretDef **newdef, const char *configDir, virSecretDef **oldDef); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 43aeae9568..de635bba3a 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -228,10 +228,10 @@ secretDefineXML(virConnectPtr conn, if (virSecretDefineXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(obj = virSecretObjListAdd(driver->secrets, def, + if (!(obj = virSecretObjListAdd(driver->secrets, &def, driver->configDir, &backup))) goto cleanup; - objDef = g_steal_pointer(&def); + objDef = virSecretObjGetDef(obj); if (!objDef->isephemeral) { if (backup && backup->isephemeral) { -- 2.32.0
Upon successful return from virStoragePoolObjListAdd() the virStoragePoolObj is the owner of secret definition. To make this ownership transfer even more visible, lets pass the definition as a double pointer and use g_steal_pointer(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 29 ++++++++++++++++------------- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 5 ++--- src/test/test_driver.c | 8 +++----- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index f906c5b141..2c29339b6a 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1513,20 +1513,20 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjList *pools, static void virStoragePoolObjAssignDef(virStoragePoolObj *obj, - virStoragePoolDef *def, + virStoragePoolDef **def, unsigned int flags) { if (virStoragePoolObjIsActive(obj) || virStoragePoolObjIsStarting(obj)) { virStoragePoolDefFree(obj->newDef); - obj->newDef = def; + obj->newDef = g_steal_pointer(def); } else { if (!obj->newDef && flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE) obj->newDef = g_steal_pointer(&obj->def); virStoragePoolDefFree(obj->def); - obj->def = def; + obj->def = g_steal_pointer(def); } } @@ -1548,11 +1548,16 @@ virStoragePoolObjAssignDef(virStoragePoolObj *obj, * If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags * then this will fail if the pool exists and is active. * + * Upon successful return the virStoragePool object is the owner + * of @def and callers should use virStoragePoolObjGetDef() or + * virStoragePoolObjGetNewDef() if they need to access the + * definition as @def is set to NULL. + * * Returns locked and reffed object pointer or NULL on error */ virStoragePoolObj * virStoragePoolObjListAdd(virStoragePoolObjList *pools, - virStoragePoolDef *def, + virStoragePoolDef **def, unsigned int flags) { virStoragePoolObj *obj = NULL; @@ -1561,10 +1566,10 @@ virStoragePoolObjListAdd(virStoragePoolObjList *pools, virObjectRWLockWrite(pools); - if (virStoragePoolObjSourceFindDuplicate(pools, def) < 0) + if (virStoragePoolObjSourceFindDuplicate(pools, *def) < 0) goto error; - rc = virStoragePoolObjIsDuplicate(pools, def, + rc = virStoragePoolObjIsDuplicate(pools, *def, !!(flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE), &obj); @@ -1579,17 +1584,17 @@ virStoragePoolObjListAdd(virStoragePoolObjList *pools, if (!(obj = virStoragePoolObjNew())) goto error; - virUUIDFormat(def->uuid, uuidstr); + virUUIDFormat((*def)->uuid, uuidstr); if (virHashAddEntry(pools->objs, uuidstr, obj) < 0) goto error; virObjectRef(obj); - if (virHashAddEntry(pools->objsName, def->name, obj) < 0) { + if (virHashAddEntry(pools->objsName, (*def)->name, obj) < 0) { virHashRemoveEntry(pools->objs, uuidstr); goto error; } virObjectRef(obj); - obj->def = def; + obj->def = g_steal_pointer(def); virObjectRWUnlock(pools); return obj; @@ -1620,9 +1625,8 @@ virStoragePoolObjLoad(virStoragePoolObjList *pools, return NULL; } - if (!(obj = virStoragePoolObjListAdd(pools, def, 0))) + if (!(obj = virStoragePoolObjListAdd(pools, &def, 0))) return NULL; - def = NULL; VIR_FREE(obj->configFile); /* for driver reload */ obj->configFile = g_strdup(path); @@ -1673,10 +1677,9 @@ virStoragePoolObjLoadState(virStoragePoolObjList *pools, } /* create the object */ - if (!(obj = virStoragePoolObjListAdd(pools, def, + if (!(obj = virStoragePoolObjListAdd(pools, &def, VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) return NULL; - def = NULL; /* XXX: future handling of some additional useful status data, * for now, if a status file for a pool exists, the pool will be marked diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 65f2ae8175..523bdec244 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -203,7 +203,7 @@ typedef enum { virStoragePoolObj * virStoragePoolObjListAdd(virStoragePoolObjList *pools, - virStoragePoolDef *def, + virStoragePoolDef **def, unsigned int flags); int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 51daf6a05d..4df2c75a2b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -746,11 +746,10 @@ storagePoolCreateXML(virConnectPtr conn, if ((backend = virStorageBackendForType(newDef->type)) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, + if (!(obj = virStoragePoolObjListAdd(driver->pools, &newDef, VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE | VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; - newDef = NULL; def = virStoragePoolObjGetDef(obj); virStoragePoolObjSetStarting(obj, true); @@ -830,7 +829,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStorageBackendForType(newDef->type) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, 0))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, &newDef, 0))) goto cleanup; newDef = virStoragePoolObjGetNewDef(obj); def = virStoragePoolObjGetDef(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 92c0462170..369edacf29 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1226,7 +1226,7 @@ testParseStorage(testDriver *privconn, if (!def) return -1; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, 0))) { + if (!(obj = virStoragePoolObjListAdd(privconn->pools, &def, 0))) { virStoragePoolDefFree(def); return -1; } @@ -6675,10 +6675,9 @@ testStoragePoolCreateXML(virConnectPtr conn, if (!(newDef = virStoragePoolDefParseString(xml, 0))) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, + if (!(obj = virStoragePoolObjListAdd(privconn->pools, &newDef, VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; - newDef = NULL; def = virStoragePoolObjGetDef(obj); if (def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { @@ -6742,9 +6741,8 @@ testStoragePoolDefineXML(virConnectPtr conn, newDef->allocation = defaultPoolAlloc; newDef->available = defaultPoolCap - defaultPoolAlloc; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, 0))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, &newDef, 0))) goto cleanup; - newDef = NULL; def = virStoragePoolObjGetDef(obj); event = virStoragePoolEventLifecycleNew(def->name, def->uuid, -- 2.32.0
Upon successful return from virDomainObjListAdd() the virDomainObj is the owner of secret definition. To make this ownership transfer even more visible, lets pass the definition as a double pointer and use g_steal_pointer(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_driver.c | 6 ++---- src/ch/ch_driver.c | 7 ++----- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/conf/virdomainobjlist.c | 22 +++++++++++++--------- src/conf/virdomainobjlist.h | 2 +- src/libxl/libxl_domain.c | 3 +-- src/libxl/libxl_driver.c | 30 ++++++++++-------------------- src/libxl/libxl_migration.c | 6 ++---- src/lxc/lxc_driver.c | 24 ++++++++---------------- src/openvz/openvz_conf.c | 3 +-- src/openvz/openvz_driver.c | 10 ++++------ src/qemu/qemu_driver.c | 30 ++++++++++-------------------- src/qemu/qemu_migration.c | 3 +-- src/qemu/qemu_snapshot.c | 9 +++------ src/test/test_driver.c | 17 +++++++---------- src/vmware/vmware_conf.c | 6 ++---- src/vmware/vmware_driver.c | 10 ++++------ 18 files changed, 76 insertions(+), 122 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 516490f6cd..eccf9b44a8 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -533,11 +533,10 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (bhyveDomainAssignAddresses(def, NULL) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(privconn->domains, def, + if (!(vm = virDomainObjListAdd(privconn->domains, &def, privconn->xmlopt, 0, &oldDef))) goto cleanup; - def = NULL; vm->persistent = 1; if (virDomainDefSave(vm->newDef ? vm->newDef : vm->def, @@ -915,12 +914,11 @@ bhyveDomainCreateXML(virConnectPtr conn, if (bhyveDomainAssignAddresses(def, NULL) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(privconn->domains, def, + if (!(vm = virDomainObjListAdd(privconn->domains, &def, privconn->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (virBhyveProcessStart(conn, vm, VIR_DOMAIN_RUNNING_BOOTED, diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 9eaf3ee499..464bcef907 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -237,15 +237,13 @@ chDomainCreateXML(virConnectPtr conn, goto cleanup; if (!(vm = virDomainObjListAdd(driver->domains, - vmdef, + &vmdef, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - vmdef = NULL; - if (virCHDomainObjBeginJob(vm, CH_JOB_MODIFY) < 0) goto cleanup; @@ -323,12 +321,11 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virDomainDefineXMLFlagsEnsureACL(conn, vmdef) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, vmdef, + if (!(vm = virDomainObjListAdd(driver->domains, &vmdef, driver->xmlopt, 0, NULL))) goto cleanup; - vmdef = NULL; vm->persistent = 1; dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 552d43b845..9b04071259 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3865,7 +3865,7 @@ virDomainDefNew(virDomainXMLOption *xmlopt) void virDomainObjAssignDef(virDomainObj *domain, - virDomainDef *def, + virDomainDef **def, bool live, virDomainDef **oldDef) { @@ -3876,7 +3876,7 @@ void virDomainObjAssignDef(virDomainObj *domain, *oldDef = domain->newDef; else virDomainDefFree(domain->newDef); - domain->newDef = def; + domain->newDef = g_steal_pointer(def); } else { if (live) { /* save current configuration to be restored on domain shutdown */ @@ -3884,13 +3884,13 @@ void virDomainObjAssignDef(virDomainObj *domain, domain->newDef = domain->def; else virDomainDefFree(domain->def); - domain->def = def; + domain->def = g_steal_pointer(def); } else { if (oldDef) *oldDef = domain->def; else virDomainDefFree(domain->def); - domain->def = def; + domain->def = g_steal_pointer(def); } } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8634960313..c4a8dcc2ea 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3388,7 +3388,7 @@ virDomainNetDefNew(virDomainXMLOption *xmlopt); virDomainDef *virDomainDefNew(virDomainXMLOption *xmlopt); void virDomainObjAssignDef(virDomainObj *domain, - virDomainDef *def, + virDomainDef **def, bool live, virDomainDef **oldDef); int virDomainObjSetDefTransient(virDomainXMLOption *xmlopt, diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index ff6c4c1b4f..063c63cd30 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -257,13 +257,17 @@ virDomainObjListAddObjLocked(virDomainObjList *doms, * the @def being added is assumed to represent a * live config, not a future inactive config * + * Upon successful return the virDomain object is the owner of + * @def and callers should use @vm->def if they need to access + * the definition as @def is set to NULL. + * * The returned @vm from this function will be locked and ref * counted. The caller is expected to use virDomainObjEndAPI * when it completes usage. */ static virDomainObj * virDomainObjListAddLocked(virDomainObjList *doms, - virDomainDef *def, + virDomainDef **def, virDomainXMLOption *xmlopt, unsigned int flags, virDomainDef **oldDef) @@ -275,13 +279,13 @@ virDomainObjListAddLocked(virDomainObjList *doms, *oldDef = NULL; /* See if a VM with matching UUID already exists */ - if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) { + if ((vm = virDomainObjListFindByUUIDLocked(doms, (*def)->uuid))) { if (vm->removing) { virReportError(VIR_ERR_OPERATION_FAILED, _("domain '%s' is already being removed"), vm->def->name); goto error; - } else if (STRNEQ(vm->def->name, def->name)) { + } else if (STRNEQ(vm->def->name, (*def)->name)) { /* UUID matches, but if names don't match, refuse it */ virUUIDFormat(vm->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, @@ -312,20 +316,20 @@ virDomainObjListAddLocked(virDomainObjList *doms, oldDef); } else { /* UUID does not match, but if a name matches, refuse it */ - if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) { + if ((vm = virDomainObjListFindByNameLocked(doms, (*def)->name))) { virUUIDFormat(vm->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("domain '%s' already exists with uuid %s"), - def->name, uuidstr); + (*def)->name, uuidstr); goto error; } if (!(vm = virDomainObjNew(xmlopt))) goto error; - vm->def = def; + vm->def = g_steal_pointer(def); if (virDomainObjListAddObjLocked(doms, vm) < 0) { - vm->def = NULL; + *def = g_steal_pointer(&vm->def); goto error; } } @@ -340,7 +344,7 @@ virDomainObjListAddLocked(virDomainObjList *doms, virDomainObj * virDomainObjListAdd(virDomainObjList *doms, - virDomainDef *def, + virDomainDef **def, virDomainXMLOption *xmlopt, unsigned int flags, virDomainDef **oldDef) @@ -498,7 +502,7 @@ virDomainObjListLoadConfig(virDomainObjList *doms, if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0) goto error; - if (!(dom = virDomainObjListAddLocked(doms, def, xmlopt, 0, &oldDef))) + if (!(dom = virDomainObjListAddLocked(doms, &def, xmlopt, 0, &oldDef))) goto error; dom->autostart = autostart; diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index 7b4e34b951..4169eb4f78 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -40,7 +40,7 @@ enum { VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), }; virDomainObj *virDomainObjListAdd(virDomainObjList *doms, - virDomainDef *def, + virDomainDef **def, virDomainXMLOption *xmlopt, unsigned int flags, virDomainDef **oldDef); diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 4f61584ceb..d9dca370c6 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1482,8 +1482,7 @@ libxlDomainStartNew(libxlDriverPrivate *driver, goto cleanup; } - virDomainObjAssignDef(vm, def, true, NULL); - def = NULL; + virDomainObjAssignDef(vm, &def, true, NULL); if (unlink(managed_save_path) < 0) VIR_WARN("Failed to remove the managed state %s", diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 39da10983e..23a28dc124 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -595,13 +595,12 @@ libxlAddDom0(libxlDriverPrivate *driver) if (virUUIDParse("00000000-0000-0000-0000-000000000000", def->uuid) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, NULL))) goto cleanup; - def = NULL; vm->persistent = 1; virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); } @@ -1028,13 +1027,12 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (virDomainCreateXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (!vm->persistent) @@ -1948,13 +1946,12 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, if (virDomainRestoreFlagsEnsureACL(conn, def) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { if (!vm->persistent) @@ -2826,12 +2823,11 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, &oldDef))) goto cleanup; - def = NULL; vm->persistent = 1; @@ -4143,10 +4139,8 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { ret = virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (!ret) + virDomainObjAssignDef(vm, &vmdef, false, NULL); } endjob: @@ -4234,10 +4228,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { ret = virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (!ret) + virDomainObjAssignDef(vm, &vmdef, false, NULL); } endjob: @@ -4319,10 +4311,8 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, /* Finally, if no error until here, we can save config. */ if (!ret && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { ret = virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (!ret) + virDomainObjAssignDef(vm, &vmdef, false, NULL); } cleanup: diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index be5cc7e049..cdd714d6b8 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -548,13 +548,12 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, &mig, &xmlout, &taint_hook) < 0) goto error; - if (!(vm = virDomainObjListAdd(driver->domains, *def, + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - *def = NULL; /* * Unless an error is encountered in this function, the job will @@ -658,13 +657,12 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, &mig, &xmlout, &taint_hook) < 0) goto error; - if (!(vm = virDomainObjListAdd(driver->domains, *def, + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - *def = NULL; /* * Unless an error is encountered in this function, the job will diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e2720a6f89..3cdf73c69f 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -435,12 +435,11 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) goto cleanup; } - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, &oldDef))) goto cleanup; - def = NULL; vm->persistent = 1; if (virDomainDefSave(vm->newDef ? vm->newDef : vm->def, @@ -1118,13 +1117,12 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, } - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) { if (!vm->persistent) @@ -1917,8 +1915,7 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, if (rc < 0) goto endjob; - virDomainObjAssignDef(vm, persistentDefCopy, false, NULL); - persistentDefCopy = NULL; + virDomainObjAssignDef(vm, &persistentDefCopy, false, NULL); } ret = 0; @@ -4368,10 +4365,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { ret = virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (!ret) + virDomainObjAssignDef(vm, &vmdef, false, NULL); } endjob: @@ -4444,8 +4439,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0) goto endjob; - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; + virDomainObjAssignDef(vm, &vmdef, false, NULL); ret = 0; endjob: @@ -4537,10 +4531,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { ret = virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (!ret) + virDomainObjAssignDef(vm, &vmdef, false, NULL); } endjob: diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 9f879e90eb..e2fbc28abc 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -527,7 +527,7 @@ int openvzLoadDomains(struct openvz_driver *driver) flags |= VIR_DOMAIN_OBJ_LIST_ADD_LIVE; if (!(dom = virDomainObjListAdd(driver->domains, - def, + &def, driver->xmlopt, flags, NULL))) @@ -547,7 +547,6 @@ int openvzLoadDomains(struct openvz_driver *driver) virDomainObjEndAPI(&dom); dom = NULL; - def = NULL; } virCommandFree(cmd); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 681eb734ce..d9c71a5722 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -842,11 +842,10 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla if (virXMLCheckIllegalChars("name", vmdef->name, "\n") < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, vmdef, + if (!(vm = virDomainObjListAdd(driver->domains, &vmdef, driver->xmlopt, 0, NULL))) goto cleanup; - vmdef = NULL; vm->persistent = 1; if (openvzSetInitialConfig(vm->def) < 0) { @@ -930,13 +929,13 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, goto cleanup; if (!(vm = virDomainObjListAdd(driver->domains, - vmdef, + &vmdef, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - 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 */ vm->persistent = 1; @@ -2094,13 +2093,12 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto error; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - def = NULL; if (!uri_in) { if ((my_hostname = virGetHostname()) == NULL) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d954635dde..503b98228a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1752,13 +1752,12 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (virDomainCreateXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { @@ -5972,13 +5971,12 @@ qemuDomainRestoreFlags(virConnectPtr conn, def = tmp; } - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (flags & VIR_DOMAIN_SAVE_RUNNING) data->header.was_running = 1; @@ -6259,8 +6257,7 @@ qemuDomainObjRestore(virConnectPtr conn, goto cleanup; } - virDomainObjAssignDef(vm, def, true, NULL); - def = NULL; + virDomainObjAssignDef(vm, &def, true, NULL); ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path, start_paused, asyncJob); @@ -6629,11 +6626,10 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, def, + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, &oldDef))) goto cleanup; - def = NULL; if (!oldDef && qemuDomainNamePathsCleanup(cfg, vm->def->name, false) < 0) goto cleanup; @@ -7958,8 +7954,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObj *vm, if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0) return -1; - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; + virDomainObjAssignDef(vm, &vmdef, false, NULL); } return 0; @@ -8095,10 +8090,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { ret = virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (!ret) + virDomainObjAssignDef(vm, &vmdef, false, NULL); } endjob: @@ -8182,8 +8175,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver, if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0) goto cleanup; - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; + virDomainObjAssignDef(vm, &vmdef, false, NULL); } ret = 0; @@ -8252,8 +8244,7 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriver *driver, if (vmdef) { if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0) return -1; - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; + virDomainObjAssignDef(vm, &vmdef, false, NULL); } return 0; @@ -9640,8 +9631,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (rc < 0) goto endjob; - virDomainObjAssignDef(vm, persistentDefCopy, false, NULL); - persistentDefCopy = NULL; + virDomainObjAssignDef(vm, &persistentDefCopy, false, NULL); } ret = 0; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9729041846..8001792f5a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2878,13 +2878,12 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, QEMU_MIGRATION_COOKIE_BLOCK_DIRTY_BITMAPS))) goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, *def, + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - *def = NULL; priv = vm->privateData; jobPriv = priv->job.privateData; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 7a1c2097cb..661aeeb8aa 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1984,13 +1984,11 @@ qemuSnapshotRevert(virDomainObj *vm, } if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; + virDomainObjAssignDef(vm, &inactiveConfig, false, NULL); defined = true; } - virDomainObjAssignDef(vm, config, true, NULL); - config = NULL; + virDomainObjAssignDef(vm, &config, true, NULL); /* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. @@ -2065,8 +2063,7 @@ qemuSnapshotRevert(virDomainObj *vm, } if (inactiveConfig) { - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); - inactiveConfig = NULL; + virDomainObjAssignDef(vm, &inactiveConfig, false, NULL); defined = true; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 369edacf29..b6bca884f0 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1043,7 +1043,7 @@ testParseDomains(testDriver *privconn, if (testDomainGenerateIfnames(def) < 0 || !(obj = virDomainObjListAdd(privconn->domains, - def, + &def, privconn->xmlopt, 0, NULL))) { virDomainDefFree(def); @@ -1053,7 +1053,7 @@ testParseDomains(testDriver *privconn, if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) goto error; - nsdata = def->namespaceData; + nsdata = obj->def->namespaceData; obj->persistent = !nsdata->transient; obj->hasManagedSave = nsdata->hasManagedSave; @@ -1768,13 +1768,12 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, if (testDomainGenerateIfnames(def) < 0) goto cleanup; if (!(dom = virDomainObjListAdd(privconn->domains, - def, + &def, privconn->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) { if (!dom->persistent) @@ -2517,13 +2516,12 @@ testDomainRestoreFlags(virConnectPtr conn, if (testDomainGenerateIfnames(def) < 0) goto cleanup; if (!(dom = virDomainObjListAdd(privconn->domains, - def, + &def, privconn->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - def = NULL; if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) { if (!dom->persistent) @@ -4208,12 +4206,11 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn, if (testDomainGenerateIfnames(def) < 0) goto cleanup; if (!(dom = virDomainObjListAdd(privconn->domains, - def, + &def, privconn->xmlopt, 0, &oldDef))) goto cleanup; - def = NULL; dom->persistent = 1; event = virDomainEventLifecycleNewFromObj(dom, @@ -9065,7 +9062,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virObjectEventStateQueue(privconn->eventState, event); } - virDomainObjAssignDef(vm, config, false, NULL); + virDomainObjAssignDef(vm, &config, false, NULL); if (testDomainStartState(privconn, vm, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT) < 0) goto cleanup; @@ -9086,7 +9083,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } else { /* Transitions 1, 4, 7 */ - virDomainObjAssignDef(vm, config, false, NULL); + virDomainObjAssignDef(vm, &config, false, NULL); if (virDomainObjIsActive(vm)) { /* Transitions 4, 7 */ diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index 811507cd55..ab03617d95 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -156,7 +156,7 @@ vmwareLoadDomains(struct vmware_driver *driver) goto cleanup; } - if (!(vm = virDomainObjListAdd(driver->domains, vmdef, + if (!(vm = virDomainObjListAdd(driver->domains, &vmdef, driver->xmlopt, 0, NULL))) goto cleanup; @@ -165,7 +165,7 @@ vmwareLoadDomains(struct vmware_driver *driver) pDomain->vmxPath = g_strdup(vmxPath); - vmwareDomainConfigDisplay(pDomain, vmdef); + vmwareDomainConfigDisplay(pDomain, vm->def); if ((vm->def->id = vmwareExtractPid(vmxPath)) < 0) goto cleanup; @@ -175,8 +175,6 @@ vmwareLoadDomains(struct vmware_driver *driver) vm->persistent = 1; virDomainObjEndAPI(&vm); - - vmdef = NULL; } ret = 0; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 3bd6d4d440..e6843ee745 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -438,7 +438,7 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla /* assign def */ if (!(vm = virDomainObjListAdd(driver->domains, - vmdef, + &vmdef, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) @@ -447,9 +447,8 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla pDomain = vm->privateData; pDomain->vmxPath = g_strdup(vmxPath); - vmwareDomainConfigDisplay(pDomain, vmdef); + vmwareDomainConfigDisplay(pDomain, vm->def); - vmdef = NULL; vm->persistent = 1; dom = virGetDomain(conn, vm->def->name, vm->def->uuid, -1); @@ -689,7 +688,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, /* assign def */ if (!(vm = virDomainObjListAdd(driver->domains, - vmdef, + &vmdef, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, @@ -699,8 +698,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, pDomain = vm->privateData; pDomain->vmxPath = g_strdup(vmxPath); - vmwareDomainConfigDisplay(pDomain, vmdef); - vmdef = NULL; + vmwareDomainConfigDisplay(pDomain, vm->def); if (vmwareStartVM(driver, vm) < 0) { if (!vm->persistent) -- 2.32.0
On Tue, Nov 23, 2021 at 06:09:36PM +0100, Michal Privoznik wrote:
At a lot of places we have the following pattern:
virXXXDef *def = parseDef();
if (!(obj = virXXXObjListAdd(def))) goto clenaup; def = NULL;
cleanup: virXXXDefFree(def);
The 'def = NULL' step is necessary because the ownership of the definition was transferred onto the object. Well, this approach is fragile as it relies on developers remembering to set the variable explicitly.
If the virXXXObjListAdd() would take address of @def then the explicit set to NULL can be left out.
Please note, I've reworked only a few virXXXObjListAdd() functions to see whether these are desired or not. If merged, I can post patches for the rest.
Michal Prívozník (4): virInterfaceObjListAssignDef: Transfer definition ownership virSecretObjListAdd: Transfer definition ownership virStoragePoolObjListAdd: Transfer definition ownership virDomainObjListAdd: Transfer definition ownership
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (2)
-
Martin Kletzander -
Michal Privoznik