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
