(moved to tech-kern, becuase source-changes-d is too buried and this is only happening there because it's post-commit discussion rather than pre-commit review) Michael van Elst <mlelstv%serpens.de@localhost> writes: > On Tue, May 05, 2015 at 07:47:09PM -0400, Greg Troxel wrote: >> >> "Michael van Elst" <mlelstv%netbsd.org@localhost> writes: >> >> > Modified Files: >> > src/sys/dev: dksubr.c >> > >> > Log Message: >> > warn about labels only when built with DIAGNOSTIC >> >> This feels like an abuse of DIAGNOSTIC, which is supposed to be about >> asserts for detecting internal inconsistencies, not changing the >> behavior of how the kernel responds to external inputs. >> >> I don't have any problem with warnings for dodgy external input being >> generally enabled, or having a separate verbose option. I can't see >> any reason not to have them, as it's not like new labels appearing is a >> high-rate event that would dos the log. > > The warnings are restricted to diagnostic kernels only because they > did fill the log on some systems, i.e. those where we provide images > for SD cards of unspecified size. Every device-open (for a device that > happens to already use the dksubr routines) prints the warning, > not just every newly detected bad label. (Separately from this, it may be that we need a general rate-limited logging facility, because logging on objectionable external input always runs into this.) > Saying that, some device drivers have private code that does the > same, some don't or do it in a different way. This excercise is > more about making things more consistent. When we have a method > to keep the noise down to a sane level, it's no problem to enable > the warnings again in a default kernel. I see the point that it's not ok to have them enabled by default - I didn't realize it wasn't just on attachment. But that really means it's not ok in DIAGNOSTIC. Since 2BSD days, the point of DIAGNOSTIC has been solely about adding checks for invariant violations and panicing if they are detected (on the theory that the kernel is so messed up it's better to stop and provide a core dump for debugging). Non-DIAGNOSTIC was abouts saving the cycles for the checks. I run DIAGONSTIC normally, and I think a lot of other people do too. As I understand our release policy, it should be on in kernels on current snapshots on constrained systems, including the SD-sized RPI images, etc. So this is a long way of saying that if there is unsafely verbose logging, it should be under some DKVERBOSE, UNSAFE_VERBOSE, VERBOSE_LOGGING or some other name, not conflated with DIAGNOSTIC. If there's precedent, that's a bug and should be fixed too.
Attachment:
pgpi9wCc1U4iI.pgp
Description: PGP signature