On 07/24/2017 03:52 AM, Erik Skultety wrote:
> On Thu, Jul 20, 2017 at 10:08:14AM -0400, John Ferlan wrote:
>> 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@xxxxxxxxxx>
>> ---
>> 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 035a56d..58481e7 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;
>
> As we discussed in v5, I'd like you to drop ^these @def (and only @def...)
> declarations and usages across the whole patch for the reasons I already
> mentioned - it makes sense in general, but it's not very useful in this
> specific case.
>
<sigh> OK, I really do dislike the obj->def->X syntax "just" for a few
functions while others use the @def nomenclature.
>> + const char *sysfs_path = opaque;
>> + int want = 0;
>> +
>> + virObjectLock(obj);
>> + def = obj->def;
>
> ...
>
>> 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, NULL);
>> + 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;
>
> With reference to v5:
> The fact that creating a helper wasn't met with an agreement from the
> reviewer's side in one case doesn't mean it doesn't make sense to do in an
> other case, like this one. So, what I actually meant by creating a helper for:
>
> BySysfsPath
> ByWWNs
> ByFabricWWN
> ByCap
> ByCapSCSIByWWNs
>
> is just simply move the lock and search logic to a separate function, that's
> all, see my attached patch. And then, as you suggested in your v5 response to
> this patch, we can move from here (my patch included) and potentially do some
> more refactoring.
Replies don't include your patch; however, I will note if you jump to
patch 13:
https://www.redhat.com/archives/libvir-list/2017-June/msg00929.html
of the virObject series I posted last month:
https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
that you'll see that is the direction this would be headed anyway. I can
do this sooner that's fine, although I prefer the name
virNodeDeviceObjListSearch as opposed to virNodeDeviceObjListFindHelper.
Ironically though you didn't like the @def usage, still you chose
virHashSearcher cb = $functionName which has one use to be used as an
argument to the function even though $functionName could be used
directly in the call. The only advantage of @cb is that it reduces the
line length of the call.
John
>
> ACK if you move the lock-search-ref-unlock-return snippets to a separate
> function for the cases mentioned above.
>
> Erik
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
