[libvirt] [PATCH v5 0/3] Make virNodeDeviceObj and virNodeDeviceObjList private

NB: Kept same title for consistency, but it's less true as what's left deals with using the now provide ObjList in order to change from a forward linked list to a hash lookup (thus conceivably speeding up searches *and* removing the need for driver level locks since the ObjList is self lockable. v4: https://www.redhat.com/archives/libvir-list/2017-July/msg00034.html Changes since v4: -> Patches 1-12 from v4 pushed -> Patch 1 (NEW) - Fix the virNodeDeviceObjListGetParentHost to not pass @def, rather pass the individual names being searched since the Deletion code needs to unlock the object and thus @def prior to calling and that could result in a possible, but improbable udevAddOneDevice change event that would have replaced @def. -> Patch 2 (former patch 13) - No change -> Patch 3 (former patch 14) - No change John Ferlan (3): nodedev: Fix usage of virNodeDeviceObjListGetParentHost nodedev: Convert virNodeDeviceObjListPtr to use hash tables nodedev: Remove driver locks around object list mgmt code src/conf/virnodedeviceobj.c | 620 ++++++++++++++++++++++++----------- src/conf/virnodedeviceobj.h | 8 +- src/node_device/node_device_driver.c | 101 +++--- src/node_device/node_device_hal.c | 16 +- src/node_device/node_device_udev.c | 13 +- src/test/test_driver.c | 41 ++- 6 files changed, 523 insertions(+), 276 deletions(-) -- 2.9.4

In order to avoid an eventual possible race, modify the call to not use the @def, but rather take the various fields needed. The race could occur because the Destroy path needs to Unlock (and Unref) the object. This could lead to an eventual change event from udevAddOneDevice which would free obj->def and it's fields that this code would be attempting to reference. So better safe than sorry. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 45 +++++++++++++++++++++++++++--------- src/conf/virnodedeviceobj.h | 8 +++++-- src/node_device/node_device_driver.c | 40 +++++++++++++++++++++++++++----- src/test/test_driver.c | 41 ++++++++++++++++++++++++-------- 4 files changed, 105 insertions(+), 29 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 4c5ee8c..8416dda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -543,24 +543,47 @@ virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) } +/** + * @devs: Pointer to the device list + * @name: The name of the device + * @parent: The parent name + * @parent_wwnn: Parent's WWNN value + * @parent_wwpn: Parent's WWPN value + * @parent_fabric_wwn: Parent's fabric WWN value + * @create: Whether this is a create or existing device + * + * Using the input name and parent values, let's look for a parent host. + * This API cannot take the @def because it's called in a Destroy path which + * needs to unlock the object prior to calling since the searching involves + * locking the object (avoids deadlock). Once the lock is removed, it's + * possible, but improbable that udevAddOneDevice could have a "change" + * event on an existing vHBA causing the @def to be replaced in the object. + * If we use that @def, then we run the risk of some bad things happening + * + * Returns parent_host value on success or -1 on failure + */ int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, + const char *name, + const char *parent, + const char *parent_wwnn, + const char *parent_wwpn, + const char *parent_fabric_wwn, int create) { int parent_host = -1; - if (def->parent) { - parent_host = virNodeDeviceObjListGetParentHostByParent(devs, def->name, - def->parent); - } else if (def->parent_wwnn && def->parent_wwpn) { - parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, def->name, - def->parent_wwnn, - def->parent_wwpn); - } else if (def->parent_fabric_wwn) { + if (parent) { + parent_host = virNodeDeviceObjListGetParentHostByParent(devs, name, + parent); + } else if (parent_wwnn && parent_wwpn) { + parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, name, + parent_wwnn, + parent_wwpn); + } else if (parent_fabric_wwn) { parent_host = - virNodeDeviceObjListGetParentHostByFabricWWN(devs, def->name, - def->parent_fabric_wwn); + virNodeDeviceObjListGetParentHostByFabricWWN(devs, name, + parent_fabric_wwn); } else if (create == CREATE_DEVICE) { /* Try to find a vport capable scsi_host when no parent supplied */ parent_host = virNodeDeviceObjListFindVportParentHost(devs); diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 788fb66..f5ea8fe 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -75,8 +75,12 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, - int create); + const char *name, + const char *parent, + const char *parent_wwnn, + const char *parent_wwpn, + const char *parent_fabric_wwn, + int create); virNodeDeviceObjListPtr virNodeDeviceObjListNew(void); diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0a19908..1f888fb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -563,7 +563,13 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + /* NB: Since there's no @obj for which @def is assigned to, we can use + * the def-> values directly - unlike the Destroy code */ + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, + def->name, def->parent, + def->parent_wwnn, + def->parent_wwpn, + def->parent_fabric_wwn, CREATE_DEVICE)) < 0) goto cleanup; @@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1; @@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup; - /* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */ + if (VIR_STRDUP(name, def->name) < 0 || + VIR_STRDUP(parent, def->parent) < 0 || + VIR_STRDUP(parent_wwnn, def->parent_wwnn) < 0 || + VIR_STRDUP(parent_wwpn, def->parent_wwpn) < 0 || + VIR_STRDUP(parent_fabric_wwn, def->parent_fabric_wwn) < 0) + goto cleanup; + virNodeDeviceObjEndAPI(&obj); - if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, + name, parent, + parent_wwnn, + parent_wwpn, + parent_fabric_wwn, EXISTING_DEVICE)) < 0) goto cleanup; @@ -626,6 +649,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) cleanup: nodeDeviceUnlock(); virNodeDeviceObjEndAPI(&obj); + VIR_FREE(name); + VIR_FREE(parent); + VIR_FREE(parent_wwnn); + VIR_FREE(parent_wwpn); + VIR_FREE(parent_fabric_wwn); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 83ab9cc..0e22ec2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5579,8 +5579,13 @@ testNodeDeviceCreateXML(virConnectPtr conn, /* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds - * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjListGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) + * something valid and no one messed up the mock environment. We also + * don't need to make local copies since there's no @obj for which + * @def is assigned to, so we can use the def-> values directly. */ + if (virNodeDeviceObjListGetParentHost(driver->devs, def->name, def->parent, + def->parent_wwnn, def->parent_wwpn, + def->parent_fabric_wwn, + CREATE_DEVICE) < 0) goto cleanup; /* In the real code, we'd call virVHBAManageVport followed by @@ -5619,7 +5624,12 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) testDriverPtr driver = dev->conn->privateData; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; - char *parent_name = NULL, *wwnn = NULL, *wwpn = NULL; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; + char *wwnn = NULL, *wwpn = NULL; virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) @@ -5629,18 +5639,25 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - if (VIR_STRDUP(parent_name, def->parent) < 0) + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */ + if (VIR_STRDUP(name, def->name) < 0 || + VIR_STRDUP(parent, def->parent) < 0 || + VIR_STRDUP(parent_wwnn, def->parent_wwnn) < 0 || + VIR_STRDUP(parent_wwpn, def->parent_wwpn) < 0 || + VIR_STRDUP(parent_fabric_wwn, def->parent_fabric_wwn) < 0) goto cleanup; - /* virNodeDeviceGetParentHost will cause the device object's lock to be - * taken, so we have to dup the parent's name and drop the lock - * before calling it. We don't need the reference to the object - * any more once we have the parent's name. */ virObjectUnlock(obj); /* We do this just for basic validation, but also avoid finding a * vport capable HBA if for some reason our vHBA doesn't exist */ - if (virNodeDeviceObjListGetParentHost(driver->devs, def, + if (virNodeDeviceObjListGetParentHost(driver->devs, name, parent, + parent_wwnn, parent_wwpn, + parent_fabric_wwn, EXISTING_DEVICE) < 0) { virObjectLock(obj); goto cleanup; @@ -5658,7 +5675,11 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) cleanup: virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(driver, event); - VIR_FREE(parent_name); + VIR_FREE(name); + VIR_FREE(parent); + VIR_FREE(parent_wwnn); + VIR_FREE(parent_wwpn); + VIR_FREE(parent_fabric_wwn); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; -- 2.9.4

On Mon, Jul 17, 2017 at 11:02:22AM -0400, John Ferlan wrote:
In order to avoid an eventual possible race, modify the call to not use the @def, but rather take the various fields needed. The race could occur because the Destroy path needs to Unlock (and Unref) the object. This could lead to an eventual change event from udevAddOneDevice which would free obj->def and it's fields that this code would be attempting to reference. So better safe than sorry.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 45 +++++++++++++++++++++++++++--------- src/conf/virnodedeviceobj.h | 8 +++++-- src/node_device/node_device_driver.c | 40 +++++++++++++++++++++++++++----- src/test/test_driver.c | 41 ++++++++++++++++++++++++-------- 4 files changed, 105 insertions(+), 29 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 4c5ee8c..8416dda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -543,24 +543,47 @@ virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) }
+/** + * @devs: Pointer to the device list + * @name: The name of the device + * @parent: The parent name + * @parent_wwnn: Parent's WWNN value + * @parent_wwpn: Parent's WWPN value + * @parent_fabric_wwn: Parent's fabric WWN value + * @create: Whether this is a create or existing device
IMHO the fact that an unlikely race might happen should stay transparent to this function, thus I'd personally drop this commentary and leave the argslist, i.e. passing @def untouched.
+ * + * Using the input name and parent values, let's look for a parent host. + * This API cannot take the @def because it's called in a Destroy path which + * needs to unlock the object prior to calling since the searching involves + * locking the object (avoids deadlock). Once the lock is removed, it's + * possible, but improbable that udevAddOneDevice could have a "change" + * event on an existing vHBA causing the @def to be replaced in the object. + * If we use that @def, then we run the risk of some bad things happening + * + * Returns parent_host value on success or -1 on failure + */ int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, + const char *name, + const char *parent, + const char *parent_wwnn, + const char *parent_wwpn, + const char *parent_fabric_wwn, int create) { int parent_host = -1;
- if (def->parent) { - parent_host = virNodeDeviceObjListGetParentHostByParent(devs, def->name, - def->parent); - } else if (def->parent_wwnn && def->parent_wwpn) { - parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, def->name, - def->parent_wwnn, - def->parent_wwpn); - } else if (def->parent_fabric_wwn) { + if (parent) { + parent_host = virNodeDeviceObjListGetParentHostByParent(devs, name, + parent); + } else if (parent_wwnn && parent_wwpn) { + parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, name, + parent_wwnn, + parent_wwpn); + } else if (parent_fabric_wwn) { parent_host = - virNodeDeviceObjListGetParentHostByFabricWWN(devs, def->name, - def->parent_fabric_wwn); + virNodeDeviceObjListGetParentHostByFabricWWN(devs, name, + parent_fabric_wwn); } else if (create == CREATE_DEVICE) { /* Try to find a vport capable scsi_host when no parent supplied */ parent_host = virNodeDeviceObjListFindVportParentHost(devs); diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 788fb66..f5ea8fe 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -75,8 +75,12 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, - int create); + const char *name, + const char *parent, + const char *parent_wwnn, + const char *parent_wwpn, + const char *parent_fabric_wwn, + int create);
virNodeDeviceObjListPtr virNodeDeviceObjListNew(void); diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0a19908..1f888fb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -563,7 +563,13 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup;
- if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + /* NB: Since there's no @obj for which @def is assigned to, we can use + * the def-> values directly - unlike the Destroy code */
Here the commentary makes perfect sense. How about simply: No @obj has @def assigned so we can use it here safely ?
+ if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, + def->name, def->parent, + def->parent_wwnn, + def->parent_wwpn, + def->parent_fabric_wwn, CREATE_DEVICE)) < 0) goto cleanup;
@@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1;
@@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup;
- /* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */
And as I originally suggested I would allocate a new temporary @def structure, initialize it, pass it to the *GetParentHost method like nothing out of the ordinary happened and mentioned in the commentary why we've done it that way (which you already do in this patch).
+ if (VIR_STRDUP(name, def->name) < 0 || + VIR_STRDUP(parent, def->parent) < 0 || + VIR_STRDUP(parent_wwnn, def->parent_wwnn) < 0 || + VIR_STRDUP(parent_wwpn, def->parent_wwpn) < 0 || + VIR_STRDUP(parent_fabric_wwn, def->parent_fabric_wwn) < 0) + goto cleanup; + virNodeDeviceObjEndAPI(&obj); - if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, + name, parent, + parent_wwnn, + parent_wwpn, + parent_fabric_wwn, EXISTING_DEVICE)) < 0) goto cleanup;
@@ -626,6 +649,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) cleanup: nodeDeviceUnlock(); virNodeDeviceObjEndAPI(&obj); + VIR_FREE(name); + VIR_FREE(parent); + VIR_FREE(parent_wwnn); + VIR_FREE(parent_wwpn); + VIR_FREE(parent_fabric_wwn); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 83ab9cc..0e22ec2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5579,8 +5579,13 @@ testNodeDeviceCreateXML(virConnectPtr conn,
/* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds - * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjListGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) + * something valid and no one messed up the mock environment. We also + * don't need to make local copies since there's no @obj for which + * @def is assigned to, so we can use the def-> values directly. */
Same suggestion about the commentary as I mentioned above - would that work out for you? ACK if we can agree on the suggestions. Erik

On 07/19/2017 09:58 AM, Erik Skultety wrote:
On Mon, Jul 17, 2017 at 11:02:22AM -0400, John Ferlan wrote:
In order to avoid an eventual possible race, modify the call to not use the @def, but rather take the various fields needed. The race could occur because the Destroy path needs to Unlock (and Unref) the object. This could lead to an eventual change event from udevAddOneDevice which would free obj->def and it's fields that this code would be attempting to reference. So better safe than sorry.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 45 +++++++++++++++++++++++++++--------- src/conf/virnodedeviceobj.h | 8 +++++-- src/node_device/node_device_driver.c | 40 +++++++++++++++++++++++++++----- src/test/test_driver.c | 41 ++++++++++++++++++++++++-------- 4 files changed, 105 insertions(+), 29 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 4c5ee8c..8416dda 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -543,24 +543,47 @@ virNodeDeviceObjListFindVportParentHost(virNodeDeviceObjListPtr devs) }
+/** + * @devs: Pointer to the device list + * @name: The name of the device + * @parent: The parent name + * @parent_wwnn: Parent's WWNN value + * @parent_wwpn: Parent's WWPN value + * @parent_fabric_wwn: Parent's fabric WWN value + * @create: Whether this is a create or existing device
IMHO the fact that an unlikely race might happen should stay transparent to this function, thus I'd personally drop this commentary and leave the argslist, i.e. passing @def untouched.
The commentary is to make it obvious *here* rather than having someone needing to research git history to get the same explanation from a commit message before they go and change the arguments to be @def and essentially undo what was done.
+ * + * Using the input name and parent values, let's look for a parent host. + * This API cannot take the @def because it's called in a Destroy path which + * needs to unlock the object prior to calling since the searching involves + * locking the object (avoids deadlock). Once the lock is removed, it's + * possible, but improbable that udevAddOneDevice could have a "change" + * event on an existing vHBA causing the @def to be replaced in the object. + * If we use that @def, then we run the risk of some bad things happening + * + * Returns parent_host value on success or -1 on failure + */ int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, + const char *name, + const char *parent, + const char *parent_wwnn, + const char *parent_wwpn, + const char *parent_fabric_wwn, int create) { int parent_host = -1;
- if (def->parent) { - parent_host = virNodeDeviceObjListGetParentHostByParent(devs, def->name, - def->parent); - } else if (def->parent_wwnn && def->parent_wwpn) { - parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, def->name, - def->parent_wwnn, - def->parent_wwpn); - } else if (def->parent_fabric_wwn) { + if (parent) { + parent_host = virNodeDeviceObjListGetParentHostByParent(devs, name, + parent); + } else if (parent_wwnn && parent_wwpn) { + parent_host = virNodeDeviceObjListGetParentHostByWWNs(devs, name, + parent_wwnn, + parent_wwpn); + } else if (parent_fabric_wwn) { parent_host = - virNodeDeviceObjListGetParentHostByFabricWWN(devs, def->name, - def->parent_fabric_wwn); + virNodeDeviceObjListGetParentHostByFabricWWN(devs, name, + parent_fabric_wwn); } else if (create == CREATE_DEVICE) { /* Try to find a vport capable scsi_host when no parent supplied */ parent_host = virNodeDeviceObjListFindVportParentHost(devs); diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 788fb66..f5ea8fe 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -75,8 +75,12 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, - virNodeDeviceDefPtr def, - int create); + const char *name, + const char *parent, + const char *parent_wwnn, + const char *parent_wwpn, + const char *parent_fabric_wwn, + int create);
virNodeDeviceObjListPtr virNodeDeviceObjListNew(void); diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0a19908..1f888fb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -563,7 +563,13 @@ nodeDeviceCreateXML(virConnectPtr conn, if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup;
- if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + /* NB: Since there's no @obj for which @def is assigned to, we can use + * the def-> values directly - unlike the Destroy code */
Here the commentary makes perfect sense. How about simply: No @obj has @def assigned so we can use it here safely ?
I can change to "@obj has not yet been added to the domain object list, so use of @def fields directly is safe."
+ if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, + def->name, def->parent, + def->parent_wwnn, + def->parent_wwpn, + def->parent_fabric_wwn, CREATE_DEVICE)) < 0) goto cleanup;
@@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1;
@@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup;
- /* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */
And as I originally suggested I would allocate a new temporary @def structure, initialize it, pass it to the *GetParentHost method like nothing out of the ordinary happened and mentioned in the commentary why we've done it that way (which you already do in this patch).
So you'd prefer some sort of virNodeDeviceDefCopy function be created? Or use VIR_ALLOC(def) and copy in the 5 fields only to then virNodeDeviceDefFree() it afterwards? Seems like overkill to me. And of course assumes that at no point in the future would virNodeDeviceObjListGetParentHost need some other field that every consumer would need to be made aware about/of... John
+ if (VIR_STRDUP(name, def->name) < 0 || + VIR_STRDUP(parent, def->parent) < 0 || + VIR_STRDUP(parent_wwnn, def->parent_wwnn) < 0 || + VIR_STRDUP(parent_wwpn, def->parent_wwpn) < 0 || + VIR_STRDUP(parent_fabric_wwn, def->parent_fabric_wwn) < 0) + goto cleanup; + virNodeDeviceObjEndAPI(&obj); - if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def, + if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, + name, parent, + parent_wwnn, + parent_wwpn, + parent_fabric_wwn, EXISTING_DEVICE)) < 0) goto cleanup;
@@ -626,6 +649,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) cleanup: nodeDeviceUnlock(); virNodeDeviceObjEndAPI(&obj); + VIR_FREE(name); + VIR_FREE(parent); + VIR_FREE(parent_wwnn); + VIR_FREE(parent_wwpn); + VIR_FREE(parent_fabric_wwn); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 83ab9cc..0e22ec2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5579,8 +5579,13 @@ testNodeDeviceCreateXML(virConnectPtr conn,
/* Unlike the "real" code we don't need the parent_host in order to * call virVHBAManageVport, but still let's make sure the code finds - * something valid and no one messed up the mock environment. */ - if (virNodeDeviceObjListGetParentHost(driver->devs, def, CREATE_DEVICE) < 0) + * something valid and no one messed up the mock environment. We also + * don't need to make local copies since there's no @obj for which + * @def is assigned to, so we can use the def-> values directly. */
Same suggestion about the commentary as I mentioned above - would that work out for you?
ACK if we can agree on the suggestions.
Erik

@@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1;
@@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup;
- /* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */
And as I originally suggested I would allocate a new temporary @def structure, initialize it, pass it to the *GetParentHost method like nothing out of the ordinary happened and mentioned in the commentary why we've done it that way (which you already do in this patch).
So you'd prefer some sort of virNodeDeviceDefCopy function be created? Or use VIR_ALLOC(def) and copy in the 5 fields only to then virNodeDeviceDefFree() it afterwards?
Yeah, exactly, I'm aware of those unused extra field, I don't know it just looked more appealing and transparent to me, again, matter of preference, the way I see it, you store/pass it in a much more compact way with the added benefit of other, in this case currently unused, fields, should virNodeDeviceObjListGetParentHost ever need them. Erik

On 07/20/2017 03:22 AM, Erik Skultety wrote:
@@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1;
@@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup;
- /* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */
And as I originally suggested I would allocate a new temporary @def structure, initialize it, pass it to the *GetParentHost method like nothing out of the ordinary happened and mentioned in the commentary why we've done it that way (which you already do in this patch).
So you'd prefer some sort of virNodeDeviceDefCopy function be created? Or use VIR_ALLOC(def) and copy in the 5 fields only to then virNodeDeviceDefFree() it afterwards?
Yeah, exactly, I'm aware of those unused extra field, I don't know it just looked more appealing and transparent to me, again, matter of preference, the way I see it, you store/pass it in a much more compact way with the added benefit of other, in this case currently unused, fields, should virNodeDeviceObjListGetParentHost ever need them.
Erik
You cut off my next line: "Seems like overkill to me." Still in actually trying to implement that - I've come to realize that @def for the CREATE path will be the only time parent_wwnn, parent_wwpn, and parent_fabric_wwn actually exist. They're not saved in the vHBA that's created... All that the created vHBA gets is the <parent>, so the delete paths do not have to call virNodeDeviceObjListGetParentHost instead they can just copy the parent_name and make a nodeDeviceObjFindByName on the parent after unlocking the original obj, then call virVHBAManageVport with the parent_host # That of course means virNodeDeviceObjListGetParentHost doesn't need the third parameter either. And I don't have to create virNodeDeviceDefCopy (although it's a benign exercise). John

