From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Bottomley Date: Mon, 28 Feb 2022 16:13:09 -0500 Subject: [Intel-wired-lan] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr In-Reply-To: <0b65541a-3da7-dc35-690a-0ada75b0adae@amd.com> References: <20220228110822.491923-1-jakobkoschel@gmail.com> <20220228110822.491923-3-jakobkoschel@gmail.com> <2e4e95d6-f6c9-a188-e1cd-b1eae465562a@amd.com> <282f0f8d-f491-26fc-6ae0-604b367a5a1a@amd.com> <0b65541a-3da7-dc35-690a-0ada75b0adae@amd.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: 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 > > > > 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. James