Thread (85 messages) 85 messages, 21 authors, 2022-03-07

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

From: Greg KH <hidden>
Date: 2022-03-01 17:58:13
Also in: alsa-devel, amd-gfx, dmaengine, dri-devel, intel-gfx, intel-wired-lan, kvm, linux-arch, linux-aspeed, linux-block, linux-cifs, linux-crypto, linux-f2fs-devel, linux-fsdevel, linux-iio, linux-media, linux-mediatek, linux-pm, linux-rdma, linux-scsi, linux-staging, linux-tegra, linux-usb, linux-wireless, linuxppc-dev, lkml, nouveau

On Tue, Mar 01, 2022 at 06:40:04PM +0100, Jakob Koschel wrote:
quoted
On 1. Mar 2022, at 18:36, Greg KH [off-list ref] wrote:

On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
quoted
quoted
On 1. Mar 2022, at 01:41, Linus Torvalds [off-list ref] wrote:

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel [off-list ref] wrote:
quoted
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.
quoted
Changing the list_for_each_entry() macro first will break all of those cases
(e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing
in total. I'll now finish all of them and then split them by
submodules as Greg suggested and repost a patch set per submodule.
Sounds good?
Sounds good to me!

If you need help carving these up and maintaining them over time as
different subsystem maintainers accept/ignore them, just let me know.
Doing large patchsets like this can be tough without a lot of
experience.
Very much appreciated!

There will probably be some cases that do not match one of the pattern
we already discussed and need separate attention.

I was planning to start with one subsystem and adjust the coming ones
according to the feedback gather there instead of posting all of them
in one go.
That seems wise.  Feel free to use USB as a testing ground for this if
you want to :)

thanks,

greg k-h
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help