Rather than use a forward linked list of elements, it'll be much more efficient to use a hash table to reference the elements by unique name and to perform hash searches. This patch does all the heavy lifting of converting the list object to use a self locking list that contains the hash table. Each of the FindBy functions that do not involve finding the object by it's key (name) is converted to use virHashSearch in order to find the specific object. When searching for the key (name), it's possible to use virHashLookup. For any of the list perusal functions that are required to evaluate each object, the virHashForEach function is used. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 575 ++++++++++++++++++++++++++++++-------------- 1 file changed, 400 insertions(+), 175 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 8416dda..acf88db 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -25,6 +25,7 @@ #include "viralloc.h" #include "virnodedeviceobj.h" #include "virerror.h" +#include "virhash.h" #include "virlog.h" #include "virstring.h" @@ -39,13 +40,19 @@ struct _virNodeDeviceObj { }; struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; + virObjectLockable parent; + + /* name string -> virNodeDeviceObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + }; static virClassPtr virNodeDeviceObjClass; +static virClassPtr virNodeDeviceObjListClass; static void virNodeDeviceObjDispose(void *opaque); +static void virNodeDeviceObjListDispose(void *opaque); static int virNodeDeviceObjOnceInit(void) @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void) virNodeDeviceObjDispose))) return -1; + if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), + "virNodeDeviceObjList", + sizeof(virNodeDeviceObjList), + virNodeDeviceObjListDispose))) + return -1; + return 0; } @@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) } +static int +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def; + const char *sysfs_path = opaque; + int want = 0; + + virObjectLock(obj); + def = obj->def; + if (STREQ_NULLABLE(def->sysfs_path, sysfs_path)) + want = 1; + virObjectUnlock(obj); + return want; +} + + virNodeDeviceObjPtr virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, const char *sysfs_path) { - size_t i; + virNodeDeviceObjPtr obj; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback, + (void *)sysfs_path); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - def = obj->def; - if ((def->sysfs_path != NULL) && - (STREQ(def->sysfs_path, sysfs_path))) { - return virObjectRef(obj); - } - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static virNodeDeviceObjPtr +virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs, + const char *name) +{ + return virObjectRef(virHashLookup(devs->objs, name)); } @@ -238,20 +274,42 @@ virNodeDeviceObjPtr virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, const char *name) { - size_t i; - - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virNodeDeviceObjPtr obj; + virObjectLock(devs); + obj = virNodeDeviceObjListFindByNameLocked(devs, name); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +struct virNodeDeviceObjListFindByWWNsData { + const char *parent_wwnn; + const char *parent_wwpn; +}; + +static int +virNodeDeviceObjListFindByWWNsCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + struct virNodeDeviceObjListFindByWWNsData *data = + (struct virNodeDeviceObjListFindByWWNsData *) opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && + STREQ_NULLABLE(cap->data.scsi_host.wwnn, data->parent_wwnn) && + STREQ_NULLABLE(cap->data.scsi_host.wwpn, data->parent_wwpn) && + virNodeDeviceFindVPORTCapDef(obj)) + want = 1; + virObjectUnlock(obj); + return want; } @@ -260,22 +318,40 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, const char *parent_wwnn, const char *parent_wwpn) { - size_t i; + virNodeDeviceObjPtr obj; + struct virNodeDeviceObjListFindByWWNsData data = { + .parent_wwnn = parent_wwnn, .parent_wwpn = parent_wwpn }; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByWWNsCallback, + &data); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - if ((cap = virNodeDeviceFindFCCapDef(obj)) && - STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && - STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && - virNodeDeviceFindVPORTCapDef(obj)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static int +virNodeDeviceObjListFindByFabricWWNCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + const char *matchstr = opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && + STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, matchstr) && + virNodeDeviceFindVPORTCapDef(obj)) + want = 1; + virObjectUnlock(obj); + return want; } @@ -283,21 +359,35 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, const char *parent_fabric_wwn) { - size_t i; + virNodeDeviceObjPtr obj; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByFabricWWNCallback, + (void *)parent_fabric_wwn); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - if ((cap = virNodeDeviceFindFCCapDef(obj)) && - STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && - virNodeDeviceFindVPORTCapDef(obj)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static int +virNodeDeviceObjListFindByCapCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + const char *matchstr = opaque; + int want = 0; + + virObjectLock(obj); + if (virNodeDeviceObjHasCap(obj, matchstr)) + want = 1; + virObjectUnlock(obj); + return want; } @@ -305,18 +395,59 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, const char *cap) { - size_t i; + virNodeDeviceObjPtr obj; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByCapCallback, + (void *)cap); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - if (virNodeDeviceObjHasCap(obj, cap)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +struct virNodeDeviceObjListFindSCSIHostByWWNsData { + const char *wwnn; + const char *wwpn; +}; + +static int +virNodeDeviceObjListFindSCSIHostByWWNsCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceObjListFindSCSIHostByWWNsData *data = + (struct virNodeDeviceObjListFindSCSIHostByWWNsData *) opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + def = obj->def; + cap = def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, data->wwnn) && + STREQ(cap->data.scsi_host.wwpn, data->wwpn)) { + want = 1; + break; + } + } + } + cap = cap->next; + } + + virObjectUnlock(obj); + return want; } @@ -325,31 +456,30 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, const char *wwnn, const char *wwpn) { - size_t i; + virNodeDeviceObjPtr obj; + struct virNodeDeviceObjListFindSCSIHostByWWNsData data = { + .wwnn = wwnn, .wwpn = wwpn }; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, + virNodeDeviceObjListFindSCSIHostByWWNsCallback, + &data); + virObjectRef(obj); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - cap = obj->def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) - return virObjectRef(obj); - } - } - cap = cap->next; - } - virObjectUnlock(obj); - } - return NULL; + return obj; +} + + +static void +virNodeDeviceObjListDispose(void *obj) +{ + virNodeDeviceObjListPtr devs = obj; + + virHashFree(devs->objs); } @@ -358,8 +488,17 @@ virNodeDeviceObjListNew(void) { virNodeDeviceObjListPtr devs; - if (VIR_ALLOC(devs) < 0) + if (virNodeDeviceObjInitialize() < 0) + return NULL; + + if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass))) return NULL; + + if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) { + virObjectUnref(devs); + return NULL; + } + return devs; } @@ -367,11 +506,7 @@ virNodeDeviceObjListNew(void) void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { - size_t i; - for (i = 0; i < devs->count; i++) - virObjectUnref(devs->objs[i]); - VIR_FREE(devs->objs); - VIR_FREE(devs); + virObjectUnref(devs); } @@ -381,22 +516,28 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, { virNodeDeviceObjPtr obj; - if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) { + virObjectLock(devs); + + if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) { + virObjectLock(obj); virNodeDeviceDefFree(obj->def); obj->def = def; - return obj; - } + } else { + if (!(obj = virNodeDeviceObjNew())) + goto cleanup; - if (!(obj = virNodeDeviceObjNew())) - return NULL; + if (virHashAddEntry(devs->objs, def->name, obj) < 0) { + virNodeDeviceObjEndAPI(&obj); + goto cleanup; + } - if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { - virNodeDeviceObjEndAPI(&obj); - return NULL; + obj->def = def; + virObjectRef(obj); } - obj->def = def; - return virObjectRef(obj); + cleanup: + virObjectUnlock(devs); + return obj; } @@ -404,21 +545,20 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - size_t i; - - virObjectUnlock(obj); + virNodeDeviceDefPtr def; - for (i = 0; i < devs->count; i++) { - virObjectLock(devs->objs[i]); - if (devs->objs[i] == obj) { - virObjectUnlock(devs->objs[i]); - virObjectUnref(devs->objs[i]); + if (!obj) + return; + def = obj->def; - VIR_DELETE_ELEMENT(devs->objs, i, devs->count); - break; - } - virObjectUnlock(devs->objs[i]); - } + virObjectRef(obj); + virObjectUnlock(obj); + virObjectLock(devs); + virObjectLock(obj); + virHashRemoveEntry(devs->objs, def->name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectUnlock(devs); } @@ -643,25 +783,89 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, } +struct virNodeDeviceCountData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + const char *matchstr; + int count; +}; + +static int +virNodeDeviceObjListNumOfDevicesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceCountData *data = opaque; + virNodeDeviceObjListFilter aclfilter = data->aclfilter; + + virObjectLock(obj); + def = obj->def; + if ((!aclfilter || aclfilter(data->conn, def)) && + (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) + data->count++; + + virObjectUnlock(obj); + return 0; +} + + int virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, virConnectPtr conn, const char *cap, virNodeDeviceObjListFilter aclfilter) { - size_t i; - int ndevs = 0; + struct virNodeDeviceCountData data = { + .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .count = 0 }; - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virObjectLock(obj); - if ((!aclfilter || aclfilter(conn, obj->def)) && - (!cap || virNodeDeviceObjHasCap(obj, cap))) - ++ndevs; - virObjectUnlock(obj); - } + virObjectLock(devs); + virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data); + virObjectUnlock(devs); + + return data.count; +} + + +struct virNodeDeviceGetNamesData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + const char *matchstr; + int nnames; + char **names; + int maxnames; + bool error; +}; + +static int +virNodeDeviceObjListGetNamesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceGetNamesData *data = opaque; + virNodeDeviceObjListFilter aclfilter = data->aclfilter; + + if (data->error) + return 0; - return ndevs; + virObjectLock(obj); + def = obj->def; + + if ((!aclfilter || aclfilter(data->conn, def)) && + (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) { + if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nnames++; + } + + cleanup: + virObjectUnlock(obj); + return 0; } @@ -673,28 +877,22 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, char **const names, int maxnames) { - int nnames = 0; - size_t i; + struct virNodeDeviceGetNamesData data = { + .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .names = names, + .nnames = 0, .maxnames = maxnames, .error = false }; - for (i = 0; i < devs->count && nnames < maxnames; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virObjectLock(obj); - if ((!aclfilter || aclfilter(conn, obj->def)) && - (!cap || virNodeDeviceObjHasCap(obj, cap))) { - if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); - } + virObjectLock(devs); + virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data); + virObjectUnlock(devs); + + if (data.error) + goto error; - return nnames; + return data.nnames; - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + error: + while (--data.nnames) + VIR_FREE(data.names[data.nnames]); return -1; } @@ -731,6 +929,51 @@ virNodeDeviceMatch(virNodeDeviceObjPtr obj, #undef MATCH +struct virNodeDeviceObjListExportData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + unsigned int flags; + virNodeDevicePtr *devices; + int ndevices; + bool error; +}; + +static int +virNodeDeviceObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceObjListExportData *data = opaque; + virNodeDevicePtr device = NULL; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; + + if ((!data->aclfilter || data->aclfilter(data->conn, def)) && + virNodeDeviceMatch(obj, data->flags)) { + if (data->devices) { + if (!(device = virGetNodeDevice(data->conn, def->name)) || + VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + data->error = true; + goto cleanup; + } + data->devices[data->ndevices] = device; + } + data->ndevices++; + } + + cleanup: + virObjectUnlock(obj); + return 0; +} + + int virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListPtr devs, @@ -738,49 +981,31 @@ virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListFilter aclfilter, unsigned int flags) { - virNodeDevicePtr *tmp_devices = NULL; - virNodeDevicePtr device = NULL; - int ndevices = 0; - int ret = -1; - size_t i; + struct virNodeDeviceObjListExportData data = { + .conn = conn, .aclfilter = aclfilter, .flags = flags, + .devices = NULL, .ndevices = 0, .error = false }; + + virObjectLock(devs); + if (devices && + VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) { + virObjectUnlock(devs); + return -1; + } - if (devices && VIR_ALLOC_N(tmp_devices, devs->count + 1) < 0) - goto cleanup; + virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data); + virObjectUnlock(devs); - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virObjectLock(obj); - if ((!aclfilter || aclfilter(conn, obj->def)) && - virNodeDeviceMatch(obj, flags)) { - if (devices) { - if (!(device = virGetNodeDevice(conn, obj->def->name)) || - VIR_STRDUP(device->parent, obj->def->parent) < 0) { - virObjectUnref(device); - virObjectUnlock(obj); - goto cleanup; - } - tmp_devices[ndevices] = device; - } - ndevices++; - } - virObjectUnlock(obj); - } + if (data.error) + goto cleanup; - if (tmp_devices) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(tmp_devices, ndevices + 1)); - *devices = tmp_devices; - tmp_devices = NULL; - } + if (data.devices) { + ignore_value(VIR_REALLOC_N(data.devices, data.ndevices + 1)); + *devices = data.devices; + } - ret = ndevices; + return data.ndevices; cleanup: - if (tmp_devices) { - for (i = 0; i < ndevices; i++) - virObjectUnref(tmp_devices[i]); - } - - VIR_FREE(tmp_devices); - return ret; + virObjectListFree(data.devices); + return -1; } -- 2.9.4

