Christoph Badura <b...@bsd.de> 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.