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)) {
