Re: [libvirt] [PATCH v6 3/4] nodedev: Convert virNodeDeviceObjListPtr to use hash tables
On 07/24/2017 09:08 AM, Erik Skultety wrote:
>>> 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...)
Oh - I meant when I reply to your patch with an attachment, I don't get
your attachment in the reply. So I guess it was a early morning and
feeble attempt to note that one had to go look at your reply in order to
get context!
I've altered the two @def in the callback functions to be
"obj->def->sysfs_path" and "obj->def->caps". I've
also made the single
virHashSearch API/helper....
I also changed the comment in my branch for this patch (from v5 review)
to say "lookup-by-name" instead of "lookup-by-uuid" and modified the
comment in .4 to be appropriately placed.
Just running things through a couple of tests before doing the push...
Thanks -
John
>
> 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