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
