Re: CVS commit: src/sys/kern

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

On Fri, Jan 11, 2019 at 12:41:36AM +0100, Michael van Elst wrote:
> On Thu, Jan 10, 2019 at 05:50:30PM +0100, Christoph Badura wrote:
> > 
> > If you hadn't reversed the order of
> > 
> > tftproot_dhcpboot()
> > if (md_is_root) ...
> > rootspec = bootspec
> > 
> > that would wouldn't have need fixing because tftproot_dhcpboot() sets
> > md_is_root.
> 
> It needed fixing because:

You are quoting slightly out of context, but it was poor wording on my
part too.  What I meant was "if you had not removed the old functionality
it wouldn't have need fixing".  Apologies.

> - md0 doesn't exist before it is opened for the first time.

Yes, that's why the old code did open it.  But you still haven't restored
the full functionality.  Or explained why you dropped it.

> - that's one reason why setting rootspec didn't work as intendend

> > Why was the order reversed anyway?  Can you please explain?
> 
> It's not about the order but about separating different cases.

Well, if it is not about the order, then you do not need to change the
order, don't you?

> The setroot code is riddled with side effects.

Sure it is.  A good starting point would have been to add comments that
explain the side effects.  And changing functions to not rely on side
effects.  All without changing functionality.

What exactly did your change do to improve the awareness of these side
effects?  I'm at a loss when it comes to that.

Here's more defects and functionality changes:

- The old code did "if (md_is_root) configure md0 as root or panic".  Now
  it does something different.
- The old code did "if (tftproot_dhcpboot(bootdv) != 0) boothowto |= RB_ASKNAME;".
  Now it does something different.  It isn't even guaranteed that
  RB_ASKNAME gets set, because bootdv is likely not null.
- tftproot_dhcpboot() returning 0 does not always indicate that the
  memory disk was successfully initialized.  md_is_root being set, however,
  reliably does.  Now md0 is configured anyway.
- The order *does* matter, because the first thing tftproot_dhcpboot()
  does is check for rootspec != NULL.  In the old code rootspec should
  only have been set by config(8).  Now it can be overridden by bootspec.
  Another functional change.
- Overriding rootspec with bootspec doesn't obey the necessary protocol.
  See my message to tech-kern from yesterday.  (This one is old, though.)

I'm getting the impression, that you do not understand the code very well.
It seems to me that you did not understand the protocol regarding changing
bootspec when you introduced the code to override it 4 years ago.
It seems to me that you did not test the changes related to md_is_root r1.221
at all.

> > Can you also please respond to the review remarks?
> I asked you for a review, it never appeared.

You asked me privately to review a file for you with no indication
whatsoever that there was any urgency to it or that you wanted to commit
soon.  Nevertheless I made time that same day to review it, but ran out of
time to write it up for you.  You didn't come back to me before
committing, so I commented publically.

https://mail-index.netbsd.org/source-changes-d/2019/01/05/msg010893.html

> I am working on this since a few months and committed it now before the branch
> to -9 after asking (several times) for a review.

You did ask several times for review?  Am I supposed to find these
requests in the tech-kern list archive?  Because I can't.  Of the 48
messages from you since 1/1/18 not one did so much as hint at you having
changes that you want reviewed or commit.

Anyway, what was the urgency?  Were you somehow under the impression that
the branch was taking place last weekend and you had to rush these
poorly tested and poorly understood changes in without you having the time to
contact the private reviewer about the status?

--chris