On Mon 20-05-24 12:46:44, Linus Torvalds wrote: > On Mon, 20 May 2024 at 04:22, Jan Kara <jack@xxxxxxx> wrote: > > > > * A few small cleanups > > This IS NOT A CLEANUP! It's a huge mistake: > > > Nikita Kiryushin (1): > > fanotify: remove unneeded sub-zero check for unsigned value > > The old code did > > WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > > and that is very legible and very understandable to humans. > > The new code is > > WARN_ON_ONCE(len >= FANOTIFY_EVENT_ALIGN); > > and now a human that reads that line needs to go back and check what > the type of 'len' is to notice that it's unsigned. It is not at all > clear from the context, and the declaration of 'len' is literally 80 > lines up. Not very close at all, in other words. So I was hesitating whether to take this or not because I liked the len < 0 check as well. Then I've convinced myself that the impression this check gives that "if we miscomputed and used more than we should, the WARN_ON_ONCE() would trigger" is incorrect because of underflow so better delete it. But now that I've written it down and looked again that impression is actually correct because the len >= FANOTIFY_EVENT_ALIGN check will catch that situation instead. Long story short, I agree with your revert. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
