Re: CVS commit: src/sys/dev/wscons

[Greg Troxel <gdt%lexort.com@localhost>]

Hans Rosenfeld <hans%netbsd.org@localhost> writes:

> I fully agree with you on what DIAGNOSTIC should do, and what it shouldn't.
> In particular, if there's a check for an unexpected situation that's
> only there in DIAGNOSTIC kernels, it shouldn't handle these situations
> gracefully where the non-DIAGNOSTIC code path would just do something
> odd, most likely leading to a panic later on. In those cases, using a
> KASSERT would be appropriate to catch the kernel bug.

KASSERT is an assert with DIAGNOSTIC, and a no-op without DIAGNOSTIC.
For things that are truly checking an invariant that must never fail,
that's appropriate.

If there is code to check for things that are some flavor of unexpected,
and handle them, and there isn't a good argument that those things
basically never happen and if they do there is a code bug, then that coe
should just not have anything to do with DIAGNOSTIC.


> That being said, while debugging kern/59206 I found plenty of these
> DIAGNOSTIC-only checks that handled error situations gracefully, which
> indicates that this isn't indicative of a kernel bug at all. But at the
> same time it would effectively hide the real cause of issues like
> kern/59206 when it's most inappropriate to do so, that is when running
> DIAGNOSTIC trying to debug it.
>
> Given that for all of these cases it's apparently possible to handle
> these situations gracefully, that should always be done, and for
> DIAGNOSTIC code it should be logged as it was before. There was already
> a few similar constructs in wsmux.c, so it's just consistent now and
> probably less prone to unexpected panicking. 

I think it's great to move most of such code out of DIAGNOSTIC.  I
didn't mean that you should not have moved the check/fix out.  I am only
complaining that you said you left the print part conditional on
DIAGNOSTIC.

> If you think some of those checks should actually be KASSERTs and cause
> immediate panics on DIAGNOSTIC kernels because they actually indicate
> kernel bugs, please point them out and I'll be happy to change them.

I didn't mean that.  I meant that you should put them under DIAGNOSTIC
as asserts (or just KASSERT) if *you* believe that they can never
happen.  You agree with that concept, and you don't see this checks as
being invariant/panic checks.  Which is all fine.

What I object to is the third point in this concept:

  check for something that is not expected, probably shouldn't have
  happened, but which we can't claim is a kernel bug

  fix up the situation so we can continue, because this is not fatal

  if DIAGNOSTIC, log it.  othewise be quiet

This is abuse of DIAGNOSTIC which is supposed to be only about choossing
between

  check invariant and panic if it fails

and

  don't check invariant

and not about anything else, for any other purpose.  This doctrine goes
back to 2.8BSD at least, or maybe my memory is off and it was 2.9.

All in all I am staying that instead of only printing about such fixups
if DIAGNOSTIC is enabled, instead only print if WSDEBUG is enabled.  Or
if WSDEBUG is enabled at compile time and the global wsdebug is
non-zero, as some systems do.  Separately from DIAGNOSTIC doctrine,
people might want DIAGNOSTIC without these messages, and they might want
WSDEBUG without DIAGNOSTIC.  There are all sorts of subsystems that have
optional printfs for various things, and they more or less all have
their own FOODEBUG flags.