Re: CVS commit: src/sys/kern

[Michael van Elst <mlelstv%serpens.de@localhost>]

On Fri, Jan 11, 2019 at 08:22:23PM +0100, Christoph Badura wrote:
> 
> What exactly did your change do to improve the awareness of these side
> effects?  I'm at a loss when it comes to that.

You noticed that some side effects were already removed.


> 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.

Yes, it no longer panics. Fine with me.


> - 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.

The tftp code is a weird hack, abusing part of the the INSTALL kernel
(== md0) handling and the NFS root handling. So the intention was
to ensure that TFTP booting still works while the code is rewritten.


> - 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.

So your complaint is that I didn't yet fix a bug in the tftpboot code.


> - 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.

It can obviously be overriden by bootspec, that's what bootspec is supposed
to do. It lets the bootloader and thus the user specify a root device without
recompiling a kernel.


> - Overriding rootspec with bootspec doesn't obey the necessary protocol.
>   See my message to tech-kern from yesterday.  (This one is old, though.)

There is no 'protocol'. I'm moving it into something more structured,
that you may call 'protocol' at some point in the future.


> I'm getting the impression, that you do not understand the code very well.

The code is a conglomerate of various hacks and ad-hoc decisions that
always caused problems. See exactly your problem with the raidframe
root hack.


> 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

So there was no review and at the same time you accuse me of not
answering your review.


> 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?

There is no urgency, there is a requirement that the code is seen and
used by others.




Greetings,
-- 
                                Michael van Elst
Internet: mlelstv%serpens.de@localhost
                                "A potential Snark may lurk in every tree."