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

[Christoph Badura <bad%bsd.de@localhost>]

On Mon, Jan 21, 2019 at 10:42:10AM -0500, Greg Troxel wrote:
> Christoph Badura <bad%bsd.de@localhost> writes:
> > 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.

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.

I think the following is better.  Compile-tested only for both #ifdef
conditions, but I think that is OK.

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	21 Jan 2019 15:01:24 -0000
@@ -491,6 +491,15 @@
 	return 0;
 }
 
+#ifdef DEBUG_ROOT
+static int
+debug_rf_containsboot(RF_Raid_t *r, device_t bdv) {
+	if (bdv == NULL)
+		return 0;
+	return rf_containsboot(r, bdv);
+}
+#endif
+
 void
 rf_buildroothack(RF_ConfigSet_t *config_sets)
 {
@@ -509,8 +518,8 @@
 		    cset->ac->clabel->autoconfigure == 1) {
 			sc = rf_auto_config_set(cset);
 			if (sc != NULL) {
-				aprint_debug("raid%d: configured ok\n",
-				    sc->sc_unit);
+				aprint_debug("raid%d: configured ok, rootable %d\n",
+				    sc->sc_unit, cset->rootable);
 				if (cset->rootable) {
 					rsc = sc;
 					num_root++;
@@ -534,8 +543,10 @@
 	/* if the user has specified what the root device should be
 	   then we don't touch booted_device or boothowto... */
 
-	if (rootspec != NULL)
+	if (rootspec != NULL) {
+		DPRINTF("%s: rootspec %s\n", __func__, rootspec);
 		return;
+	}
 
 	/* we found something bootable... */
 
@@ -577,9 +588,9 @@
 			candidate_root = dksc->sc_dev;
 		DPRINTF("%s: candidate root=%p\n", __func__, candidate_root);
 		DPRINTF("%s: booted_device=%p root_partition=%d "
-		   "contains_boot=%d\n", __func__, booted_device,
-		   rsc->sc_r.root_partition,
-		   rf_containsboot(&rsc->sc_r, booted_device));
+			"contains_boot=%d",
+		    __func__, booted_device, rsc->sc_r.root_partition,
+			   debug_rf_containsboot(&rsc->sc_r, booted_device));
 		if (booted_device == NULL ||
 		    rsc->sc_r.root_partition == 1 ||
 		    rf_containsboot(&rsc->sc_r, booted_device)) {