@@ -39,13 +40,19 @@ struct _virNodeDeviceObj { };
struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; + virObjectLockable parent; + + /* name string -> virNodeDeviceObj mapping + * for O(1), lockless lookup-by-uuid */
I think you meant lookup-by-name ^here. More importantly though, I don't understand the comment at all, well I understand the words :), but the context is all noise - why should be the lookup lockless? You always lock the objlist prior to calling virHashLookup, followed by ref'ing and returning the result, I know we have that in other conf/vir*obj* modules and those don't make any more sense to me either.
+ virHashTable *objs; + };
static virClassPtr virNodeDeviceObjClass; +static virClassPtr virNodeDeviceObjListClass; static void virNodeDeviceObjDispose(void *opaque); +static void virNodeDeviceObjListDispose(void *opaque);
static int virNodeDeviceObjOnceInit(void) @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void) virNodeDeviceObjDispose))) return -1;
+ if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), + "virNodeDeviceObjList", + sizeof(virNodeDeviceObjList), + virNodeDeviceObjListDispose))) + return -1; + return 0; }
@@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) }
+static int +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def;
I recall having discussed ^this with you already, but this is one-time use only and I simply don't think it's necessary, I'd use helper variables for the sole purpose of getting rid of multiple levels of dereference either in a multi-use situation, or the number of levels dereferenced makes the line really long and the usage unreadable. (this also occurs on multiple places...no need to repeat this...)
+ const char *sysfs_path = opaque; + int want = 0; + + virObjectLock(obj); + def = obj->def; + if (STREQ_NULLABLE(def->sysfs_path, sysfs_path)) + want = 1; + virObjectUnlock(obj); + return want; +} + + virNodeDeviceObjPtr virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, const char *sysfs_path) { - size_t i; + virNodeDeviceObjPtr obj;
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback, + (void *)sysfs_path); + virObjectRef(obj); + virObjectUnlock(devs);
^This pattern occurs multiple times within the patch, I think it deserves a simple helper
+ if (obj) virObjectLock(obj); - def = obj->def; - if ((def->sysfs_path != NULL) && - (STREQ(def->sysfs_path, sysfs_path))) { - return virObjectRef(obj); - } - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + + +static virNodeDeviceObjPtr +virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs, + const char *name) +{ + return virObjectRef(virHashLookup(devs->objs, name)); }
@@ -238,20 +274,42 @@ virNodeDeviceObjPtr virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, const char *name) { - size_t i; - - for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virNodeDeviceObjPtr obj;
+ virObjectLock(devs); + obj = virNodeDeviceObjListFindByNameLocked(devs, name); + virObjectUnlock(devs); + if (obj) virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + + +struct virNodeDeviceObjListFindByWWNsData { + const char *parent_wwnn; + const char *parent_wwpn; +}; + +static int +virNodeDeviceObjListFindByWWNsCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + struct virNodeDeviceObjListFindByWWNsData *data = + (struct virNodeDeviceObjListFindByWWNsData *) opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && + STREQ_NULLABLE(cap->data.scsi_host.wwnn, data->parent_wwnn) && + STREQ_NULLABLE(cap->data.scsi_host.wwpn, data->parent_wwpn) && + virNodeDeviceFindVPORTCapDef(obj)) + want = 1; + virObjectUnlock(obj); + return want; }
@@ -260,22 +318,40 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, const char *parent_wwnn, const char *parent_wwpn) { - size_t i; + virNodeDeviceObjPtr obj; + struct virNodeDeviceObjListFindByWWNsData data = { + .parent_wwnn = parent_wwnn, .parent_wwpn = parent_wwpn };
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByWWNsCallback, + &data); + virObjectRef(obj); + virObjectUnlock(devs);
+ if (obj) virObjectLock(obj); - if ((cap = virNodeDeviceFindFCCapDef(obj)) && - STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && - STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) && - virNodeDeviceFindVPORTCapDef(obj)) - return virObjectRef(obj); - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + + +static int +virNodeDeviceObjListFindByFabricWWNCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + const char *matchstr = opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + if ((cap = virNodeDeviceFindFCCapDef(obj)) && + STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, matchstr) && + virNodeDeviceFindVPORTCapDef(obj)) + want = 1; + virObjectUnlock(obj); + return want; }
@@ -283,21 +359,35 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs, const char *parent_fabric_wwn) { - size_t i; + virNodeDeviceObjPtr obj;
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByFabricWWNCallback, + (void *)parent_fabric_wwn); + virObjectRef(obj); + virObjectUnlock(devs);
+ if (obj) virObjectLock(obj); - if ((cap = virNodeDeviceFindFCCapDef(obj)) && - STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) && - virNodeDeviceFindVPORTCapDef(obj)) - return virObjectRef(obj); - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + + +static int +virNodeDeviceObjListFindByCapCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + const char *matchstr = opaque; + int want = 0; + + virObjectLock(obj); + if (virNodeDeviceObjHasCap(obj, matchstr)) + want = 1; + virObjectUnlock(obj); + return want; }
@@ -305,18 +395,59 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs, const char *cap) { - size_t i; + virNodeDeviceObjPtr obj;
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByCapCallback, + (void *)cap); + virObjectRef(obj); + virObjectUnlock(devs);
+ if (obj) virObjectLock(obj); - if (virNodeDeviceObjHasCap(obj, cap)) - return virObjectRef(obj); - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + + +struct virNodeDeviceObjListFindSCSIHostByWWNsData { + const char *wwnn; + const char *wwpn; +}; + +static int +virNodeDeviceObjListFindSCSIHostByWWNsCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def; + struct virNodeDeviceObjListFindSCSIHostByWWNsData *data = + (struct virNodeDeviceObjListFindSCSIHostByWWNsData *) opaque; + virNodeDevCapsDefPtr cap; + int want = 0; + + virObjectLock(obj); + def = obj->def; + cap = def->caps; + + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { + virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); + if (cap->data.scsi_host.flags & + VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + if (STREQ(cap->data.scsi_host.wwnn, data->wwnn) && + STREQ(cap->data.scsi_host.wwpn, data->wwpn)) { + want = 1; + break; + } + } + } + cap = cap->next; + } + + virObjectUnlock(obj); + return want; }
@@ -325,31 +456,30 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, const char *wwnn, const char *wwpn) { - size_t i; + virNodeDeviceObjPtr obj; + struct virNodeDeviceObjListFindSCSIHostByWWNsData data = { + .wwnn = wwnn, .wwpn = wwpn };
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDevCapsDefPtr cap; + virObjectLock(devs); + obj = virHashSearch(devs->objs, + virNodeDeviceObjListFindSCSIHostByWWNsCallback, + &data); + virObjectRef(obj); + virObjectUnlock(devs);
+ if (obj) virObjectLock(obj); - cap = obj->def->caps; - - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) { - virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host); - if (cap->data.scsi_host.flags & - VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - if (STREQ(cap->data.scsi_host.wwnn, wwnn) && - STREQ(cap->data.scsi_host.wwpn, wwpn)) - return virObjectRef(obj); - } - } - cap = cap->next; - } - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + + +static void +virNodeDeviceObjListDispose(void *obj) +{ + virNodeDeviceObjListPtr devs = obj; + + virHashFree(devs->objs); }
@@ -358,8 +488,17 @@ virNodeDeviceObjListNew(void) { virNodeDeviceObjListPtr devs;
- if (VIR_ALLOC(devs) < 0) + if (virNodeDeviceObjInitialize() < 0) + return NULL; + + if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass))) return NULL; + + if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) { + virObjectUnref(devs); + return NULL; + } + return devs; }
@@ -367,11 +506,7 @@ virNodeDeviceObjListNew(void) void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs) { - size_t i; - for (i = 0; i < devs->count; i++) - virObjectUnref(devs->objs[i]); - VIR_FREE(devs->objs); - VIR_FREE(devs); + virObjectUnref(devs); }
@@ -381,22 +516,28 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, { virNodeDeviceObjPtr obj;
- if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) { + virObjectLock(devs); + + if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) { + virObjectLock(obj); virNodeDeviceDefFree(obj->def); obj->def = def; - return obj; - } + } else { + if (!(obj = virNodeDeviceObjNew())) + goto cleanup;
- if (!(obj = virNodeDeviceObjNew())) - return NULL; + if (virHashAddEntry(devs->objs, def->name, obj) < 0) { + virNodeDeviceObjEndAPI(&obj); + goto cleanup; + }
- if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) { - virNodeDeviceObjEndAPI(&obj); - return NULL; + obj->def = def; + virObjectRef(obj); } - obj->def = def;
- return virObjectRef(obj); + cleanup: + virObjectUnlock(devs); + return obj; }
@@ -404,21 +545,20 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - size_t i; - - virObjectUnlock(obj); + virNodeDeviceDefPtr def;
- for (i = 0; i < devs->count; i++) { - virObjectLock(devs->objs[i]); - if (devs->objs[i] == obj) { - virObjectUnlock(devs->objs[i]); - virObjectUnref(devs->objs[i]); + if (!obj) + return; + def = obj->def;
In this specific case, what's the benefit of having @def...
- VIR_DELETE_ELEMENT(devs->objs, i, devs->count); - break; - } - virObjectUnlock(devs->objs[i]); - } + virObjectRef(obj); + virObjectUnlock(obj); + virObjectLock(devs); + virObjectLock(obj); + virHashRemoveEntry(devs->objs, def->name);
...and use it here instead of obj->def->name... I'd prefer if you just dropped it here.
+ virObjectUnlock(obj); + virObjectUnref(obj); + virObjectUnlock(devs); }
@@ -643,25 +783,89 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, }
+struct virNodeDeviceCountData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + const char *matchstr; + int count; +};
These single purpose structures often have the same members across multiple callbacks, I was wondering whether we could just make one generic one, call it virNodeDeviceQueryData. But then, it would unnecessarily big, most fields would be unused, I don't know, it might not be a "nicer" solution eventually, so I'm ambivalent on this... Erik

