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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux