Re: [libvirt] [PATCH v6 3/4] nodedev: Convert virNodeDeviceObjListPtr to use hash tables

>> 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