On 07/20/2017 10:48 AM, Erik Skultety wrote:
@@ -39,13 +40,19 @@ struct _virNodeDeviceObj { };
struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; + virObjectLockable parent; + + /* name string -> virNodeDeviceObj mapping + * for O(1), lockless lookup-by-uuid */
I think you meant lookup-by-name ^here. More importantly though, I don't understand the comment at all, well I understand the words :), but the context is all noise - why should be the lookup lockless? You always lock the objlist prior to calling virHashLookup, followed by ref'ing and returning the result, I know we have that in other conf/vir*obj* modules and those don't make any more sense to me either.
hrmph... this showed up after I posted v6.... I'll provide my comments here... Sure I meant "by-name"... The comment was originally sourced from _virDomainObjList... I've always just copied it around ;-) The comment in/for domain objs list was added in commit id 'a3adcce79' when the code was still part of domain_conf.c I think the "decoding" is that prior to that one would have 0(n) mutex locks taken for each domain obj in the list as the list was being traversed. With hash tables one as 0(1) mutex locks taken to lock the list before get the entry out of the hash table.
+ virHashTable *objs; + };
static virClassPtr virNodeDeviceObjClass; +static virClassPtr virNodeDeviceObjListClass; static void virNodeDeviceObjDispose(void *opaque); +static void virNodeDeviceObjListDispose(void *opaque);
static int virNodeDeviceObjOnceInit(void) @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void) virNodeDeviceObjDispose))) return -1;
+ if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), + "virNodeDeviceObjList", + sizeof(virNodeDeviceObjList), + virNodeDeviceObjListDispose))) + return -1; + return 0; }
@@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj) }
+static int +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload; + virNodeDeviceDefPtr def;
I recall having discussed ^this with you already, but this is one-time use only and I simply don't think it's necessary, I'd use helper variables for the sole purpose of getting rid of multiple levels of dereference either in a multi-use situation, or the number of levels dereferenced makes the line really long and the usage unreadable. (this also occurs on multiple places...no need to repeat this...)
This avoids obj->def->sysfs_path below. While obj->def isn't being made part of the object by any of these changes, it could well be someday. I guess it's mostly a personal preference. The compiler will certainly optimize away my preferences.
+ const char *sysfs_path = opaque; + int want = 0; + + virObjectLock(obj); + def = obj->def; + if (STREQ_NULLABLE(def->sysfs_path, sysfs_path)) + want = 1; + virObjectUnlock(obj); + return want; +} + + virNodeDeviceObjPtr virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, const char *sysfs_path) { - size_t i; + virNodeDeviceObjPtr obj;
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback, + (void *)sysfs_path); + virObjectRef(obj); + virObjectUnlock(devs);
^This pattern occurs multiple times within the patch, I think it deserves a simple helper
Oh - I've been down that fork in the road before - create what I think is a "simple" helper combining things only to get shot down during review because it was deemed unnecessary or that each of these should have their own separate pair of API's Each one of these is a unique way to search the objs list for a piece of data by some value that is not a key... FindBySysfsPath FindByWWNs FindByFabricWWN FindByCap FindSCSIHostByWWNs What comes to mind to me as a simple helper would be to create a 'typed' structure and API's to use switch/case in order to make the comparison. Perhaps some future change could do that.
+ if (obj) virObjectLock(obj); - def = obj->def; - if ((def->sysfs_path != NULL) && - (STREQ(def->sysfs_path, sysfs_path))) { - return virObjectRef(obj); - } - virObjectUnlock(obj); - }
- return NULL; + return obj; +} + +
[...]
@@ -404,21 +545,20 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - size_t i; - - virObjectUnlock(obj); + virNodeDeviceDefPtr def;
- for (i = 0; i < devs->count; i++) { - virObjectLock(devs->objs[i]); - if (devs->objs[i] == obj) { - virObjectUnlock(devs->objs[i]); - virObjectUnref(devs->objs[i]); + if (!obj) + return; + def = obj->def;
In this specific case, what's the benefit of having @def...
- VIR_DELETE_ELEMENT(devs->objs, i, devs->count); - break; - } - virObjectUnlock(devs->objs[i]); - } + virObjectRef(obj); + virObjectUnlock(obj); + virObjectLock(devs); + virObjectLock(obj); + virHashRemoveEntry(devs->objs, def->name);
...and use it here instead of obj->def->name... I'd prefer if you just dropped it here.
As stated above, personal preference. I know I'm swimming against the stream of very review though so eventually my fingers will capitulate and I won't be so offended by seeing obj->def->name. Of course if obj->def ever becomes part of the @obj, then guess what - all this works by only changing the def = obj->def to an def = virObject*GetDef(obj);. Because *all* these changes were made when that was a reality in my branches - I haven't changed them. Now it's less of a reality, but maybe some time I or someone else will come up with a grand way to add @def to @obj. Short term IDC other than personal preference. Again, the compiler will optimize my preference away.
+ virObjectUnlock(obj); + virObjectUnref(obj); + virObjectUnlock(devs); }
@@ -643,25 +783,89 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, }
+struct virNodeDeviceCountData { + virConnectPtr conn; + virNodeDeviceObjListFilter aclfilter; + const char *matchstr; + int count; +};
These single purpose structures often have the same members across multiple callbacks, I was wondering whether we could just make one generic one, call it virNodeDeviceQueryData. But then, it would unnecessarily big, most fields would be unused, I don't know, it might not be a "nicer" solution eventually, so I'm ambivalent on this...
Erik
Well..... That's the direction the common virObject series took, but no one has taken the plunge yet to look at that (see patch 12/16 in the series). John

