| Message ID | 20220920122302.99195-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | coding-style.rst: document BUG() and WARN() rules | expand |
On 9/20/22 05:23, David Hildenbrand wrote: > checkpatch does not point out that VM_BUG_ON() and friends should be > avoided, however, Linus notes: > > VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally > no different, the only difference is "we can make the code smaller > because these are less important". [1] > > So let's warn on VM_BUG_ON() and other BUG variants as well. While at it, > make it clearer that the kernel really shouldn't be crashed. > > As there are some subsystem BUG macros that actually don't end up crashing > the kernel -- for example, KVM_BUG_ON() -- exclude these manually. > > [1] https://lore.kernel.org/r/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@mail.gmail.com > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > scripts/checkpatch.pl | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 79e759aac543..21f3a79aa46f 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -4695,12 +4695,12 @@ sub process { > } > } > > -# avoid BUG() or BUG_ON() > - if ($line =~ /\b(?:BUG|BUG_ON)\b/) { > +# do not use BUG() or variants > + if ($line =~ /\b(?!AA_|BUILD_|DCCP_|IDA_|KVM_|RWLOCK_|snd_|SPIN_)(?:[a-zA-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/) { Should this be a separate patch? Adding a bunch of exceptions to the BUG() rules is a separate and distinct thing from adding VM_BUG_ON() and other *BUG*() variants to the mix. > my $msg_level = \&WARN; > $msg_level = \&CHK if ($file); > &{$msg_level}("AVOID_BUG", > - "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr); > + "Do not crash the kernel unless it is unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than BUG() or variants.\n" . $herecurr); Here's a requested tweak, to clean up the output and fix punctuation: "Avoid crashing the kernel--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants.\n" . $herecurr); thanks,
On Thu, 2022-09-22 at 19:05 -0700, John Hubbard wrote: > On 9/20/22 05:23, David Hildenbrand wrote: > > checkpatch does not point out that VM_BUG_ON() and friends should be > > avoided, however, Linus notes: > > > > VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally > > no different, the only difference is "we can make the code smaller > > because these are less important". [1] > > > > So let's warn on VM_BUG_ON() and other BUG variants as well. While at it, > > make it clearer that the kernel really shouldn't be crashed. > > > > As there are some subsystem BUG macros that actually don't end up crashing > > the kernel -- for example, KVM_BUG_ON() -- exclude these manually. > > > > [1] https://lore.kernel.org/r/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@mail.gmail.com [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > > @@ -4695,12 +4695,12 @@ sub process { > > } > > } > > > > -# avoid BUG() or BUG_ON() > > - if ($line =~ /\b(?:BUG|BUG_ON)\b/) { > > +# do not use BUG() or variants > > + if ($line =~ /\b(?!AA_|BUILD_|DCCP_|IDA_|KVM_|RWLOCK_|snd_|SPIN_)(?:[a-zA-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/) { > > Should this be a separate patch? Adding a bunch of exceptions to the BUG() rules is > a separate and distinct thing from adding VM_BUG_ON() and other *BUG*() variants to > the mix. Not in my opinion. > > my $msg_level = \&WARN; > > $msg_level = \&CHK if ($file); > > &{$msg_level}("AVOID_BUG", > > - "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr); > > + "Do not crash the kernel unless it is unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than BUG() or variants.\n" . $herecurr); > > Here's a requested tweak, to clean up the output and fix punctuation: > > "Avoid crashing the kernel--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants.\n" . $herecurr); Fixing punctuation here would be removing the trailing period as checkpatch only has periods for multi-sentence output messages. And I think that "Do not crash" is a stronger statement than "Avoid crashing" so I prefer the original suggestion but it's not a big deal either way.
On 9/22/22 19:11, Joe Perches wrote: >> Should this be a separate patch? Adding a bunch of exceptions to the BUG() rules is >> a separate and distinct thing from adding VM_BUG_ON() and other *BUG*() variants to >> the mix. > > Not in my opinion. OK, then. :) > >>> my $msg_level = \&WARN; >>> $msg_level = \&CHK if ($file); >>> &{$msg_level}("AVOID_BUG", >>> - "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr); >>> + "Do not crash the kernel unless it is unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than BUG() or variants.\n" . $herecurr); >> >> Here's a requested tweak, to clean up the output and fix punctuation: >> >> "Avoid crashing the kernel--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants.\n" . $herecurr); > > Fixing punctuation here would be removing the trailing period as checkpatch > only has periods for multi-sentence output messages. OK, let's do that too. > > And I think that "Do not crash" is a stronger statement than "Avoid crashing" > so I prefer the original suggestion but it's not a big deal either way. Yes, stronger wording is better. So how about this: "Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants\n" . $herecurr); thanks,
>> And I think that "Do not crash" is a stronger statement than "Avoid crashing" >> so I prefer the original suggestion but it's not a big deal either way. > > Yes, stronger wording is better. So how about this: > > "Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants\n" . $herecurr); Okay, let's use that. Thanks!
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 79e759aac543..21f3a79aa46f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4695,12 +4695,12 @@ sub process { } } -# avoid BUG() or BUG_ON() - if ($line =~ /\b(?:BUG|BUG_ON)\b/) { +# do not use BUG() or variants + if ($line =~ /\b(?!AA_|BUILD_|DCCP_|IDA_|KVM_|RWLOCK_|snd_|SPIN_)(?:[a-zA-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/) { my $msg_level = \&WARN; $msg_level = \&CHK if ($file); &{$msg_level}("AVOID_BUG", - "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr); + "Do not crash the kernel unless it is unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than BUG() or variants.\n" . $herecurr); } # avoid LINUX_VERSION_CODE
checkpatch does not point out that VM_BUG_ON() and friends should be avoided, however, Linus notes: VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally no different, the only difference is "we can make the code smaller because these are less important". [1] So let's warn on VM_BUG_ON() and other BUG variants as well. While at it, make it clearer that the kernel really shouldn't be crashed. As there are some subsystem BUG macros that actually don't end up crashing the kernel -- for example, KVM_BUG_ON() -- exclude these manually. [1] https://lore.kernel.org/r/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@mail.gmail.com Signed-off-by: David Hildenbrand <david@redhat.com> --- scripts/checkpatch.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)