[libvirt] [PATCH v2 0/9] Be consistent with vir*Obj*Remove* APIs
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html NB: This can wait until 4.2.0 is release, but I figured I'd post this now just to put it on the radar and of course in hopes that someone will look during the idle moment or two before the release. Changes since v1: Short story: Rework the processing of the code Longer story: In his review Erik noted that there's a "fire dance" when processing the vir*Obj*Remove APIs of requiring a locked object upon entry, then adding a reference to that object, unlocking the object, locking the list to which it is contained, and then relocking the object. So it took some time to think about it and during one lengthy meeting today I had the aha moment that the *Remove API's could take the same key (e.g., uuid or name) used to Add or Find the object and use it for the Remove API. This allows the Remove API to not require a locked (and reffed) object upon entry and perform the lock dance, remove the object, and return just just a reffed object forcing the caller to know to Unref object. Instead, let's simplify things. The *Remove API can take the key, Find the object in the list, remove it from the hash tables, and dispose of the object. In essence the antecedent to the Add or AssignDef API's taking a def, creating an object, and adding it the object to the hash table(s). If there are two *Remove threads competing, one will win and perform the removal, while the other will call *Remove, but won't find the object in the hash table, and just return none the wiser. And yes, I think this can also work for the Domain code, but it's going to take a few patch series to get there as that code is not consistent between consumers. John Ferlan (9): secret: Rework LoadAllConfigs secret: Alter virSecretObjListRemove processing interface: Alter virInterfaceObjListRemove processing nodedev: Alter virNodeDeviceObjListRemove processing conf: Clean up virStoragePoolObjLoad error processing storage: Clean up storagePoolCreateXML error processing test: Clean up testStoragePoolCreateXML error processing conf: Move virStoragePoolObjRemove closer to AssignDef storagepool: Alter virStoragePoolObjRemove processing src/conf/virinterfaceobj.c | 26 +++++++---- src/conf/virinterfaceobj.h | 2 +- src/conf/virnodedeviceobj.c | 29 +++++++----- src/conf/virnodedeviceobj.h | 2 +- src/conf/virsecretobj.c | 58 +++++++++++------------- src/conf/virsecretobj.h | 2 +- src/conf/virstorageobj.c | 92 +++++++++++++++++++++++--------------- src/conf/virstorageobj.h | 2 +- src/node_device/node_device_hal.c | 16 ++++--- src/node_device/node_device_udev.c | 13 ++++-- src/secret/secret_driver.c | 15 ++++--- src/storage/storage_driver.c | 65 +++++++++++++++------------ src/test/test_driver.c | 78 +++++++++++++++++--------------- 13 files changed, 225 insertions(+), 175 deletions(-) -- 2.13.6
Move the virSecretObjEndAPI into virSecretLoad and alter the processing in order to accomodate that. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 47e0b2896..4aaf47b5d 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -906,12 +906,13 @@ virSecretLoadValue(virSecretObjPtr obj) } -static virSecretObjPtr +static int virSecretLoad(virSecretObjListPtr secrets, const char *file, const char *path, const char *configDir) { + int ret = -1; virSecretDefPtr def = NULL; virSecretObjPtr obj = NULL; @@ -927,13 +928,16 @@ virSecretLoad(virSecretObjListPtr secrets, if (virSecretLoadValue(obj) < 0) { virSecretObjListRemove(secrets, obj); - virObjectUnref(obj); - obj = NULL; + virObjectLock(obj); + goto cleanup; } + ret = 0; + cleanup: virSecretDefFree(def); - return obj; + virSecretObjEndAPI(&obj); + return ret; } @@ -952,7 +956,6 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, * loop (if any). It's better to keep the secrets we managed to find. */ while (virDirRead(dir, &de, NULL) > 0) { char *path; - virSecretObjPtr obj; if (!virFileHasSuffix(de->d_name, ".xml")) continue; @@ -960,15 +963,10 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) continue; - if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) { - VIR_ERROR(_("Error reading secret: %s"), - virGetLastErrorMessage()); - VIR_FREE(path); - continue; - } + if (virSecretLoad(secrets, de->d_name, path, configDir) < 0) + VIR_ERROR(_("Error reading secret: %s"), virGetLastErrorMessage()); VIR_FREE(path); - virSecretObjEndAPI(&obj); } VIR_DIR_CLOSE(dir); -- 2.13.6
Current processing requires a "fire dance" unlocking the @obj, adding an @obj ref, locking the @secrets, and relocking @obj in order to ensure proper lock ordering. This can be avoided by changing virSecretObjListRemove to take a @uuidstr instead of @obj. Then, we can lock the @secrets list, look up the @obj by @uuidstr (like we do when adding), and remove the @obj from the list. This removes the last reference to the object effectively reaping it. NB: Since prior to calling we remove the reference to the object we cannot pass anything contained within the object (such as the obj->def) because it's possible that the object could be reaped by two competing remove threads. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 38 +++++++++++++++++--------------------- src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 15 +++++++++------ 3 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 4aaf47b5d..09f6ead64 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -284,32 +284,25 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, /* * virSecretObjListRemove: * @secrets: list of secret objects - * @secret: a secret object + * @uuidstr: secret uuid to find + * + * Find the object by the @uuidstr in the list, remove the object from + * the list hash table, and free the object. * - * Remove the object from the hash table. The caller must hold the lock - * on the driver owning @secrets and must have also locked @secret to - * ensure no one else is either waiting for @secret or still using it. + * Upon entry it's expected that prior to entry any locks on + * the object related to @uuidstr will have been removed. */ void virSecretObjListRemove(virSecretObjListPtr secrets, - virSecretObjPtr obj) + const char *uuidstr) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virSecretDefPtr def; - - if (!obj) - return; - def = obj->def; - - virUUIDFormat(def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); + virSecretObjPtr obj; virObjectRWLockWrite(secrets); - virObjectLock(obj); - virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(obj); - virObjectUnref(obj); + if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { + virHashRemoveEntry(secrets->objs, uuidstr); + virSecretObjEndAPI(&obj); + } virObjectRWUnlock(secrets); } @@ -927,8 +920,11 @@ virSecretLoad(virSecretObjListPtr secrets, def = NULL; if (virSecretLoadValue(obj) < 0) { - virSecretObjListRemove(secrets, obj); - virObjectLock(obj); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(obj->def->uuid, uuidstr); + virSecretObjEndAPI(&obj); + virSecretObjListRemove(secrets, uuidstr); goto cleanup; } diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index d412ee6a7..68bafc718 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -49,7 +49,7 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, void virSecretObjListRemove(virSecretObjListPtr secrets, - virSecretObjPtr obj); + const char *uuidstr); virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 23a3c9bda..75bc2b0cf 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -270,9 +270,11 @@ secretDefineXML(virConnectPtr conn, virSecretObjSetDef(obj, backup); VIR_STEAL_PTR(def, objDef); } else { - virSecretObjListRemove(driver->secrets, obj); - virObjectUnref(obj); - obj = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(objDef->uuid, uuidstr); + virSecretObjEndAPI(&obj); + virSecretObjListRemove(driver->secrets, uuidstr); } cleanup: @@ -392,6 +394,7 @@ secretUndefine(virSecretPtr secret) int ret = -1; virSecretObjPtr obj; virSecretDefPtr def; + char uuidstr[VIR_UUID_STRING_BUFLEN]; virObjectEventPtr event = NULL; if (!(obj = secretObjFromSecret(secret))) @@ -412,9 +415,9 @@ secretUndefine(virSecretPtr secret) virSecretObjDeleteData(obj); - virSecretObjListRemove(driver->secrets, obj); - virObjectUnref(obj); - obj = NULL; + virUUIDFormat(def->uuid, uuidstr); + virSecretObjEndAPI(&obj); + virSecretObjListRemove(driver->secrets, uuidstr); ret = 0; -- 2.13.6
Current processing requires a "fire dance" unlocking the @obj, adding an @obj ref, locking the @interfaces, and relocking @obj in order to ensure proper lock ordering. This can be avoided by changing virInterfaceObjListRemove to take @name instead of @obj. Then, we can lock the @interfaces list, look up the @obj by @name (like we do when adding), and remove the @obj from the list. This removes the last reference to the object effectively reaping it. NB: Since prior to calling we remove the reference to the object we cannot pass anything contained within the object (such as the obj->def or obj->def->name) because it's possible that the object could be reaped by two competing remove threads. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 26 +++++++++++++++++--------- src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 4 ++-- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index f90c0bd9c..c3a7d4cd8 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -358,20 +358,28 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, } +/* + * virInterfaceObjListRemove: + * @interfaces: list of interface objects + * @name: name of interface definition to remove + * + * Find the object by name in the list, remove the object from the + * list hash table, and free the object. + * + * Upon entry it's expected that prior to entry any locks on + * the object related to @name will have been removed. + */ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr obj) + const char *name) { - if (!obj) - return; + virInterfaceObjPtr obj; - virObjectRef(obj); - virObjectUnlock(obj); virObjectRWLockWrite(interfaces); - virObjectLock(obj); - virHashRemoveEntry(interfaces->objsName, obj->def->name); - virObjectUnlock(obj); - virObjectUnref(obj); + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, name))) { + virHashRemoveEntry(interfaces->objsName, name); + virInterfaceObjEndAPI(&obj); + } virObjectRWUnlock(interfaces); } diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 799d38038..82eb2ee87 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -66,7 +66,7 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr obj); + const char *name); typedef bool (*virInterfaceObjListFilter)(virConnectPtr conn, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 99c27cc0a..ddddd8dcb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4092,8 +4092,8 @@ testInterfaceUndefine(virInterfacePtr iface) if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - virInterfaceObjListRemove(privconn->ifaces, obj); - virObjectUnref(obj); + virInterfaceObjEndAPI(&obj); + virInterfaceObjListRemove(privconn->ifaces, iface->name); return 0; } -- 2.13.6
Current processing requires a "fire dance" unlocking the @obj, adding an @obj ref, locking the @devs, and relocking @obj in order to ensure proper proper lock ordering. This can be avoided by changing virNodeDeviceObjListRemove to take @name instead of @obj. Then, we can lock the @devs list, look up the @obj by @name (like we do when adding), and remove the @obj from the list. This removes the last reference to the object effectively reaping it. NB: Since prior to calling we remove the reference to the object we cannot pass anything contained within the object (such as the obj->def or obj->def->name) because it's possible that the object could be reaped by two competing remove threads. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 29 +++++++++++++++++------------ src/conf/virnodedeviceobj.h | 2 +- src/node_device/node_device_hal.c | 16 +++++++++------- src/node_device/node_device_udev.c | 13 +++++++++---- src/test/test_driver.c | 28 ++++++++++++++-------------- 5 files changed, 50 insertions(+), 38 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee4..9064a6cfb 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -470,23 +470,28 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, } +/* + * virNodeDeviceObjListRemove: + * @devs: list of node device objects + * @name: a node device definition + * + * Find the object by name in the list, remove the object from the + * list hash table, and free the object. + * + * Upon entry it's expected that prior to entry any locks on + * the object related to @name will have been removed. + */ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr obj) + const char *name) { - virNodeDeviceDefPtr def; - - if (!obj) - return; - def = obj->def; + virNodeDeviceObjPtr obj; - virObjectRef(obj); - virObjectUnlock(obj); virObjectRWLockWrite(devs); - virObjectLock(obj); - virHashRemoveEntry(devs->objs, def->name); - virObjectUnlock(obj); - virObjectUnref(obj); + if ((obj = virNodeDeviceObjListFindByNameLocked(devs, name))) { + virHashRemoveEntry(devs->objs, name); + virNodeDeviceObjEndAPI(&obj); + } virObjectRWUnlock(devs); } diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 87f908369..b9752bc62 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -72,7 +72,7 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev); + const char *name); int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 6ad56f416..46a419a6e 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -511,8 +511,8 @@ dev_refresh(const char *udi) /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); + virNodeDeviceObjListRemoveDef(driver->devs, name); dev_create(udi); } else { VIR_DEBUG("no device named %s", name); @@ -536,12 +536,14 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, virNodeDeviceObjPtr obj; obj = virNodeDeviceObjListFindByName(driver->devs, name); - VIR_DEBUG("%s", name); - if (obj) - virNodeDeviceObjListRemove(driver->devs, obj); - else + if (!obj) { VIR_DEBUG("no device named %s", name); - virObjectUnref(obj); + return; + } + + VIR_DEBUG("%s", name); + virNodeDeviceObjEndAPI(&obj); + virNodeDeviceObjListRemove(driver->devs, name); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e87eb32a8..f6d223fe4 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1282,6 +1282,7 @@ udevRemoveOneDevice(struct udev_device *device) virNodeDeviceDefPtr def; virObjectEventPtr event = NULL; const char *name = NULL; + char *defname = NULL; name = udev_device_get_syspath(device); if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, name))) { @@ -1290,15 +1291,19 @@ udevRemoveOneDevice(struct udev_device *device) return -1; } def = virNodeDeviceObjGetDef(obj); + if (VIR_STRDUP(defname, def->name) < 0) { + virNodeDeviceObjEndAPI(&obj); + return -1; + } event = virNodeDeviceEventLifecycleNew(def->name, VIR_NODE_DEVICE_EVENT_DELETED, 0); - VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - def->name, name); - virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); + VIR_DEBUG("Removing device '%s' with sysfs path '%s'", defname, name); + virNodeDeviceObjEndAPI(&obj); + virNodeDeviceObjListRemove(driver->devs, defname); + VIR_FREE(defname); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ddddd8dcb..568d537e9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4723,8 +4723,8 @@ testDestroyVport(testDriverPtr privconn, VIR_NODE_DEVICE_EVENT_DELETED, 0); - virNodeDeviceObjListRemove(privconn->devs, obj); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); + virNodeDeviceObjListRemove(privconn->devs, "scsi_host12"); testObjectEventQueue(privconn, event); return 0; @@ -5670,6 +5670,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virNodeDeviceObjPtr obj = NULL; virNodeDeviceObjPtr parentobj = NULL; virNodeDeviceDefPtr def; + char *parent = NULL; char *wwnn = NULL, *wwpn = NULL; virObjectEventPtr event = NULL; @@ -5681,18 +5682,19 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) goto cleanup; /* Unlike the real code we cannot run into the udevAddOneDevice race - * which would replace obj->def, so no need to save off the parent, - * but do need to drop the @obj lock so that the FindByName code doesn't - * deadlock on ourselves */ - virObjectUnlock(obj); + * which would replace obj->def, but we still need to save off the + * parent name since we're about to Unref and Unlock the @obj containing + * the @def so that we don't deadlock in virNodeDeviceObjListFindByName. */ + if (VIR_STRDUP(parent, def->parent) < 0) + goto cleanup; + + virNodeDeviceObjEndAPI(&obj); /* We do this just for basic validation and throw away the parentobj * since there's no vport_delete to be run */ - if (!(parentobj = virNodeDeviceObjListFindByName(driver->devs, - def->parent))) { + if (!(parentobj = virNodeDeviceObjListFindByName(driver->devs, parent))) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find parent '%s' definition"), def->parent); - virObjectLock(obj); + _("cannot find parent '%s' definition"), parent); goto cleanup; } virNodeDeviceObjEndAPI(&parentobj); @@ -5701,16 +5703,14 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) VIR_NODE_DEVICE_EVENT_DELETED, 0); - virObjectLock(obj); - virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); - obj = NULL; + virNodeDeviceObjListRemove(driver->devs, dev->name); cleanup: virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(driver, event); VIR_FREE(wwnn); VIR_FREE(wwpn); + VIR_FREE(parent); return ret; } -- 2.13.6
Use an error label to converge all the clean up processing options. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 799b8c9fa..471262f29 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1115,8 +1115,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, const char *path, const char *autostartLink) { - virStoragePoolDefPtr def; - virStoragePoolObjPtr obj; + virStoragePoolDefPtr def = NULL; + virStoragePoolObjPtr obj = NULL; if (!(def = virStoragePoolDefParseFile(path))) return NULL; @@ -1126,32 +1126,33 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, _("Storage pool config filename '%s' does " "not match pool name '%s'"), path, def->name); - virStoragePoolDefFree(def); - return NULL; + goto error; } - if (!(obj = virStoragePoolObjAssignDef(pools, def))) { - virStoragePoolDefFree(def); - return NULL; - } + if (!(obj = virStoragePoolObjAssignDef(pools, def))) + goto error; + def = NULL; VIR_FREE(obj->configFile); /* for driver reload */ - if (VIR_STRDUP(obj->configFile, path) < 0) { - virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); - return NULL; - } + if (VIR_STRDUP(obj->configFile, path) < 0) + goto error; + VIR_FREE(obj->autostartLink); /* for driver reload */ - if (VIR_STRDUP(obj->autostartLink, autostartLink) < 0) { - virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); - return NULL; - } + if (VIR_STRDUP(obj->autostartLink, autostartLink) < 0) + goto error; obj->autostart = virFileLinkPointsTo(obj->autostartLink, obj->configFile); return obj; + + error: + if (obj) { + virStoragePoolObjRemove(pools, obj); + virObjectUnref(obj); + } + virStoragePoolDefFree(def); + return NULL; } -- 2.13.6
Rather than 3 separate, but same 4 lines of code - let's create an error label to make a common error path. This will help shortly when the error path changes slightly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d5e38af5a..6276545eb 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -744,22 +744,14 @@ storagePoolCreateXML(virConnectPtr conn, if (build_flags || (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) { - if (backend->buildPool(obj, build_flags) < 0) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + if (backend->buildPool(obj, build_flags) < 0) + goto error; } } if (backend->startPool && - backend->startPool(obj) < 0) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + backend->startPool(obj) < 0) + goto error; stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); @@ -770,10 +762,7 @@ storagePoolCreateXML(virConnectPtr conn, unlink(stateFile); if (backend->stopPool) backend->stopPool(obj); - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; + goto error; } event = virStoragePoolEventLifecycleNew(def->name, @@ -793,6 +782,12 @@ storagePoolCreateXML(virConnectPtr conn, virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool; + + error: + virStoragePoolObjRemove(driver->pools, obj); + virObjectUnref(obj); + obj = NULL; + goto cleanup; } static virStoragePoolPtr -- 2.13.6
Rather than 2 separate, but same 4 lines of code - let's create an error label to make a common error path. This will help shortly when the error path changes slightly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 568d537e9..0a284e3d1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4562,20 +4562,12 @@ testStoragePoolCreateXML(virConnectPtr conn, * rename a few fields to mock that. */ if (testCreateVport(privconn, def->source.adapter.data.fchost.wwnn, - def->source.adapter.data.fchost.wwpn) < 0) { - virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + def->source.adapter.data.fchost.wwpn) < 0) + goto error; } - if (testStoragePoolObjSetDefaults(obj) == -1) { - virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + if (testStoragePoolObjSetDefaults(obj) == -1) + goto error; /* *SetDefaults fills this in for the persistent pools, but this * would be a transient pool so remove it; otherwise, the Destroy @@ -4596,6 +4588,12 @@ testStoragePoolCreateXML(virConnectPtr conn, virStoragePoolObjEndAPI(&obj); testDriverUnlock(privconn); return pool; + + error: + virStoragePoolObjRemove(privconn->pools, obj); + virObjectUnref(obj); + obj = NULL; + goto cleanup; } -- 2.13.6
A subsequent patch will need to use the local FindByUUIDLocked, so rather than create a forward decl or move when needed, let's just do it now for ease of future review. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 471262f29..f48f08a64 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -517,25 +517,6 @@ virStoragePoolObjListSearch(virStoragePoolObjListPtr pools, } -void -virStoragePoolObjRemove(virStoragePoolObjListPtr pools, - virStoragePoolObjPtr obj) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(obj->def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); - virObjectRWLockWrite(pools); - virObjectLock(obj); - virHashRemoveEntry(pools->objs, uuidstr); - virHashRemoveEntry(pools->objsName, obj->def->name); - virObjectUnlock(obj); - virObjectUnref(obj); - virObjectRWUnlock(pools); -} - - static virStoragePoolObjPtr virStoragePoolObjFindByUUIDLocked(virStoragePoolObjListPtr pools, const unsigned char *uuid) @@ -1053,6 +1034,25 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, } +void +virStoragePoolObjRemove(virStoragePoolObjListPtr pools, + virStoragePoolObjPtr obj) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(obj->def->uuid, uuidstr); + virObjectRef(obj); + virObjectUnlock(obj); + virObjectRWLockWrite(pools); + virObjectLock(obj); + virHashRemoveEntry(pools->objs, uuidstr); + virHashRemoveEntry(pools->objsName, obj->def->name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(pools); +} + + /** * virStoragePoolObjAssignDef: * @pools: Storage Pool object list pointer -- 2.13.6
Current processing requires a "fire dance" unlocking the @obj, adding an @obj ref, locking the @pools, and relocking @obj in order to ensure proper lock ordering. This can be avoided by changing virStoragePoolObjRemove to take a @name instead of @obj. Then, we can lock the @pools list, look up the @obj by @name (like we do when adding), and remove the @obj from the list. This removes the last reference to the object effectively reaping it. NB: Since prior to calling we remove the reference to the object we cannot pass anything contained within the object (such as the obj->def or obj->def->name) because it's possible that the object could be reaped by two competing remove threads. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 41 +++++++++++++++++++++++++++++------------ src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 44 +++++++++++++++++++++++++++++--------------- src/test/test_driver.c | 30 +++++++++++++++++++----------- 4 files changed, 78 insertions(+), 39 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index f48f08a64..df6febfd0 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1034,21 +1034,31 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, } +/* + * virStoragePoolObjRemove: + * @pools: list of storage pool objects + * @name: name of storage pool to remove + * + * Find the object by name in the list, remove the object from + * each hash table in the list, and free the object. + * + * Upon entry it's expected that prior to entry any locks on + * the object related to @name will have been removed. + */ void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, - virStoragePoolObjPtr obj) + const char *name) { + virStoragePoolObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); virObjectRWLockWrite(pools); - virObjectLock(obj); - virHashRemoveEntry(pools->objs, uuidstr); - virHashRemoveEntry(pools->objsName, obj->def->name); - virObjectUnlock(obj); - virObjectUnref(obj); + if ((obj = virStoragePoolObjFindByNameLocked(pools, name))) { + virUUIDFormat(obj->def->uuid, uuidstr); + virHashRemoveEntry(pools->objs, uuidstr); + virHashRemoveEntry(pools->objsName, name); + virStoragePoolObjEndAPI(&obj); + } virObjectRWUnlock(pools); } @@ -1117,6 +1127,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, { virStoragePoolDefPtr def = NULL; virStoragePoolObjPtr obj = NULL; + char *name = NULL; if (!(def = virStoragePoolDefParseFile(path))) return NULL; @@ -1129,6 +1140,9 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, goto error; } + if (VIR_STRDUP(name, def->name) < 0) + goto error; + if (!(obj = virStoragePoolObjAssignDef(pools, def))) goto error; def = NULL; @@ -1144,13 +1158,16 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, obj->autostart = virFileLinkPointsTo(obj->autostartLink, obj->configFile); + VIR_FREE(name); + return obj; error: - if (obj) { - virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); + if (obj && name) { + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(pools, name); } + VIR_FREE(name); virStoragePoolDefFree(def); return NULL; } diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index dd7001c4b..047b08a92 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -242,7 +242,7 @@ virStoragePoolObjListNew(void); void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, - virStoragePoolObjPtr obj); + const char *name); int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6276545eb..814e5cb97 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -82,19 +82,21 @@ static void storageDriverUnlock(void) /** * virStoragePoolUpdateInactive: * @poolptr: pointer to a variable holding the pool object pointer + * @name: Name of the pool * * This function is supposed to be called after a pool becomes inactive. The * function switches to the new config object for persistent pools. Inactive * pools are removed. */ static void -virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) +virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr, + const char *name) { virStoragePoolObjPtr obj = *objptr; if (!virStoragePoolObjGetConfigFile(obj)) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(driver->pools, name); *objptr = NULL; } else if (virStoragePoolObjGetNewDef(obj)) { virStoragePoolObjDefUseNewDef(obj); @@ -110,6 +112,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, bool active = false; virStorageBackendPtr backend; char *stateFile; + char *name = NULL; if (!(stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"))) goto cleanup; @@ -120,6 +123,9 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, goto cleanup; } + if (VIR_STRDUP(name, def->name) < 0) + goto cleanup; + /* Backends which do not support 'checkPool' are considered * inactive by default. */ if (backend->checkPool && @@ -149,12 +155,13 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, virStoragePoolObjSetActive(obj, active); if (!virStoragePoolObjIsActive(obj)) - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(&obj, name); cleanup: if (!active && stateFile) ignore_value(unlink(stateFile)); VIR_FREE(stateFile); + VIR_FREE(name); return; } @@ -707,6 +714,7 @@ storagePoolCreateXML(virConnectPtr conn, virStorageBackendPtr backend; virObjectEventPtr event = NULL; char *stateFile = NULL; + char *name = NULL; unsigned int build_flags = 0; virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD | @@ -731,6 +739,9 @@ storagePoolCreateXML(virConnectPtr conn, if ((backend = virStorageBackendForType(newDef->type)) == NULL) goto cleanup; + if (VIR_STRDUP(name, newDef->name) < 0) + goto cleanup; + if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef))) goto cleanup; newDef = NULL; @@ -776,6 +787,7 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: + VIR_FREE(name); VIR_FREE(stateFile); virStoragePoolDefFree(newDef); if (event) @@ -784,9 +796,8 @@ storagePoolCreateXML(virConnectPtr conn, return pool; error: - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(driver->pools, name); goto cleanup; } @@ -800,6 +811,7 @@ storagePoolDefineXML(virConnectPtr conn, virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; virObjectEventPtr event = NULL; + char *name = NULL; virCheckFlags(0, NULL); @@ -821,15 +833,17 @@ storagePoolDefineXML(virConnectPtr conn, if (virStorageBackendForType(newDef->type) == NULL) goto cleanup; + if (VIR_STRDUP(name, newDef->name) < 0) + goto cleanup; + if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); if (virStoragePoolObjSaveDef(driver, obj, def) < 0) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(driver->pools, name); goto cleanup; } @@ -841,6 +855,7 @@ storagePoolDefineXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: + VIR_FREE(name); if (event) virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolDefFree(newDef); @@ -895,9 +910,8 @@ storagePoolUndefine(virStoragePoolPtr pool) 0); VIR_INFO("Undefining storage pool '%s'", def->name); - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(driver->pools, pool->name); ret = 0; cleanup: @@ -1089,7 +1103,7 @@ storagePoolDestroy(virStoragePoolPtr pool) virStoragePoolObjSetActive(obj, false); - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(&obj, pool->name); ret = 0; @@ -1212,7 +1226,7 @@ storagePoolRefresh(virStoragePoolPtr pool, 0); virStoragePoolObjSetActive(obj, false); - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(&obj, pool->name); goto cleanup; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0a284e3d1..70509a204 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4540,6 +4540,7 @@ testStoragePoolCreateXML(virConnectPtr conn, virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; virObjectEventPtr event = NULL; + char *name = NULL; virCheckFlags(0, NULL); @@ -4550,6 +4551,9 @@ testStoragePoolCreateXML(virConnectPtr conn, if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, true) < 0) goto cleanup; + if (VIR_STRDUP(name, newDef->name) < 0) + goto cleanup; + if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef))) goto cleanup; newDef = NULL; @@ -4583,6 +4587,7 @@ testStoragePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: + VIR_FREE(name); virStoragePoolDefFree(newDef); testObjectEventQueue(privconn, event); virStoragePoolObjEndAPI(&obj); @@ -4590,9 +4595,8 @@ testStoragePoolCreateXML(virConnectPtr conn, return pool; error: - virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(privconn->pools, name); goto cleanup; } @@ -4608,6 +4612,7 @@ testStoragePoolDefineXML(virConnectPtr conn, virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; virObjectEventPtr event = NULL; + char *name = NULL; virCheckFlags(0, NULL); @@ -4622,6 +4627,9 @@ testStoragePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, false) < 0) goto cleanup; + if (VIR_STRDUP(name, newDef->name) < 0) + goto cleanup; + if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef))) goto cleanup; newDef = NULL; @@ -4632,15 +4640,15 @@ testStoragePoolDefineXML(virConnectPtr conn, 0); if (testStoragePoolObjSetDefaults(obj) == -1) { - virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(privconn->pools, name); goto cleanup; } pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: + VIR_FREE(name); virStoragePoolDefFree(newDef); testObjectEventQueue(privconn, event); virStoragePoolObjEndAPI(&obj); @@ -4663,8 +4671,8 @@ testStoragePoolUndefine(virStoragePoolPtr pool) VIR_STORAGE_POOL_EVENT_UNDEFINED, 0); - virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(privconn->pools, pool->name); testObjectEventQueue(privconn, event); return 0; @@ -4757,10 +4765,10 @@ testStoragePoolDestroy(virStoragePoolPtr pool) 0); if (!(virStoragePoolObjGetConfigFile(obj))) { - virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; + virStoragePoolObjEndAPI(&obj); + virStoragePoolObjRemove(privconn->pools, pool->name); } + ret = 0; cleanup: -- 2.13.6
ping? Tks - John On 03/28/2018 05:19 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html
NB: This can wait until 4.2.0 is release, but I figured I'd post this now just to put it on the radar and of course in hopes that someone will look during the idle moment or two before the release.
Changes since v1:
Short story: Rework the processing of the code
Longer story:
In his review Erik noted that there's a "fire dance" when processing the vir*Obj*Remove APIs of requiring a locked object upon entry, then adding a reference to that object, unlocking the object, locking the list to which it is contained, and then relocking the object.
So it took some time to think about it and during one lengthy meeting today I had the aha moment that the *Remove API's could take the same key (e.g., uuid or name) used to Add or Find the object and use it for the Remove API. This allows the Remove API to not require a locked (and reffed) object upon entry and perform the lock dance, remove the object, and return just just a reffed object forcing the caller to know to Unref object.
Instead, let's simplify things. The *Remove API can take the key, Find the object in the list, remove it from the hash tables, and dispose of the object. In essence the antecedent to the Add or AssignDef API's taking a def, creating an object, and adding it the object to the hash table(s). If there are two *Remove threads competing, one will win and perform the removal, while the other will call *Remove, but won't find the object in the hash table, and just return none the wiser.
And yes, I think this can also work for the Domain code, but it's going to take a few patch series to get there as that code is not consistent between consumers.
John Ferlan (9): secret: Rework LoadAllConfigs secret: Alter virSecretObjListRemove processing interface: Alter virInterfaceObjListRemove processing nodedev: Alter virNodeDeviceObjListRemove processing conf: Clean up virStoragePoolObjLoad error processing storage: Clean up storagePoolCreateXML error processing test: Clean up testStoragePoolCreateXML error processing conf: Move virStoragePoolObjRemove closer to AssignDef storagepool: Alter virStoragePoolObjRemove processing
src/conf/virinterfaceobj.c | 26 +++++++---- src/conf/virinterfaceobj.h | 2 +- src/conf/virnodedeviceobj.c | 29 +++++++----- src/conf/virnodedeviceobj.h | 2 +- src/conf/virsecretobj.c | 58 +++++++++++------------- src/conf/virsecretobj.h | 2 +- src/conf/virstorageobj.c | 92 +++++++++++++++++++++++--------------- src/conf/virstorageobj.h | 2 +- src/node_device/node_device_hal.c | 16 ++++--- src/node_device/node_device_udev.c | 13 ++++-- src/secret/secret_driver.c | 15 ++++--- src/storage/storage_driver.c | 65 +++++++++++++++------------ src/test/test_driver.c | 78 +++++++++++++++++--------------- 13 files changed, 225 insertions(+), 175 deletions(-)
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html
NB: This can wait until 4.2.0 is release, but I figured I'd post this now just to put it on the radar and of course in hopes that someone will look during the idle moment or two before the release.
Changes since v1:
Short story: Rework the processing of the code
Longer story:
In his review Erik noted that there's a "fire dance" when processing the vir*Obj*Remove APIs of requiring a locked object upon entry, then adding a reference to that object, unlocking the object, locking the list to which it is contained, and then relocking the object.
So it took some time to think about it and during one lengthy meeting today I had the aha moment that the *Remove API's could take the same key (e.g., uuid or name) used to Add or Find the object and use it for the Remove API. This allows the Remove API to not require a locked (and reffed) object upon entry and perform the lock dance, remove the object, and return just just a reffed object forcing the caller to know to Unref object.
Instead, let's simplify things. The *Remove API can take the key, Find the object in the list, remove it from the hash tables, and dispose of the object. In essence the antecedent to the Add or AssignDef API's taking a def, creating an object, and adding it the object to the hash table(s). If there are two *Remove threads competing, one will win and perform the removal, while the other will call *Remove, but won't find the object in the hash table, and just return none the wiser.
And yes, I think this can also work for the Domain code, but it's going to take a few patch series to get there as that code is not consistent between consumers.
John Ferlan (9): secret: Rework LoadAllConfigs secret: Alter virSecretObjListRemove processing interface: Alter virInterfaceObjListRemove processing nodedev: Alter virNodeDeviceObjListRemove processing conf: Clean up virStoragePoolObjLoad error processing storage: Clean up storagePoolCreateXML error processing test: Clean up testStoragePoolCreateXML error processing conf: Move virStoragePoolObjRemove closer to AssignDef storagepool: Alter virStoragePoolObjRemove processing
Side note: Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re looking quite similar? Not sure if it’s easily feasible in all places. […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 04/10/2018 04:47 AM, Marc Hartmayer wrote:
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html
NB: This can wait until 4.2.0 is release, but I figured I'd post this now just to put it on the radar and of course in hopes that someone will look during the idle moment or two before the release.
Changes since v1:
Short story: Rework the processing of the code
Longer story:
In his review Erik noted that there's a "fire dance" when processing the vir*Obj*Remove APIs of requiring a locked object upon entry, then adding a reference to that object, unlocking the object, locking the list to which it is contained, and then relocking the object.
So it took some time to think about it and during one lengthy meeting today I had the aha moment that the *Remove API's could take the same key (e.g., uuid or name) used to Add or Find the object and use it for the Remove API. This allows the Remove API to not require a locked (and reffed) object upon entry and perform the lock dance, remove the object, and return just just a reffed object forcing the caller to know to Unref object.
Instead, let's simplify things. The *Remove API can take the key, Find the object in the list, remove it from the hash tables, and dispose of the object. In essence the antecedent to the Add or AssignDef API's taking a def, creating an object, and adding it the object to the hash table(s). If there are two *Remove threads competing, one will win and perform the removal, while the other will call *Remove, but won't find the object in the hash table, and just return none the wiser.
And yes, I think this can also work for the Domain code, but it's going to take a few patch series to get there as that code is not consistent between consumers.
John Ferlan (9): secret: Rework LoadAllConfigs secret: Alter virSecretObjListRemove processing interface: Alter virInterfaceObjListRemove processing nodedev: Alter virNodeDeviceObjListRemove processing conf: Clean up virStoragePoolObjLoad error processing storage: Clean up storagePoolCreateXML error processing test: Clean up testStoragePoolCreateXML error processing conf: Move virStoragePoolObjRemove closer to AssignDef storagepool: Alter virStoragePoolObjRemove processing
Side note: Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re looking quite similar? Not sure if it’s easily feasible in all places.
Well - that was the point of what I started last year, but there hasn't been any general agreement or acceptance of patches for that. My changes made use of objects and more generic naming to unify things; however, they weren't acceptable with (IIRC and in my words) the preference being more specific naming using switch/case statements and shim API's. The last full posting (a v5) of what I had done is here: https://www.redhat.com/archives/libvir-list/2017-August/msg00659.html If you feel so inclined you can follow the history of comments through v4: https://www.redhat.com/archives/libvir-list/2017-August/msg00537.html and v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html w/ review comments starting here: https://www.redhat.com/archives/libvir-list/2017-July/msg01032.html Maybe once the domain code is modified to be more common (in process now) and if nwfilter ever could gain acceptance, something could be done. Still I have my doubts it'll happen especially since nwfilter patches just cannot get any sort of agreement, last try here: https://www.redhat.com/archives/libvir-list/2018-February/msg00325.html John
[…snip]
-- Beste Grüße / Kind regards Marc Hartmayer
IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
John Ferlan -
Marc Hartmayer