On Thu, Jul 20, 2017 at 04:32:46PM -0400, John Ferlan wrote:
On 07/20/2017 10:48 AM, Erik Skultety wrote:
@@ -39,13 +40,19 @@ struct _virNodeDeviceObj { };
struct _virNodeDeviceObjList { - size_t count; - virNodeDeviceObjPtr *objs; + virObjectLockable parent; + + /* name string -> virNodeDeviceObj mapping + * for O(1), lockless lookup-by-uuid */
I think you meant lookup-by-name ^here. More importantly though, I don't understand the comment at all, well I understand the words :), but the context is all noise - why should be the lookup lockless? You always lock the objlist prior to calling virHashLookup, followed by ref'ing and returning the result, I know we have that in other conf/vir*obj* modules and those don't make any more sense to me either.
hrmph... this showed up after I posted v6.... I'll provide my comments here...
Sure I meant "by-name"... The comment was originally sourced from _virDomainObjList... I've always just copied it around ;-)
Just don't forget to change it when pushing ;).
The comment in/for domain objs list was added in commit id 'a3adcce79' when the code was still part of domain_conf.c
I think the "decoding" is that prior to that one would have 0(n) mutex locks taken for each domain obj in the list as the list was being traversed. With hash tables one as 0(1) mutex locks taken to lock the list before get the entry out of the hash table.
Thanks for summary and pointing me to the commit, it makes sense now. ...
+ const char *sysfs_path = opaque; + int want = 0; + + virObjectLock(obj); + def = obj->def; + if (STREQ_NULLABLE(def->sysfs_path, sysfs_path)) + want = 1; + virObjectUnlock(obj); + return want; +} + + virNodeDeviceObjPtr virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs, const char *sysfs_path) { - size_t i; + virNodeDeviceObjPtr obj;
- for (i = 0; i < devs->count; i++) { - virNodeDeviceObjPtr obj = devs->objs[i]; - virNodeDeviceDefPtr def; + virObjectLock(devs); + obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback, + (void *)sysfs_path); + virObjectRef(obj); + virObjectUnlock(devs);
^This pattern occurs multiple times within the patch, I think it deserves a simple helper
Oh - I've been down that fork in the road before - create what I think is a "simple" helper combining things only to get shot down during review because it was deemed unnecessary or that each of these should have their own separate pair of API's
Each one of these is a unique way to search the objs list for a piece of data by some value that is not a key...
FindBySysfsPath FindByWWNs FindByFabricWWN FindByCap FindSCSIHostByWWNs
Yep, those are the ones I meant, see my reply to v6. Erik

Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 61 +++++++----------------------------- src/node_device/node_device_hal.c | 16 ++-------- src/node_device/node_device_udev.c | 13 +++----- 3 files changed, 18 insertions(+), 72 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 1f888fb..1e6709d 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -174,19 +174,13 @@ nodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) { - int ndevs = 0; - if (virNodeNumOfDevicesEnsureACL(conn) < 0) return -1; virCheckFlags(0, -1); - nodeDeviceLock(); - ndevs = virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, - virNodeNumOfDevicesCheckACL); - nodeDeviceUnlock(); - - return ndevs; + return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, + virNodeNumOfDevicesCheckACL); } @@ -197,20 +191,14 @@ nodeListDevices(virConnectPtr conn, int maxnames, unsigned int flags) { - int nnames; - if (virNodeListDevicesEnsureACL(conn) < 0) return -1; virCheckFlags(0, -1); - nodeDeviceLock(); - nnames = virNodeDeviceObjListGetNames(driver->devs, conn, - virNodeListDevicesCheckACL, - cap, names, maxnames); - nodeDeviceUnlock(); - - return nnames; + return virNodeDeviceObjListGetNames(driver->devs, conn, + virNodeListDevicesCheckACL, + cap, names, maxnames); } @@ -219,19 +207,14 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, unsigned int flags) { - int ret = -1; - virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1); if (virConnectListAllNodeDevicesEnsureACL(conn) < 0) return -1; - nodeDeviceLock(); - ret = virNodeDeviceObjListExport(conn, driver->devs, devices, - virConnectListAllNodeDevicesCheckACL, - flags); - nodeDeviceUnlock(); - return ret; + return virNodeDeviceObjListExport(conn, driver->devs, devices, + virConnectListAllNodeDevicesCheckACL, + flags); } @@ -240,11 +223,7 @@ nodeDeviceObjFindByName(const char *name) { virNodeDeviceObjPtr obj; - nodeDeviceLock(); - obj = virNodeDeviceObjListFindByName(driver->devs, name); - nodeDeviceUnlock(); - - if (!obj) { + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, name))) { virReportError(VIR_ERR_NO_NODE_DEVICE, _("no node device with matching name '%s'"), name); @@ -294,11 +273,8 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, virCheckFlags(0, NULL); - nodeDeviceLock(); - obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn); - nodeDeviceUnlock(); - - if (!obj) + if (!(obj = virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, + wwnn, wwpn))) return NULL; def = virNodeDeviceObjGetDef(obj); @@ -509,13 +485,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn, virNodeDevicePtr device = NULL; time_t start = 0, now = 0; - /* The thread that creates the device takes the driver lock, so we - * must release it in order to allow the device to be created. - * We're not doing anything with the driver pointer at this point, - * so it's safe to release it, assuming that the pointer itself - * doesn't become invalid. */ - nodeDeviceUnlock(); - nodeDeviceGetTime(&start); while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) { @@ -532,8 +501,6 @@ nodeDeviceFindNewDevice(virConnectPtr conn, break; } - nodeDeviceLock(); - return device; } @@ -552,8 +519,6 @@ nodeDeviceCreateXML(virConnectPtr conn, virCheckFlags(0, NULL); virt_type = virConnectGetType(conn); - nodeDeviceLock(); - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) goto cleanup; @@ -586,7 +551,6 @@ nodeDeviceCreateXML(virConnectPtr conn, "wwnn '%s' and wwpn '%s'"), def->name, wwnn, wwpn); cleanup: - nodeDeviceUnlock(); virNodeDeviceDefFree(def); VIR_FREE(wwnn); VIR_FREE(wwpn); @@ -612,8 +576,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj); - nodeDeviceLock(); - if (virNodeDeviceDestroyEnsureACL(device->conn, def) < 0) goto cleanup; @@ -647,7 +609,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; cleanup: - nodeDeviceUnlock(); virNodeDeviceObjEndAPI(&obj); VIR_FREE(name); VIR_FREE(parent); diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index b220798..8731e3b 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -508,20 +508,15 @@ dev_refresh(const char *udi) const char *name = hal_name(udi); virNodeDeviceObjPtr obj; - nodeDeviceLock(); if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) { /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ virNodeDeviceObjListRemove(driver->devs, obj); - } else { - VIR_DEBUG("no device named %s", name); - } - nodeDeviceUnlock(); - - if (obj) { virObjectUnref(obj); dev_create(udi); + } else { + VIR_DEBUG("no device named %s", name); } } @@ -541,14 +536,12 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, const char *name = hal_name(udi); virNodeDeviceObjPtr obj; - nodeDeviceLock(); obj = virNodeDeviceObjListFindByName(driver->devs, name); VIR_DEBUG("%s", name); if (obj) virNodeDeviceObjListRemove(driver->devs, obj); else VIR_DEBUG("no device named %s", name); - nodeDeviceUnlock(); virObjectUnref(obj); } @@ -561,11 +554,8 @@ device_cap_added(LibHalContext *ctx, virNodeDeviceObjPtr obj; virNodeDeviceDefPtr def; - nodeDeviceLock(); - obj = virNodeDeviceObjListFindByName(driver->devs, name); - nodeDeviceUnlock(); VIR_DEBUG("%s %s", cap, name); - if (obj) { + if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) { def = virNodeDeviceObjGetDef(obj); (void)gather_capability(ctx, udi, cap, &def->caps); virNodeDeviceObjEndAPI(&obj); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1b10c16..434efd7 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1607,7 +1607,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, const char *action = NULL; int udev_fd = -1; - nodeDeviceLock(); udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1639,7 +1638,6 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, cleanup: udev_device_unref(device); - nodeDeviceUnlock(); return; } @@ -1767,7 +1765,6 @@ nodeStateInitialize(bool privileged, { udevPrivate *priv = NULL; struct udev *udev = NULL; - int ret = -1; if (VIR_ALLOC(priv) < 0) return -1; @@ -1847,18 +1844,16 @@ nodeStateInitialize(bool privileged, goto cleanup; /* Populate with known devices */ - + nodeDeviceUnlock(); if (udevEnumerateDevices(udev) != 0) goto cleanup; - ret = 0; + return 0; cleanup: nodeDeviceUnlock(); - - if (ret == -1) - nodeStateCleanup(); - return ret; + nodeStateCleanup(); + return -1; } -- 2.9.4
participants (2)
-
Erik Skultety
-
John Ferlan