[Drbd-dev] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Christian König]

Am 28.02.22 um 22:13 schrieb James Bottomley:
> On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote:
>> Am 28.02.22 um 21:42 schrieb James Bottomley:
>>> On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
>>>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>>>>> On Mon, Feb 28, 2022 at 4:19 AM Christian König
>>>>> <christian.koenig at amd.com> wrote:
>>>>> [SNIP]
>>>>> Anybody have any ideas?
>>>> I think we should look at the use cases why code is touching
>>>> (pos)
>>>> after the loop.
>>>>
>>>> Just from skimming over the patches to change this and experience
>>>> with the drivers/subsystems I help to maintain I think the
>>>> primary pattern looks something like this:
>>>>
>>>> list_for_each_entry(entry, head, member) {
>>>>        if (some_condition_checking(entry))
>>>>            break;
>>>> }
>>>> do_something_with(entry);
>>> Actually, we usually have a check to see if the loop found
>>> anything, but in that case it should something like
>>>
>>> if (list_entry_is_head(entry, head, member)) {
>>>       return with error;
>>> }
>>> do_somethin_with(entry);
>>>
>>> Suffice?  The list_entry_is_head() macro is designed to cope with
>>> the bogus entry on head problem.
>> That will work and is also what people already do.
>>
>> The key problem is that we let people do the same thing over and
>> over again with slightly different implementations.
>>
>> Out in the wild I've seen at least using a separate variable, using
>> a bool to indicate that something was found and just assuming that
>> the list has an entry.
>>
>> The last case is bogus and basically what can break badly.
> Yes, I understand that.  I'm saying we should replace that bogus checks
> of entry->something against some_value loop termination condition with
> the list_entry_is_head() macro.  That should be a one line and fairly
> mechanical change rather than the explosion of code changes we seem to
> have in the patch series.

Yes, exactly that's my thinking as well.

Christian.

>
> James
>
>