From owner-svn-src-all@FreeBSD.ORG Mon Dec 17 22:02:56 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 66AF997B; Mon, 17 Dec 2012 22:02:56 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by mx1.freebsd.org (Postfix) with ESMTP id AC3168FC0C; Mon, 17 Dec 2012 22:02:54 +0000 (UTC) Received: by mail-wg0-f52.google.com with SMTP id 12so2839872wgh.31 for ; Mon, 17 Dec 2012 14:02:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=CUuTEzCDxqxxYZ3DWcty/om5SxC8tywmUvEbsLYeVP0=; b=bRtnGYxriAL35qu4Ae2v3MHMtVtmuxycT6I1XurnvHJfKTMgferzJYXJPqhL+eaKvo GwrYPbwhL7Ev+tLYuhNZccHEcNWHZ47MA//QS3pdoLzV/KJch4bQQbZUIA0Ek4YBzLG/ AMY1/6Mu/Ujkxm90XzvKc8fVJcrF6IhKJ1y6pWw6N3DYmaXT5eTSqzH2mDXjy93jGwMf xS2h10su3lHVknjQmOS1sluDwgzs8Hfllr0kA4/+EG6+Ma45jZqjM+HO80U24jOSMN2s ogdT59z++PoDnWX5Zt8thkAbSBbJlAs1L5GUWHGNP/u9A+2yG6Se/Drs49W30hwjW7su SfrQ== MIME-Version: 1.0 Received: by 10.194.83.36 with SMTP id n4mr9426726wjy.59.1355781773725; Mon, 17 Dec 2012 14:02:53 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.217.57.9 with HTTP; Mon, 17 Dec 2012 14:02:53 -0800 (PST) In-Reply-To: <50CF92F0.5020904@FreeBSD.org> References: <201212110708.qBB78EWx025288@svn.freebsd.org> <50CBC285.7060307@mu.org> <20121215161414.V1029@besplex.bde.org> <201212171439.27297.jhb@freebsd.org> <50CF8CE7.4020906@mu.org> <50CF92F0.5020904@FreeBSD.org> Date: Mon, 17 Dec 2012 14:02:53 -0800 X-Google-Sender-Auth: w1ax1vdRzgz6CrFxL3v1BbRbmp0 Message-ID: Subject: Re: svn commit: r244112 - head/sys/kern From: Adrian Chadd To: Andriy Gapon Content-Type: text/plain; charset=ISO-8859-1 X-Mailman-Approved-At: Mon, 17 Dec 2012 22:19:28 +0000 Cc: Alfred Perlstein , src-committers@freebsd.org, John Baldwin , svn-src-all@freebsd.org, Alfred Perlstein , Gleb Smirnoff , Robert Watson , Navdeep Parhar , Bruce Evans , svn-src-head@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Dec 2012 22:02:56 -0000 On 17 December 2012 13:47, Andriy Gapon wrote: > But you see, the following is still illogical _to me_. And this is the core of the problem. A lot of developers are interpreting the KASSERT() conditions as an invariant condition that, if in any way enabled, should be completely trusted, believed and cause an immediate panic(). However, we leave them out on shipping, production kernels. Why's that? Because the contract here is "the code should never hit these situations, so we don't bother checking." It's totally understandable at this point why there's such a huge amount of confusion here. On one hand we have a construct that allows the developer to enforce correct behaviour and panic early if it gets caught out; on the other hand we totally ignore all of that in shipping, production kernels. Why are they there, if we just ship production releases with INVARIANTS disabled? > If one believes that all KASSERTs are bogus then one should compile them out and > just forget about them. There's a lot of perfectly good reasons for leaving KASSERT()s there in the source. The main reason: because they enforce what the correct behaviour SHOULD be. Even if the code doesn't fail the invariant check, it may do so in the future. Why? * because we're not perfect, not at all. The existing code may be subtly buggy. * it protects those bits of code from changes in other areas of code that can cause flow-on effects that trigger these kinds of invariant checks. > If one believes that at least some KASSERTs are such that the execution should > not continue after them, then one should enable all of them and panic on all of > them (better safe than sorry). Yes, but we really should turn as many of those panic conditions into "handle the invariant failure gracefully", since it may be recoverable. Again, right now GENERIC wouldn't panic if those conditions are met, as right now we don't compile them in. > If one is not satisfied with the above choices, then one should meticulously go > over all (or at least some) of KASSERTs and make a decision which KASSERTs are > bogus, which are true and which should be turned into something else (error > checking, warnings, etc). Again, look above. Those assertions mean more than "make sure the code works right at the present moment." However, I'm in the field of "defensive programming" - a lot of those invariants likely could be dealt with inside the function itself. Eg, something like this: KASSERT(n >= 0 || n < max, ("n out of bounds")); if (n < 0 || n >= max) return -EINVAL; .. that kind of thing should be the norm. In any case, _that_ is a separate discussion to all of this. > The proposed solution simultaneously enables all KASSERTs, as if all of them > were true, and doesn't panic on any KASSERT, as if neither of them is fatal > (i.e. they all are bogus to one degree and another). .. GENERIC doesn't panic on the KASSERTs, because they're not even compiled in. Thus, the kernel doesn't panic (much) more or less than before. See below. > P.S. I am not talking about the status quo because the status quo would be > maintained even without the changes in question. The status quo in _behaviour_ is: * shipping kernels don't panic on an invariant check, because we don't even compile them in; * doing the invariant checks shouldn't affect future execution paths (except for whatever changes occur because we're printing them out); * if it crashed in GENERIC because it wasn't caught by a (not compiled in) invariant check, it will very likely still crash with alfreds work - we'll just get told about it beforehand. * if it wouldn't crash in GENERIC after it wasn't caught by a (not compiled in) invariant check, we'll still get a printout of the assertion failure, then execution will continue. The _behaviour_ doesn't change, outside of some extra console IO and some potential for race conditions to occur "differently." Adrian