Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Ingo Molnar]

* Richard Weinberger <richard.weinberger@xxxxxxxxx> wrote:

> On Wed, Apr 30, 2014 at 5:03 PM, Paul Gortmaker
> <paul.gortmaker@xxxxxxxxxxxxx> wrote:
> > A long standing problem for us has been the misuse of BUG/BUG_ON.
> > The typical misuse is someone only thinking of what represents
> > a bug in their local code, and especially for people relatively
> > new to Linux, starting out in device drivers, the appeal of using
> > BUG w/o knowing what it really does is too great.
> >
> > So you end up with some trivial non system critical driver bringing
> > the whole system to a grinding halt just because it detected an
> > internal inconsistency.  That just makes users unhappy and looks bad.
> >
> > It is hopeless to think we can reclaim BUG/BUG_ON for their original
> > intent, given there are currently ~20k instances.  To make progress
> > here, we create BUG_AND_HALT variants, which leave no doubt as to
> > what they do in name alone.
> >
> > Then we can incrementally move the real BUG users (unrecoverable
> > filesystem corruption, page table mangling, etc) onto BUG_AND_HALT,
> > and finally at some time in the future we'll simply make the old
> > BUG/BUG_ON be aliases for WARN/WARN_ON, once we've moved over the
> > bulk of the instances really needing to halt the system.
> >
> > Suggested-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>
> > ---
> >
> > [This might not be a unique idea; but I'm pretty sure I'd first
> > heard of it during a discussion with Ingo at RT summit last year.]
> >
> >  include/asm-generic/bug.h | 16 ++++++++++++++++
> >  scripts/checkpatch.pl     |  7 +++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index 630dd2372238..57b79a394ceb 100644
> > --- a/include/asm-generic/bug.h
> > +++ b/include/asm-generic/bug.h
> > @@ -43,6 +43,14 @@ struct bug_entry {
> >   * If you're tempted to BUG(), think again:  is completely giving up
> >   * really the *only* solution?  There are usually better options, where
> >   * users don't need to reboot ASAP and can mostly shut down cleanly.
> > + *
> > + * Sadly nobody listens to the above, and trying to reclaim BUG/BUG_ON
> > + * for their original intent is about as hopeful as wishing "selfie"
> > + * wasn't headed for the OED.  So the plan is to avoid BUG/BUG_ON
> > + * entirely.  Either use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON.
> > + * Once the critical (e.g. fs etc) BUG/BUG_ON users are updated to use
> > + * the clearly named HALT variants, we can point the old BUG/BUG_ON
> > + * defines below to be clones of the less drastic WARN variants.
> >   */
> >  #ifndef HAVE_ARCH_BUG
> >  #define BUG() do { \
> > @@ -51,10 +59,18 @@ struct bug_entry {
> >  } while (0)
> >  #endif
> >
> > +#ifndef HAVE_ARCH_BUG_AND_HALT
> > +#define BUG_AND_HALT BUG
> > +#endif
> > +
> >  #ifndef HAVE_ARCH_BUG_ON
> >  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
> >  #endif
> >
> > +#ifndef HAVE_ARCH_BUG_AND_HALT_ON
> > +#define BUG_AND_HALT_ON BUG_ON
> > +#endif
> > +
> >  /*
> >   * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
> >   * significant issues that need prompt attention if they should ever
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 34eb2160489d..3cbf3591cf76 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2010,6 +2010,13 @@ sub process {
> >                         $rpt_cleaners = 1;
> >                 }
> >
> > +# Dont use BUG/BUG_ON; use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON
> > +               if ($rawline =~ /^\+.*BUG\(/ || $rawline =~ /^\+.*BUG_ON\(/) {
> > +                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> > +                       WARN("BUG/BUG_ON",
> > +                            "Use of BUG/BUG_ON is deprecated. Use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON\n" . $herevet);
> > +               }
> > +
> >  # Check for FSF mailing addresses.
> >                 if ($rawline =~ /\bwrite to the Free/i ||
> >                     $rawline =~ /\b59\s+Temple\s+Pl/i ||
> > --
> 
> I like the idea but not the name.
> What about DIE() and DIE_ON()?

CRASH_ON() might be a suggestive name as well, as from the user's 
point of view we are crashing her system.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/