> @@ -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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
