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

[Erik Skultety]

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

What do you mean? Don't you see squash.patch in the attachment? (yes, I
attached a patch instead of inlining it, the reason for that being that the
patch is not particularly small and inlining it would disrupt the thread...)

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

Yeah, ok, since I'd rather not review multiple series that more-or-less depend
on each other in parallel and try to connect relating changes back and forth, I
couldn't have noticed this fact, but looking at the patch you linked, sure, the
approach is the same. As long as the change we're discussing makes it in
(one way or the other), I'm fine with it.

>
> Ironically though you didn't like the @def usage, still you chose
> virHashSearcher cb = $functionName which has one use to be used as an

Alright, fair point, any further discussion would be unnecessary, act like I
didn't say anything.

My ACK still stands.

Erik