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

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

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

>> > +	if (bdv == NULL)
>> > +		return 0;
>> > +
>> 
>> This looked suspicious, even before I read the code.
>> The question is if it is ever legitimate for bdv to be NULL.
>
> That is an excellent point.  The short answer is, no it isn't.  And it
> never was NULL in the code that used it.  I got a trap into ddb because of
> a null pointer deref in the DPRINTF that I changed (in the 4th hunk of my
> patch).
>
>> I am a fan of having comments before every function declaring their
>> preconditions and what they guarantee on exit.  Then all uses can be
>> audited if they guarantee the the preconditions are true.  This approach
>> is really hard-core in eiffel, known as design by contract.
>
> Yes, I totally agree.  Also to the rest of your message that I didn't quote.
>
> When I prepared the patch yesterday I was about to delete the above change
> because at first I couldn't remember why I added it ~3 weeks ago.  That
> should have raised a big fat warning sign.
>
> I thought about adding a comment after I read your private mail
> earlier today.  In the end I decided it is better to not change
> rf_containsboot() and instead introduce a wrapper for the benefit of the
> DPRINTF.

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.