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

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

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

> Here is some instrumentation I found useful during my recent debugging.
> If there are no objections, I'd like to commit soon.
>
> The change to rf_containsroot() simplifies the second DPRINTF that I added.
>
> Index: rf_netbsdkintf.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/raidframe/rf_netbsdkintf.c,v
> retrieving revision 1.356
> diff -u -r1.356 rf_netbsdkintf.c
> --- rf_netbsdkintf.c	23 Jan 2018 22:42:29 -0000	1.356
> +++ rf_netbsdkintf.c	20 Jan 2019 22:32:14 -0000
> @@ -472,6 +472,9 @@
>  	const char *bootname = device_xname(bdv);
>  	size_t len = strlen(bootname);
>  
> +	if (bdv == NULL)
> +		return 0;
> +
>  	for (int col = 0; col < r->numCol; col++) {
>  		const char *devname = r->Disks[col].devname;
>  		devname += sizeof("/dev/") - 1;

This looked suspicious, even before I read the code.

The question is if it is ever legitimate for bdv to be NULL.

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.

In NetBSD, many functions have KASSERT at the beginning.  This checks them
(under DIAGNOSTIC) but it also is a way of documenting the rules.

From a quick glance at the code it seems obvious that it's not ok to
call these functions with a NULL bdv.

So if bdv is an argument and not allowed to be NULL, then early on in
that function, where you check/return, there should be

  KASSERT(bdv != NULL)

Not really on point, but as a caution there should be no behavior change
in any function under DIAGNOSTIC, if the code is bug free and
preconditions are met.  So "if something we can rely on isn't true,
panic" is fine, but many other things rae not.