* [GIT PULL] fanotify cleanup for v5.4-rc1 @ 2019-09-20 11:00 Jan Kara 2019-09-21 21:10 ` Linus Torvalds 0 siblings, 1 reply; 3+ messages in thread From: Jan Kara @ 2019-09-20 11:00 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel Hello Linus, could you please pull from git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify_for_v5.4-rc1 Top of the tree is ba38e358907e. The full shortlog is: zhengbin (1): fanotify: remove always false comparison in copy_fid_to_user The diffstat is fs/notify/fanotify/fanotify_user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Thanks Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PULL] fanotify cleanup for v5.4-rc1 2019-09-20 11:00 [GIT PULL] fanotify cleanup for v5.4-rc1 Jan Kara @ 2019-09-21 21:10 ` Linus Torvalds 2019-09-23 9:42 ` Jan Kara 0 siblings, 1 reply; 3+ messages in thread From: Linus Torvalds @ 2019-09-21 21:10 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel On Fri, Sep 20, 2019 at 4:00 AM Jan Kara <jack@suse.cz> wrote: > > could you please pull from Pulled and then unpulled. This is a prime example of a "cleanup" that should never ever be done, and a compiler warning that is a disgrace and shouldn't happen. This code: WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); is obvious and makes sense. It clearly and unambiguously checks that 'len' is in the specified range. In contrast, this code: WARN_ON_ONCE(len >= FANOTIFY_EVENT_ALIGN); quite naturally will make a human wonder "what about negative values". Yes, it turns out that "len" is unsigned. That isn't actually immediately obvious to a human, since the declaration of 'len' is 20+ lines earlier (and even then the type doesn't say "unsigned", although a lot of people do recognize "size_t" as such). In fact, maybe some day the type will change, and the careful range checking means that the code continues to work correctly. The fact that "len" is unsigned _is_ obvious to the compiler, which just means that now that compiler can ignore the "< 0" thing and optimize it away. Great. But that doesn't make the compiler warning valid, and it doesn't make the patch any better. When it comes to actual code quality, the version that checks against zero is the better version. Please stop using -Wtype-limits with compilers that are too stupid to understand that range checking with the type range is sane. Compilers that think that warning for the above kind of thing is ok are inexcusable garbage. And compiler writers who think that the warning is a good thing can't see the forest for the trees. They are too hung up on a detail to see the big picture. Why/how was this found in the first place? We don't enable type-limit checking by default. We may have to add an explicit ccflags-y += $(call cc-disable-warning, type-limits) if these kinds of patches continue to happen, which would be sad. There are _valid_ type limits. But this kind of range-based check is not a valid thing to warn about, and we shouldn't make the kernel source code worse just because the compiler is doing garbage things. Linus ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PULL] fanotify cleanup for v5.4-rc1 2019-09-21 21:10 ` Linus Torvalds @ 2019-09-23 9:42 ` Jan Kara 0 siblings, 0 replies; 3+ messages in thread From: Jan Kara @ 2019-09-23 9:42 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel, zhengbin, Matthew Wilcox Quoting full email for Matthew and Zhengbin to have context. On Sat 21-09-19 14:10:52, Linus Torvalds wrote: > On Fri, Sep 20, 2019 at 4:00 AM Jan Kara <jack@suse.cz> wrote: > > > > could you please pull from > > Pulled and then unpulled. > > This is a prime example of a "cleanup" that should never ever be done, > and a compiler warning that is a disgrace and shouldn't happen. > > This code: > > WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > > is obvious and makes sense. It clearly and unambiguously checks that > 'len' is in the specified range. > > In contrast, this code: > > WARN_ON_ONCE(len >= FANOTIFY_EVENT_ALIGN); > > quite naturally will make a human wonder "what about negative values". > > Yes, it turns out that "len" is unsigned. That isn't actually > immediately obvious to a human, since the declaration of 'len' is 20+ > lines earlier (and even then the type doesn't say "unsigned", although > a lot of people do recognize "size_t" as such). > > In fact, maybe some day the type will change, and the careful range > checking means that the code continues to work correctly. Yeah, I was also a bit undecided about this patch because the check with "len < 0" seems more obvious. But then decided to take it because we have a very similar WARN_ON_ONCE() at the beginning of the function (copy_fid_to_user()) making sure "len" is large enough. But seeing your arguments I'll just drop the patch. Thanks for review! > The fact that "len" is unsigned _is_ obvious to the compiler, which > just means that now that compiler can ignore the "< 0" thing and > optimize it away. Great. > > But that doesn't make the compiler warning valid, and it doesn't make > the patch any better. > > When it comes to actual code quality, the version that checks against > zero is the better version. > > Please stop using -Wtype-limits with compilers that are too stupid to > understand that range checking with the type range is sane. > > Compilers that think that warning for the above kind of thing is ok > are inexcusable garbage. > > And compiler writers who think that the warning is a good thing can't > see the forest for the trees. They are too hung up on a detail to see > the big picture. > > Why/how was this found in the first place? We don't enable type-limit > checking by default. The report has come from a CI system run at Huawei. Not sure what exactly they run there. > We may have to add an explicit > > ccflags-y += $(call cc-disable-warning, type-limits) > > if these kinds of patches continue to happen, which would be sad. > There are _valid_ type limits. > > But this kind of range-based check is not a valid thing to warn about, > and we shouldn't make the kernel source code worse just because the > compiler is doing garbage things. > > Linus Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-23 9:42 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-20 11:00 [GIT PULL] fanotify cleanup for v5.4-rc1 Jan Kara 2019-09-21 21:10 ` Linus Torvalds 2019-09-23 9:42 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).