Hello, Thank you Cameron (47516). +1!
Carry on! -r > Hello, > > First I was asked if a certain developer could talk to me regarding ARM3. I > said yes, never heard back, and then saw commits done to pfnlist.c. While > interesting and useful code, some communication would've been nice in order > to talk about the goals, since much of that code is in flux (I feel bad when > someone improves a function that I will later remove, as two of the improved > functions are only temporary -- the changes to the other functions will > remain, obviously). Normally, this would not have been a problem, if not for > the strange formatting and the fact this code was now broken and causing a > boot regression. > > I showed up in the #reactos-dev channel and instructed developers on how to > fix the problem: remove the entire function and use MiInsertPageInFreeList > instead, or fix MiInitializePagingLists to perform its work at DISPATCH_LEVEL > instead, by acquiring the PFN lock as it should do. > > None of this was done, instead "janderwald", in 47514, "Fixed" the ASSERT by > saying "Any IRQL lower than DISPATCH_LEVEL" is okay. Janderwald, whoever you > are, do you even understand this code? Are you even a kernel-mode developer? > Does it make to you that the code would need to assert for low IRQL? The > whole point is to validate that the PFN lock is being held. Even if you are > not a kernel-mode dev, isn't it clear that all the other assertions are also > checking for DISPATCH_LEVEL+? > > And even if that's not clear, I had written down the solution in the > #reactos-dev channel, expecting that probably not many people would figure it > out. > > So I guess some lessons to learn for this project: > > 1) If you modify someone's code, make sure you test your changes (and > preferably don't mess up the formatting) so their code is not now blamed for > a boot regression. > 2) If someone tells you how to fix their code after it's been messed up, > apply the fix. > 3) If you choose to roll your own fix, write something useful instead of > breaking the function (logic-wise) even more. > > Again, I'm afraid the problem is most of you are students and don't have work > ethics of real employees -- this seems to be the root of many problems this > project has had since the last 2 years I've been watching and working on the > ARM port. > > Next steps: > > - Janderwald: remove your ASSERT hack. Either 1) Remove the ASSERT (stop > checking for an invariant that doesn't happen) or 2) Fix the caller (so the > invariant holds) > - Arthur: changes are appreciated, but let's talk more so you don't end up > improving dead code. > > Carry on! > > -r > > _______________________________________________ > Ros-dev mailing list > [email protected] > http://www.reactos.org/mailman/listinfo/ros-dev _______________________________________________ Ros-dev mailing list [email protected] http://www.reactos.org/mailman/listinfo/ros-dev
