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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> @@ -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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.