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

[Hans Rosenfeld <hans%netbsd.org@localhost>]

Hi Greg,

On Sun, Mar 23, 2025 at 08:31:14AM -0400, Greg Troxel wrote:
> "Hans Rosenfeld" <hans%netbsd.org@localhost> writes:
> 
> > Module Name:	src
> > Committed By:	hans
> > Date:		Sun Mar 23 12:19:32 UTC 2025
> >
> > Modified Files:
> > 	src/sys/dev/wscons: wsmouse.c
> >
> > Log Message:
> > wsmouse(4): fix bogus DIAGNOSTIC checks
> >
> > Similar to wskbd(4), these checks should be done always, and the only
> > thing DIAGNOSTIC about them should be the printing of the message.
> 
> This doesn't sound quite right.
> 
> DIAGNOSTIC is supposed to be about checking invariants and panicing.
> Because the invariants are always true (hah), this should be a no-op and
> is omitted in production.
> 
> If the code is checking something that is input or somehow could be
> invalid data *without a kernel bug*, then I agree it should always
> happen.  But noting it (as a kernel printf or whatever) should not be
> conditioned on DIAGNOSTIC.   Probably instead some WSDEBUG, like we have
> USBDEBUG etc.

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.

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. 

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.


Hans



-- 
%SYSTEM-F-ANARCHISM, The operating system has been overthrown