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

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

On Mon, Jan 21, 2019 at 11:44:04PM +0100, Christoph Badura wrote:
> On Tue, Jan 22, 2019 at 08:32:48AM +1100, matthew green wrote:
> > > > @@ -472,6 +472,9 @@
> > > >  	const char *bootname = device_xname(bdv);
> > > >  	size_t len = strlen(bootname);
> > > >  
> > > > +	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.
> > 
> > infact, bdv has already been dereferenced at this point:
> > the assignment to bootname 3 lines up.
> 
> Argh.  That's what I get for first removing the code I added which
> correctly moved the initialization of bootname and len to after the
> bdv==NULL check and the re-adding it at midnight.

Take 3.  Back to what I intended to submit in the first place.

booted_device is NULL when booting a Xen kernel.  In that case it is OK
for rf_containsboot() to return 0 because the raid set can't contain "it".
This matches the checkes for booted_device == NULL near the two
invocations of rf_containsboot().

I think the check for booted_device == NULL in the last hunk can be
omitted, but I can't easily test right now.  So I marked it XXX and would
leave it for later.

--chris

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	22 Jan 2019 17:30:43 -0000
@@ -469,8 +469,15 @@
 
 static int
 rf_containsboot(RF_Raid_t *r, device_t bdv) {
-	const char *bootname = device_xname(bdv);
-	size_t len = strlen(bootname);
+	const char *bootname;
+	size_t len;
+
+	/* if bdv is NULL, the set can't contain it. exit early. */
+	if (bdv == NULL)
+		return 0;
+
+	bootname = device_xname(bdv);
+	len = strlen(bootname);
 
 	for (int col = 0; col < r->numCol; col++) {
 		const char *devname = r->Disks[col].devname;
@@ -509,8 +516,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 +541,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 +586,12 @@
 			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,
+			   rf_containsboot(&rsc->sc_r, booted_device));
+		/* XXX the check for booted_device == NULL can probably be
+		 * dropped, now that rf_containsboot handles that case.
+		 */
 		if (booted_device == NULL ||
 		    rsc->sc_r.root_partition == 1 ||
 		    rf_containsboot(&rsc->sc_r, booted_device)) {