Date: Wed, 12 Dec 2012 16:41:45 -0800 From: Adrian Chadd <adrian@freebsd.org> To: Navdeep Parhar <np@freebsd.org> Cc: Alfred Perlstein <alfred@freebsd.org>, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Alfred Perlstein <bright@mu.org>, Andriy Gapon <avg@freebsd.org>, src-committers@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r244112 - head/sys/kern Message-ID: <CAJ-VmomAj9Yu=zrF2uKnAUSt4DDxNpNuyCUUE%2Bg8j4E3=WhP=g@mail.gmail.com> In-Reply-To: <50C9206D.6080502@FreeBSD.org> References: <201212110708.qBB78EWx025288@svn.freebsd.org> <201212121046.43706.jhb@freebsd.org> <CAJ-Vmo=U04GX%2BZyKuzXLwV%2BPpzU6_dm5BCmL=DWfsmhTVAR%2BsA@mail.gmail.com> <201212121658.49048.jhb@freebsd.org> <50C90567.8080406@FreeBSD.org> <50C909BD.9090709@mu.org> <50C91B32.4080904@FreeBSD.org> <50C91CD3.7030900@mu.org> <50C9206D.6080502@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
If a kassert is inviolable, then make it a panic() and include in the default kernel. Right now there's no distinction between "kassert for a condition that shouldn't happen, but we fail gracefully" and "kassert for a condition that shouldn't happen, and we don't handle at all." Ideally we'd create two separate macros to express that, t.hen we should make all kassert's that don't fail gracefully to fail gracefully. Anything left over should've been a panic() in the first place. Right now, with no INVARIANTS compiled in, we don't check KASSERT(). Again, the difference between panic and KASSERT. TL;DR - we're doing panic/assert sub-optimally; we don't have a consistent definition of what an invariant check is; Alfred's work doesn't change that. Adrian On 12 December 2012 16:25, Navdeep Parhar <np@freebsd.org> wrote: > On 12/12/12 16:09, Alfred Perlstein wrote: >> On 12/12/12 4:02 PM, Navdeep Parhar wrote: >>> On 12/12/12 14:48, Alfred Perlstein wrote: >>>> On 12/12/12 2:29 PM, Andriy Gapon wrote: >>>>> Now we get a new middle-ground: get both worse performance (because >>>>> KASSERTs are compiled in) and a risk of harming your data (because >>>>> KASSERTs no longer panic). The upside: there is no panic! There's just >>>>> a log message (or etc). and chance to get more log messages because >>>>> the insanity propagates. And a chance to lose your data (your >>>>> customer's) - but I've already mentioned this. I am not sure that I >>>>> like this kind of middle-ground. >>>> I have a number of points here: >>>> >>>> The most important one being: >>>> 1) without kassert you would still have the bug, just that it would be >>>> unreported. >>>> The upside: there is no panic! There's **NO** log message (or etc). >>>> and chance to get more log messages because the insanity propagates. >>>> >>>> Terrible! >>>> >>>> Let me explain that again: >>>> If you don't compile in KASSERT, then it's not like the condition is >>>> never going to happen. Instead it will just be unreported. >>> A KASSERT() really is for a condition that should never happen. It is >>> primarily useful during development and testing (and when the code is >>> reworked or redesigned). I agree with Andriy here -- a non-fatal assert >>> shouldn't really exist. >> >> >> What do you think happens to a FreeBSD kernel when INVARIANTS is >> compiled in and it trips an assertion after my change? > > I know the new knob has sane defaults. My point was that invariants > should be considered inviolable. A knob that allows for > it's-really-not-supposed-to-fail-but-in-case-it-does... dilutes their > meaning, so it may have been better to introduce a new macro for the > kind of tests you had in mind. I would use it too instead of the if > (!foo) kdb_backtrace() that I often resort to for conditions that I'm > not sure about. > > Regards, > Navdeep
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmomAj9Yu=zrF2uKnAUSt4DDxNpNuyCUUE%2Bg8j4E3=WhP=g>