4:19 p.m.
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
4:19 p.m.
New subject: [libvirt] [PATCH v2 1/9] secret: Rework LoadAllConfigs
Move the virSecretObjEndAPI into virSecretLoad and alter the processing
in order to accomodate that.
Signed-off-by: John Ferlan <jferlan(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 2/9] secret: Alter virSecretObjListRemove processing
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(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 3/9] interface: Alter virInterfaceObjListRemove processing
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(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 4/9] nodedev: Alter virNodeDeviceObjListRemove processing
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(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 5/9] conf: Clean up virStoragePoolObjLoad error processing
Use an error label to converge all the clean up processing options.
Signed-off-by: John Ferlan <jferlan(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 6/9] storage: Clean up storagePoolCreateXML error processing
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(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 7/9] test: Clean up testStoragePoolCreateXML error processing
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(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 8/9] conf: Move virStoragePoolObjRemove closer to AssignDef
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(a)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
4:19 p.m.
New subject: [libvirt] [PATCH v2 9/9] storagepool: Alter virStoragePoolObjRemove processing
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(a)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
6:20 a.m.
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(-)
3:47 a.m.
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan(a)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
7:03 a.m.
On 04/10/2018 04:47 AM, Marc Hartmayer wrote:
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan
<jferlan(a)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
2453
days inactive
2466
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Marc Hartmayer