Re: CVS commit: src/sys/kern

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

On Sat, Jan 05, 2019 at 09:39:56AM +0000, Michael van Elst wrote:
> Modified Files:
> 	src/sys/kern: kern_subr.c
> 
> Log Message:
> Refactor setroot, no functional change intended.

I'm surprised this change to a part as critical as set_root() didn't go
through tech-kern for review.  Particularly because this does change
functionality.

You asked me privately for review and I spend Thursday afternoon on that
but haven't had time to write it up yet.  I was just sitting down to do
that when I noticed your commit.  You could have contacted me about the
status of the review.  I was going to offer you to discuss this face to
face at MeKA.  I'm still offering to do that.

Also, may I ask what the purpose of these changes is?  Is there,
perhaps, a plan to make further changes?
XXX Or was it done just because you didn't like the current code.

The good news is that the test cases I was dealing with earlier this week
still work and break as before.  Also the change I mentioned on ICB still
does fix the broken case.

Index: src/sys/kern/kern_subr.c
diff -u src/sys/kern/kern_subr.c:1.220 src/sys/kern/kern_subr.c:1.221
--- src/sys/kern/kern_subr.c:1.220	Sun Oct  7 11:24:16 2018
+++ src/sys/kern/kern_subr.c	Sat Jan  5 09:39:56 2019
 	 */
-	if (md_is_root) {
-		int md_major;
-		dev_t md_dev;
-
-		bootdv = NULL;
-		md_major = devsw_name2blk("md", NULL, 0);
-		if (md_major >= 0) {
-			md_dev = MAKEDISKDEV(md_major, 0, RAW_PART);
-			if (bdev_open(md_dev, FREAD, S_IFBLK, curlwp) == 0)
-				bootdv = device_find_by_xname("md0");
-		}
-		if (bootdv == NULL)
-			panic("Cannot open \"md0\" (root)");
-	}

Here, functionality got removed without explanation.

 
+	do {
+		if (boothowto & RB_ASKNAME)
+			setroot_ask(bootdv, bootpartition);
+		else
+			setroot_root(bootdv, bootpartition);
+

I don't like the style of this.  It conveys the false impression that
bootdv and bootpartition are the only input parameters to these functions.
That is not true, though.  They still take the globals as input and set a
bunch of them.  At least that much was clear from the old code.


setroot_ask() now:

+		dv = getdisk(buf, len, bootpartition, &nrootdev, 0);
+		if (dv != NULL) {
+			rootdv = dv;
+			break;
+		}
+	}
+	rootdev = nrootdev;

Here the global rootdev becomes the new canoncical place...
 
+				ndumpdev = MAKEDISKDEV(major(nrootdev),
+				    DISKUNIT(nrootdev), 1);

... but you continue to use the local variable nrootdev.  That doesn't
help clarity.

+		if (len == 4 && strcmp(buf, "halt") == 0)
+			cpu_reboot(RB_HALT, NULL);
+		else if (len == 6 && strcmp(buf, "reboot") == 0)
+			cpu_reboot(0, NULL);
 #if defined(DDB)
+		else if (len == 3 && strcmp(buf, "ddb") == 0) {
+			console_debugger();
+		}
 #endif

This hunk of code is an obvious candidate to factor out from here and
parsedisk().

I do appreciate the updated comments.

The comment block about the "quirt" before the call to setroot_nfs()
would be better before the the function definition itself as that's where
one wants the explanation of the behaviour.

setroot_ask() doesn't have any comment, documenting what it does.  It
would benefit from one, though.

The comment before the definition of setroot_root documents a return value
from a void function.

This was perhaps unfinished work?

The old setroot() was a large function.  But it had a clear structure of
multiple steps.  It had the advantage of not hiding which globals were
modified where in the function.  Also, it didn't take me more than an hour
to understand what it was doing.  In contrast to needing more to an hour
to review you proposed change.

My advice is to revert this, go back to the drawing board, come up with
clear goals that bring us forward, and discuss the proposal on tech-kern.

Also, I have changes to setroot() that I want to get out of my local tree,
so a decision to revert or not should be made soonish.

--chris