Re: patch: debug instrumentation for dev/raidframe/rf_netbsdkintf.c

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

Christoph Badura <bad%bsd.de@localhost> writes:

> On Mon, Jan 21, 2019 at 04:24:49PM -0500, Greg Troxel wrote:
>> Separetaly from debug code being careful, if it's a rule that bdv can't
>> be NULL, it's just as well to put in a KASSERT.  Then we'll find out
>> where that isn't true and can fix it.
>
> I must not be getting something.  If rf_containsboot() is passed a NULL
> pointer, it will trap with a page fault and we can get a stacktrace from
> ddb.  If we add a KASSERT it will panic and we can get a stacktrace from
> ddb.  I don't see where the benefit in that is.

The benefit is that the panic from the KASSERT is cleaner, and it
documents for readers of the function that the author believes it is a
rule.   And it will definitely fault even on machines will can
dereference NULL - that is technically if not practically architecture dependent.

> Do you think we should add a KASSERT to document that rf_containsboot()
> does expect a valid pointer? I'd see value in that and would go ahead with
> it.

Yes.  Basically, in any kernel function, if there is a requirement that
a pointer be non-NULL, then there should be a KASSERT and the code
should then feel free to assume it is valid.

When a KASSERT is hit, the user gets a message with the KASSERT
expression and the source file/line, instead of a page fault traceback.
It's very easy and quick to go from that printout to the KASSERT that
failed.

Plus, adding the KASSERT, or talking about adding it, is a good way to
check if there is consensus among the other developers that this really
is a rule.  In NetBSD, people are really good at telling you you're
wrong!